Skip to content

Chris/schy 45 datastreamreplicator for python#58

Merged
cbrady merged 36 commits intomainfrom
chris/schy-45-datastreamreplicator-for-python
Apr 17, 2026
Merged

Chris/schy 45 datastreamreplicator for python#58
cbrady merged 36 commits intomainfrom
chris/schy-45-datastreamreplicator-for-python

Conversation

@cbrady
Copy link
Copy Markdown
Collaborator

@cbrady cbrady commented Mar 25, 2026

No description provided.

@cbrady cbrady force-pushed the chris/schy-45-datastreamreplicator-for-python branch from cf3013c to 69d2a75 Compare March 26, 2026 00:30
@cbrady cbrady marked this pull request as ready for review March 26, 2026 01:17
@cbrady cbrady requested a review from a team as a code owner March 26, 2026 01:17
Copy link
Copy Markdown
Collaborator

@bpapillon bpapillon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid checking in the wasm file? I remember we had a similar convo about this during one of the last SDKs, my though was that since we have another repo responsible for producing that wasm binary, we should be able to pull it in at build time.

Related - we should probably figure out a way to do some kind of build test, as I'm not certain how the packaging and publishing system here determines which files to include. We need to make sure that the wasm file, however we pull it in, ends up getting shipped with the published Python package.

Comment thread pyproject.toml Outdated
Comment thread src/schematic/client.py
@cbrady cbrady requested a review from bpapillon March 27, 2026 15:48
bpapillon
bpapillon previously approved these changes Apr 1, 2026
@cbrady cbrady force-pushed the chris/schy-45-datastreamreplicator-for-python branch from 8214655 to fd7ddf5 Compare April 3, 2026 17:28
Copy link
Copy Markdown
Collaborator

@bpapillon bpapillon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think our goal here should be to get to feature parity across the SDKs aside from OpenFeature which we can ignore, and parameter defaults which we've discussed separately we can do a sweep after this to get into alignment. So we should implement check_flags here as well.

Also, let's comp the test cases against reference implementations like Node, Ruby, Go - I think we are lighter here.

Comment thread src/schematic/datastream/datastream_client.py Outdated
@cbrady cbrady requested a review from bpapillon April 7, 2026 16:12
Comment thread src/schematic/client.py Outdated
Comment thread src/schematic/client.py Outdated
@cbrady cbrady requested a review from bpapillon April 8, 2026 03:43
bpapillon
bpapillon previously approved these changes Apr 13, 2026
Copy link
Copy Markdown
Collaborator

@bpapillon bpapillon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this may be minor enough to do as a followup, but we should make sure we close the _health_check_client http client in DataStreamClient.close so we don't leak the connection pool. probably not a huge issue in practice since DataStreamClient.close is probably only getting called when the service that runs the schematic client singleton is shutting down, but I could see it being a problem in integration tests or things like that where maybe we need to shut down the client in between usages.

Copy link
Copy Markdown
Collaborator

@bpapillon bpapillon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always forget that the python client is actually two clients. we should support datastream in the sync client as well

@cbrady cbrady force-pushed the chris/schy-45-datastreamreplicator-for-python branch from cf0465e to e6f307e Compare April 14, 2026 19:25
@cbrady
Copy link
Copy Markdown
Collaborator Author

cbrady commented Apr 15, 2026

@bpapillon yeah the reason i didn't add it as part of the SchematicClient is because the datastream functionality is async by nature. there is a way to accomplish it but there are a few issues, many python webservers use fork which would break the thread running the datastream, it adds overhead to the requests (not a ton but enough to note), Exception/traceback quality degrades, and shutdown need to be strictly ordered which could be broken by our customer's own shutdown routines.

@cbrady cbrady requested a review from bpapillon April 16, 2026 00:07
bpapillon
bpapillon previously approved these changes Apr 17, 2026
Copy link
Copy Markdown
Collaborator

@bpapillon bpapillon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'm fine merging without that support. We do have datastream support in some other clients that have similar issues, so we should probably figure out a way to do this in the sync client at some point, but for now lets just progress and note this in the docs.

@cbrady cbrady merged commit ec2d6ae into main Apr 17, 2026
4 checks passed
@cbrady cbrady deleted the chris/schy-45-datastreamreplicator-for-python branch April 17, 2026 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants