Streaming support #365

Tymek · 2025-08-12T16:43:06Z

Description

Added streaming support.

Fixes internal issue CTO-249

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Unit tests
Spec Tests
Integration tests / Manual Tests

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

Tymek · 2025-08-19T14:19:01Z

UnleashClient/__init__.py

+                    else:
+                        # MODE: offline
+
+                        job_args = base_job_args
+                        job_func = load_features


Python SDK sets up periodic update on bootstrapped toggles. I don't think this is consistent across SDKs

Yep, it's a Python specific thing. It's to work around the incompatibility between the threading model used in this SDK and server tech like gunicorn which uses a forked process model.

coveralls · 2025-08-19T17:07:33Z

Pull Request Test Coverage Report for Build 17217536564

Details

112 of 134 (83.58%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-2.6%) to 92.397%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
UnleashClient/init.py	24	28	85.71%
UnleashClient/streaming/event_processor.py	33	37	89.19%
UnleashClient/streaming/connector.py	51	65	78.46%

Totals
Change from base Build 16809197061:	-2.6%
Covered Lines:	559
Relevant Lines:	605

💛 - Coveralls

kwasniew

do we have any unit or integration tests showing how streaming works?

UnleashClient/__init__.py

UnleashClient/streaming/manager.py

…ingEventProcessor

for more information, see https://pre-commit.ci

kwasniew

I still don't see any tests

kwasniew · 2025-08-22T07:38:26Z

UnleashClient/__init__.py

@@ -111,7 +113,7 @@ class UnleashClient:
    :param event_callback: Function to call if impression events are enabled.  WARNING: Depending on your event library, this may have performance implications!
    """

-    def __init__(
+    def __init__(  # noqa: PLR0915


why was it added?

"too many statements" linting exception

26 is... well... a LOT of parameters. I'm open to being told this isn't the right time but gosh this feels like we should fix this at this point

As the person most responsible for this, I would tend to agree but any change would be (kind of by definition) backwards incompatible. I would be on board with updating that but I think it would be a major version bump. (Unless you want a v2 client object...but that way lies maintenance headaches).

If I did this nowadays (with the benefit of like 10 years more experience), I would probably split between required and pseudo-required arguments (url an headers respectively) and put some of the less important/optional configuration (jitter) in an options dataclass or similar.

kwasniew · 2025-08-22T07:49:21Z

UnleashClient/__init__.py

@@ -136,6 +138,8 @@ def __init__(
        scheduler_executor: Optional[str] = None,


In Ruby we managed to re-use fetcher_scheduled_executor that is either polling toggle fetcher or streaming. Check usage of https://github.com/Unleash/unleash-ruby-sdk/blob/main/lib/unleash/client.rb#L14 for details. @sighphyre started a great discussion about it here https://github.com/Unleash/unleash-ruby-sdk/pull/248/files#r2262867667. Since Python is similar to Ruby it should be doable too. In Java it was too difficult. In Node I will try to migrate towards this approach too in a subsequent PR.

Agreed! This SDK is a bit more complex than Ruby but the abstractions are more or less sane so I don't think this should be massively challenging

kwasniew · 2025-08-22T07:54:06Z

UnleashClient/__init__.py

@@ -169,6 +173,9 @@ def __init__(
        self.unleash_verbose_log_level = verbose_log_level
        self.unleash_event_callback = event_callback
        self._ready_callback = build_ready_callback(event_callback)
+        self.experimental_mode = experimental_mode


I don't like the fact that we have explosion of fields that work for either polling or streaming. In clear OO design we'd have swappable mechanism for one fetching strategy at a time. I know in Java and .NET we also mix but I plan to fix this in Node. Maybe worth investigating this option in Python?

UnleashClient/streaming/event_processor.py

kwasniew · 2025-08-22T08:40:14Z

UnleashClient/streaming/connector.py

+        self._headers = {**headers, "Accept": "text/event-stream"}
+        self._timeout = request_timeout
+        self._on_ready = on_ready
+        self._sse_factory = sse_client_factory


creating testability/swapping option is only valuable if you gonnd use this option. From my review it looks like we exposed this option (increased complexity) but we never exercised this option. I think in the production implementation of a connector.py we only need to wrap LD client. More detail why too much dependency inversion can be harmful: https://dannorth.net/blog/cupid-the-back-story/#dependency-inversion-principle

+1 for this. Too much injection is how you end up with a need for IoC containers shudder. No one wants that

kwasniew · 2025-08-22T08:46:11Z

tests/unit_tests/streaming/test_client_streaming.py

+        self.data = data
+
+
+class FiniteSSEClient:


The current approach with FiniteSSEClient and factory pattern is:

More isolated (unit test style)

Faster but less realistic

Requires production code changes (factory parameter)

Network-level testing (check Java and Ruby, Node is not doing this) would be more realistic because:

Tests the actual SSE client library behavior

Verifies correct handling of SSE format

Catches integration issues

More confidence in production behavior

This code uses responses library for polling mode tests.
It could theoretically mock SSE responses

ruby example: https://github.com/Unleash/unleash-ruby-sdk/pull/248/files#diff-9f5de0ba29bb75d4bbc376507bb2ecbde605866a7a2bfcdf550c370298ae8f54R766

Agree with this, this is too complex and too delicate of a feature to only lean on unit tests. These don't give me enough confidence that this features works correctly and that we're not going to completely break it by accident in the future

UnleashClient/streaming/connector.py

Tymek · 2025-08-25T10:26:59Z

UnleashClient/__init__.py

+                        job_args = {
+                            **base_job_args,
+                            "url": self.unleash_url,
+                            "app_name": self.unleash_app_name,
+                            "instance_id": self.unleash_instance_id,
+                            "headers": {
+                                **base_headers,
+                                "unleash-interval": self.unleash_refresh_interval_str_millis,
+                            },
+                            "custom_options": self.unleash_custom_options,
+                            "request_timeout": self.unleash_request_timeout,
+                            "request_retries": self.unleash_request_retries,
+                            "event_callback": self.unleash_event_callback,
+                            "project": self.unleash_project_name,
+                        }
+
+                        job_func: Callable
+                        job_func = fetch_and_load_features


My goal here was to avoid larger refactor. I'd like to extract 'mode' (fetching strategy / connector) into a separate layer, but in this SDK fetching runs on the same 'scheduler' as metrics. Decoupling this should be done in another PR in my opinion.

I agree it belongs in a different PR but I think that PR should come before this one. This function is now 150 lines, in the Python world that's a monster of a function. I think we're starting to see that show itself in the form of comments like "MODE: Polling" which are trying to add order to the chaos here. IMO this is now too complex and needs to be split up. I want to say the Poller vs Streamer abstraction is probably what we want here

sighphyre

Okay so left some initial thoughts. I think we should consider refactoring before we do this PR so we can get a reasonable abstraction that swaps out a Poller for a EventStreamer. There's also 7 lint disables in this PR, currently the SDK has 6, most of which are in tests, so we're more than doubling the linter disables which is a smell for me. I strongly feel we should fix that before merging. I'm not aware of any massive urgency around this feature here so we should be able to spend a little bit more time on this so that future work isn't difficult

sighphyre · 2025-08-21T06:47:13Z

UnleashClient/streaming/manager.py

+        if self._thread:
+            self._thread.join(timeout=5)
+
+    def _run(self):  # noqa: PLR0912


I think Ruff has a point, there's a lot of branching here. I think we should fix that and break this up rather than disable linting

sighphyre · 2025-08-25T14:48:57Z

requirements.txt

@@ -6,7 +6,7 @@ mmhash3
 python-dateutil
 requests
 semver
-yggdrasil-engine
+yggdrasil-engine>=1.0.0



Need a launchdarkly-eventsource here as well

sighphyre · 2025-08-25T15:00:59Z

UnleashClient/__init__.py

@@ -136,6 +138,8 @@ def __init__(
        scheduler_executor: Optional[str] = None,
        multiple_instance_mode: InstanceAllowType = InstanceAllowType.WARN,
        event_callback: Optional[Callable[[BaseEvent], None]] = None,
+        experimental_mode: Optional[dict] = None,


It's really unclear to me how to use this. I think it wants a proper type on it so that end users can leverage their type checker

sighphyre · 2025-08-25T15:05:13Z

UnleashClient/__init__.py

@@ -267,8 +274,10 @@ def initialize_client(self, fetch_toggles: bool = True) -> None:
            try:
                base_headers = {
                    **self.unleash_custom_headers,
+                    **APPLICATION_HEADERS,


Why these changes here? What does this fix? What test breaks without it and what test ensures that it now correctly works?

sighphyre · 2025-08-25T15:14:42Z

UnleashClient/__init__.py

-                    kwargs=job_args,
+
+                # Decide upstream connection mode
+                mode = (


It's not a major issue but I think the belongs somewhere else. Be cool to see a a function that returns an Enum of POLLING | OFFLINE | STREAMING or something like that

sighphyre · 2025-08-26T07:39:50Z

tests/unit_tests/streaming/test_client_streaming.py

+        self.data = data
+
+
+class FiniteSSEClient:


Agree with this, this is too complex and too delicate of a feature to only lean on unit tests. These don't give me enough confidence that this features works correctly and that we're not going to completely break it by accident in the future

sighphyre · 2025-08-26T07:41:01Z

UnleashClient/__init__.py

@@ -111,7 +113,7 @@ class UnleashClient:
    :param event_callback: Function to call if impression events are enabled.  WARNING: Depending on your event library, this may have performance implications!
    """

-    def __init__(
+    def __init__(  # noqa: PLR0915


26 is... well... a LOT of parameters. I'm open to being told this isn't the right time but gosh this feels like we should fix this at this point

sighphyre · 2025-08-26T07:50:35Z

UnleashClient/__init__.py

@@ -136,6 +138,8 @@ def __init__(
        scheduler_executor: Optional[str] = None,


Agreed! This SDK is a bit more complex than Ruby but the abstractions are more or less sane so I don't think this should be massively challenging

sighphyre · 2025-08-26T07:53:24Z

UnleashClient/__init__.py

+                    else:
+                        # MODE: offline
+
+                        job_args = base_job_args
+                        job_func = load_features


Yep, it's a Python specific thing. It's to work around the incompatibility between the threading model used in this SDK and server tech like gunicorn which uses a forked process model.

UnleashClient/streaming/event_processor.py

initial streaming support

fe6832e

thomasheartman added this to Issues and PRs Aug 12, 2025

github-project-automation bot moved this to New in Issues and PRs Aug 12, 2025

Tymek added 3 commits August 12, 2025 19:03

streaming example

db0a837

delta polling

86dd667

formatting

f2eab4f

Tymek force-pushed the feat/streaming branch from 0252e92 to f2eab4f Compare August 12, 2025 17:09

update example

b25e820

Tymek force-pushed the feat/streaming branch from ee3f641 to b25e820 Compare August 12, 2025 17:17

gastonfournier moved this from New to In Progress in Issues and PRs Aug 13, 2025

Tymek added 3 commits August 19, 2025 11:00

refactor: consolidate job arguments

5c5e542

refactor: simplify mode selection

f7596b7

refactor: update mode selection logic

394ac5f

Tymek commented Aug 19, 2025

View reviewed changes

Tymek added 3 commits August 19, 2025 17:01

refactor: enhance job scheduling logic for streaming and polling modes

6bbc162

fix: lint errors

fecb283

fix: update client specification version to 5.2.2

9ffc01a

chore: remove streaming example and its dependencies

c4ffef3

Tymek force-pushed the feat/streaming branch from 2bba842 to c4ffef3 Compare August 19, 2025 17:08

Tymek requested a review from kwasniew August 19, 2025 17:37

Tymek marked this pull request as ready for review August 20, 2025 07:47

update yggdrasil

2cafba5

kwasniew reviewed Aug 20, 2025

View reviewed changes

UnleashClient/__init__.py Outdated Show resolved Hide resolved

kwasniew reviewed Aug 20, 2025

View reviewed changes

UnleashClient/streaming/manager.py Outdated Show resolved Hide resolved

kwasniew reviewed Aug 20, 2025

View reviewed changes

UnleashClient/streaming/manager.py Outdated Show resolved Hide resolved

Tymek added 3 commits August 20, 2025 14:04

fix: polling

ac8ce42

remove delta-polling

23be09e

refactor: replace StreamingManager with StreamingConnector and Stream…

2072ab4

…ingEventProcessor

Tymek and others added 2 commits August 21, 2025 11:15

fix: add retry strategies and error handling

d6b0293

[pre-commit.ci] auto fixes from pre-commit.com hooks

2beb595

for more information, see https://pre-commit.ci

Tymek requested a review from kwasniew August 21, 2025 09:29

kwasniew reviewed Aug 21, 2025

View reviewed changes

Tymek added 2 commits August 21, 2025 14:50

add tests

3e4500b

lint

a76d913

Tymek force-pushed the feat/streaming branch 2 times, most recently from 9f44f35 to 364a973 Compare August 21, 2025 14:43

Tymek requested a review from kwasniew August 21, 2025 14:44

refactor tests

3cf0222

Tymek force-pushed the feat/streaming branch from 364a973 to 3cf0222 Compare August 21, 2025 14:54

kwasniew reviewed Aug 22, 2025

View reviewed changes

UnleashClient/streaming/event_processor.py Show resolved Hide resolved

kwasniew reviewed Aug 22, 2025

View reviewed changes

UnleashClient/streaming/connector.py Outdated Show resolved Hide resolved

kwasniew reviewed Aug 22, 2025

View reviewed changes

UnleashClient/streaming/connector.py Outdated Show resolved Hide resolved

remove heartbeat

f626b57

Tymek force-pushed the feat/streaming branch from 96d811c to f626b57 Compare August 25, 2025 09:20

add TODO for backup file in event processing

4fcfce9

Tymek commented Aug 25, 2025

View reviewed changes

Tymek requested a review from sighphyre August 25, 2025 18:35

sighphyre reviewed Aug 26, 2025

View reviewed changes

Tymek marked this pull request as draft August 26, 2025 08:10

		@@ -136,6 +138,8 @@ def __init__(
		scheduler_executor: Optional[str] = None,

Streaming support #365

Are you sure you want to change the base?

Streaming support #365

Uh oh!

Conversation

Tymek commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 17217536564

Details

💛 - Coveralls

Uh oh!

kwasniew left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kwasniew left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanklee86 Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sighphyre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Tymek commented Aug 12, 2025 •

edited

Loading

coveralls commented Aug 19, 2025 •

edited

Loading

ivanklee86 Aug 26, 2025 •

edited

Loading