Adding logging to SDK #262
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a default logging initializer (env-configurable), expands logging across runtime flows, changes Runtime._worker to accept a per-worker index and creates per-worker tasks, makes enqueue exceptions propagate, and adds many comprehensive tests; minor type-ignore edits in one test. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant R as Runtime
participant L as Logger
participant SM as StateManager (HTTP)
participant S as Secrets (HTTP)
participant N as Node Impl
rect rgba(220,235,255,0.35)
note over R,L: Startup: default logging + env config
R->>L: _setup_default_logging()
R->>R: _set_config_from_env() (logs fallbacks)
R->>SM: _register(nodes) (logs request/response)
SM-->>R: registration response (logged)
end
par Workers[0..N-1]
rect rgba(230,255,230,0.35)
note over R,N: Per-worker indexed loop (idx logged)
R->>R: _worker(idx) start (log idx)
loop Poll/Enqueue/Execute
R->>SM: poll states
SM-->>R: states
R->>R: _enqueue(states)
alt enqueue failure
R->>L: log failure
R--x R: exception re-raised
else enqueue success
R->>S: _get_secrets(state)
S-->>R: secrets (or 404 logged)
R->>N: execute(inputs,secrets) (log start/result)
alt execution success
R->>SM: _notify_executed(state, outputs)
else execution error
R->>SM: _notify_errored(state, traceback)
end
end
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @NiveditJain, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces comprehensive logging capabilities to the Python SDK's Runtime component. It establishes a flexible default logging configuration that respects existing user setups and environment variables, and integrates detailed informational and error logging across crucial operations such as runtime initialization, node registration, state enqueueing, and worker execution. This enhancement significantly improves observability and debugging for the SDK.
Highlights
- Configurable Default Logging: A new _setup_default_logging function is introduced to configure basic logging if no handlers are already present, allowing customization via EXOSPHERE_LOG_LEVEL and EXOSPHERE_DISABLE_DEFAULT_LOGGING environment variables.
- Enhanced Operational Visibility: Informational logs have been added for key runtime events, including the use of environment variables for configuration, node registration status, and the enqueueing of states.
- Improved Error Diagnostics: Error logging is now more robust, capturing failures during node registration, state enqueueing, and worker execution, with tracebacks included for execution errors.
- Detailed Worker Activity Logging: The _worker function now logs its startup, the execution of individual states, and the retrieval of secrets and outputs, providing better insight into processing flow.
- Refined Exception Handling: Exceptions in _enqueue_call and _enqueue are now explicitly re-raised after logging, ensuring proper error propagation.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Code Review
This pull request introduces logging to the SDK, which is a valuable addition for observability. The implementation of a default logging setup that respects user configurations is well-done. However, I've identified a few critical issues in the new logging code. There are a couple of instances where log messages will cause KeyError exceptions due to incorrect dictionary key usage. Additionally, there's a potential UnboundLocalError in an exception handler that could crash worker tasks. Addressing these issues is crucial for the stability of the runtime.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
python-sdk/exospherehost/runtime.py (3)
170-198: Registration logs are fine; consider reducing payload verbosity.Listing every node at INFO can get noisy in large deployments. Prefer logging the count at INFO and full names at DEBUG.
- logger.info(f"Registering nodes: {[f\"{self._namespace}/{node.__name__}\" for node in self._nodes]}") + logger.info("Registering %d nodes for namespace %s", len(self._nodes), self._namespace) + logger.debug("Nodes: %s", [f"{self._namespace}/{node.__name__}" for node in self._nodes]) ... - logger.info(f"Registered nodes: {[f\"{self._namespace}/{node.__name__}\" for node in self._nodes]}") + logger.info("Registered %d nodes for namespace %s", len(self._nodes), self._namespace) + logger.debug("Registered nodes: %s", [f"{self._namespace}/{node.__name__}" for node in self._nodes])
253-259: Optional: consistent error handling for notify paths.
_enqueue_callraises on non-200, but_notify_executed/_notify_erroreddo not. Consider a consistent policy (either log-and-continue or raise) and possibly retries with backoff to handle transient errors.
200-220: Efficiency: consider a shared aiohttp session.Creating a new
ClientSessionper request is expensive. Consider a lifecycle-managed session created in_startand closed on shutdown.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
python-sdk/exospherehost/runtime.py(12 hunks)python-sdk/tests/test_runtime_validation.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
python-sdk/tests/test_runtime_validation.py (2)
python-sdk/tests/test_base_node.py (2)
Outputs(10-11)Inputs(7-8)python-sdk/exospherehost/node/BaseNode.py (1)
BaseNode(6-100)
python-sdk/exospherehost/runtime.py (1)
python-sdk/exospherehost/node/BaseNode.py (4)
BaseNode(6-100)_execute(70-83)Inputs(29-35)Secrets(45-68)
🔇 Additional comments (4)
python-sdk/exospherehost/runtime.py (4)
12-12: LGTM: module-level logger.Using a module-scoped logger is consistent and appropriate.
84-85: Initialize logging before using the runtime — good placement.Calling
_setup_default_logging()in__init__ensures logs from validation and startup are captured. No concerns.
406-407: LGTM: per-worker indices make logs traceable.Passing indices to
_workerimproves observability and troubleshooting.
216-218: Confirm desired fail-fast semantics for state-enqueue failuresIt looks like the code in
python-sdk/exospherehost/runtime.py (lines 216–218)
now logs and immediately raises on any non-200 response:logger.error(f"Failed to enqueue states: {res}") raise RuntimeError(f"Failed to enqueue states: {res}")That change will cause the poller to stop and the entire runtime to tear down (via your
asyncio.gather). If the downstream API can return transient errors (e.g. occasional 500s or network blips), you may want to retry with exponential backoff rather than failing fast on the first hiccup.Please verify whether you intend to:
- Fail fast on any enqueue error (current behavior).
- Or implement a retry/backoff strategy to tolerate transient failures.
If you’d like to tolerate transients, here’s a minimal sketch:
- if response.status != 200: - logger.error(f"Failed to enqueue states: {res}") - raise RuntimeError(f"Failed to enqueue states: {res}") + if response.status != 200: + logger.error("Failed to enqueue states (status=%s): %s", response.status, res) + return {"states": []} # signal “no work” so upstream can back offAnd in your
_enqueueloop, track backoff:# pseudo-code in _enqueue backoff = min(backoff * 2, 30) if not data.get("states") else 1 await asyncio.sleep(backoff)If you do want true fail-fast semantics, this change is fine as-is.
- Introduced a suite of tests for BaseNode, covering initialization, execution, and edge cases, ensuring robust validation of node behavior. - Added integration tests for Runtime and StateManager, validating state management, error handling, and interaction with external services. - Implemented tests for package imports and structure, ensuring all expected components are accessible and correctly configured. - Enhanced test coverage for concurrency scenarios, verifying the correct handling of multiple simultaneous executions and state triggers. - Improved overall test organization and clarity, contributing to a more maintainable and reliable codebase.
- Added `# type: ignore` comments to suppress type checking warnings in various test files, ensuring compatibility with type checkers while maintaining existing functionality. - Updated test cases across multiple modules, including BaseNode, StateManager, and integration tests, to enhance clarity and prevent type-related issues during testing. - Improved overall test robustness by ensuring that type hints do not interfere with test execution.
There was a problem hiding this comment.
Actionable comments posted: 26
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
python-sdk/tests/test_base_node_comprehensive.py(1 hunks)python-sdk/tests/test_integration.py(1 hunks)python-sdk/tests/test_package_init.py(1 hunks)python-sdk/tests/test_runtime_comprehensive.py(1 hunks)python-sdk/tests/test_statemanager_comprehensive.py(1 hunks)python-sdk/tests/test_version.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
python-sdk/tests/test_version.py (1)
python-sdk/tests/test_package_init.py (1)
test_version_import(73-80)
python-sdk/tests/test_runtime_comprehensive.py (3)
python-sdk/exospherehost/runtime.py (14)
Runtime(45-424)_setup_default_logging(14-42)_get_enque_endpoint(133-137)_get_executed_endpoint(139-143)_get_errored_endpoint(145-149)_get_register_endpoint(151-155)_get_secrets_endpoint(157-161)_register(163-198)_enqueue_call(200-219)_worker(356-392)_notify_executed(240-259)_notify_errored(261-280)_get_secrets(282-297)start(410-424)python-sdk/exospherehost/node/BaseNode.py (1)
BaseNode(6-100)python-sdk/tests/test_integration.py (13)
create_mock_aiohttp_session(9-40)Inputs(44-46)Inputs(64-65)Inputs(79-80)Outputs(48-50)Outputs(67-68)Outputs(82-83)Secrets(52-54)Secrets(70-71)Secrets(85-86)execute(56-60)execute(73-75)execute(88-91)
python-sdk/tests/test_package_init.py (3)
python-sdk/exospherehost/runtime.py (1)
Runtime(45-424)python-sdk/exospherehost/node/BaseNode.py (1)
BaseNode(6-100)python-sdk/exospherehost/statemanager.py (2)
StateManager(42-227)TriggerState(10-39)
python-sdk/tests/test_base_node_comprehensive.py (1)
python-sdk/exospherehost/node/BaseNode.py (2)
BaseNode(6-100)_execute(70-83)
python-sdk/tests/test_statemanager_comprehensive.py (1)
python-sdk/exospherehost/statemanager.py (8)
StateManager(42-227)TriggerState(10-39)_get_trigger_state_endpoint(61-62)_get_upsert_graph_endpoint(64-65)_get_get_graph_endpoint(67-68)trigger(70-133)get_graph(135-168)upsert_graph(170-227)
python-sdk/tests/test_integration.py (4)
python-sdk/exospherehost/runtime.py (4)
Runtime(45-424)_register(163-198)_worker(356-392)_enqueue_call(200-219)python-sdk/exospherehost/node/BaseNode.py (1)
BaseNode(6-100)python-sdk/exospherehost/statemanager.py (4)
StateManager(42-227)TriggerState(10-39)upsert_graph(170-227)trigger(70-133)python-sdk/tests/test_runtime_comprehensive.py (17)
create_mock_aiohttp_session(11-42)Inputs(46-47)Inputs(60-61)Inputs(75-76)Inputs(91-92)Outputs(49-50)Outputs(63-64)Outputs(78-79)Outputs(94-95)Secrets(52-53)Secrets(66-67)Secrets(81-82)Secrets(97-98)execute(55-56)execute(69-71)execute(84-87)execute(100-101)
🪛 GitHub Actions: Ruff check on changed files only
python-sdk/tests/test_version.py
[error] 1-1: F401: pytest imported but unused.
python-sdk/tests/test_runtime_comprehensive.py
[error] 3-3: F401: os imported but unused.
[error] 568-568: F841: Local variable runtime assigned to but never used.
python-sdk/tests/test_package_init.py
[error] 103-103: F401: sys imported but unused.
[error] 110-110: F401: exospherehost imported but unused.
python-sdk/tests/test_base_node_comprehensive.py
[error] 3-3: F401: unittest.mock.patch imported but unused.
[error] 3-3: F401: unittest.mock.MagicMock imported but unused.
python-sdk/tests/test_statemanager_comprehensive.py
[error] 3-3: F401: os imported but unused.
python-sdk/tests/test_integration.py
[error] 3-3: F401: os imported but unused.
🔇 Additional comments (3)
python-sdk/tests/test_base_node_comprehensive.py (1)
1-99: General: comprehensive and well-structured BaseNode tests.Good coverage across happy paths, list/none outputs, error paths, and edge cases.
python-sdk/tests/test_runtime_comprehensive.py (2)
125-184: LGTM! Comprehensive initialization and validation tests.The
TestRuntimeInitializationclass provides thorough coverage of runtime initialization scenarios, including parameter validation, environment variable configuration, and error cases. The tests are well-structured and cover all the validation rules.
186-216: LGTM! Well-structured endpoint construction tests.The
TestRuntimeEndpointConstructiontests comprehensively verify that all endpoint URLs are correctly constructed with the proper namespace, version, and path components.
- Included `pytest-asyncio` in both `pyproject.toml` and `uv.lock` to support asynchronous testing. - Updated the version specification for `pytest-asyncio` to `>=0.24.0` to ensure compatibility with existing test frameworks.
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (23)
python-sdk/tests/test_version.py (4)
20-26: Avoid brittle identity check for strings; use equality only.Object identity for strings is an implementation detail and can be brittle. Equality is sufficient.
def test_version_consistency(): """Test that version is consistent across imports.""" from exospherehost._version import version as version1 from exospherehost._version import version as version2 assert version1 == version2 - assert version1 is version2
10-17: Optionally validate PEP 440 compliance instead of just non-empty.Strengthen this test by asserting the version string parses under packaging.version.Version.
def test_version_format(): """Test that version follows a reasonable format.""" - # Version should be a string that could be a semantic version - assert version is not None - # Should not be empty - assert version.strip() != "" - # Should not contain only whitespace - assert not version.isspace() + from packaging.version import Version + # Non-empty and parses under PEP 440 + assert isinstance(version, str) + assert version.strip() != "" + # Raises if invalid + Version(version)
29-34: Avoid redundancy with package-level version test or keep both consistent.This duplicates assertions already covered in test_package_init.py. Either remove this block or leave it as-is; if keeping it, there’s no need to also assert identity in the other test.
34-34: Nit: ensure trailing newline at EOF.Many tools expect a trailing newline; add one if missing.
python-sdk/tests/test_statemanager_comprehensive.py (2)
7-39: DRY the aiohttp session mock helper across tests.create_mock_aiohttp_session is duplicated in multiple modules. Extract it to tests/conftest.py as a fixture (e.g., mock_aiohttp_session) and reuse.
Example fixture (tests/conftest.py):
import pytest from unittest.mock import AsyncMock, MagicMock @pytest.fixture def mock_aiohttp_session(): mock_session = AsyncMock() mock_post_response = AsyncMock() mock_get_response = AsyncMock() mock_put_response = AsyncMock() post_ctx = AsyncMock(); post_ctx.__aenter__.return_value = mock_post_response; post_ctx.__aexit__.return_value = None get_ctx = AsyncMock(); get_ctx.__aenter__.return_value = mock_get_response; get_ctx.__aexit__.return_value = None put_ctx = AsyncMock(); put_ctx.__aenter__.return_value = mock_put_response; put_ctx.__aexit__.return_value = None mock_session.post = MagicMock(return_value=post_ctx) mock_session.get = MagicMock(return_value=get_ctx) mock_session.put = MagicMock(return_value=put_ctx) mock_session.__aenter__.return_value = mock_session mock_session.__aexit__.return_value = None return mock_session, mock_post_response, mock_get_response, mock_put_responseThen replace local calls with the fixture argument and
mock_session_class.return_value = mock_session.
170-187: Don’t use asyncio.run inside pytest tests; convert to async tests.Mixing asyncio.run with pytest’s event loop is error-prone. Use @pytest.mark.asyncio and await the coroutine.
- def test_trigger_validation_no_state_or_states(self, state_manager_config): - sm = StateManager(**state_manager_config) - - with pytest.raises(ValueError, match="Either state or states must be provided"): - asyncio.run(sm.trigger("test_graph")) + @pytest.mark.asyncio + async def test_trigger_validation_no_state_or_states(self, state_manager_config): + sm = StateManager(**state_manager_config) + with pytest.raises(ValueError, match="Either state or states must be provided"): + await sm.trigger("test_graph") - def test_trigger_validation_both_state_and_states(self, state_manager_config): + @pytest.mark.asyncio + async def test_trigger_validation_both_state_and_states(self, state_manager_config): sm = StateManager(**state_manager_config) state = TriggerState(identifier="test", inputs={"key": "value"}) with pytest.raises(ValueError, match="Only one of state or states must be provided"): - asyncio.run(sm.trigger("test_graph", state=state, states=[state])) + await sm.trigger("test_graph", state=state, states=[state]) - def test_trigger_validation_empty_states_list(self, state_manager_config): + @pytest.mark.asyncio + async def test_trigger_validation_empty_states_list(self, state_manager_config): sm = StateManager(**state_manager_config) with pytest.raises(ValueError, match="States must be a non-empty list"): - asyncio.run(sm.trigger("test_graph", states=[])) + await sm.trigger("test_graph", states=[])python-sdk/tests/test_integration.py (6)
8-40: Consolidate duplicated aiohttp mocking helper into a shared fixture.Same helper exists in multiple files. Move it to tests/conftest.py and reuse to reduce maintenance.
131-156: Patch the symbol where it’s used: exospherehost.runtime.ClientSession (not aiohttp.ClientSession).Runtime imports ClientSession into its module scope; patch that symbol to avoid real network I/O.
- async def test_runtime_worker_with_state_manager(self, mock_env_vars): - with patch('aiohttp.ClientSession') as mock_session: + async def test_runtime_worker_with_state_manager(self, mock_env_vars): + with patch('exospherehost.runtime.ClientSession') as mock_session: # Mock all HTTP responses mock_register_response = AsyncMock() mock_register_response.status = 200 mock_register_response.json = AsyncMock(return_value={"status": "registered"}) @@ - mock_session.return_value.__aenter__.return_value.post.return_value.__aenter__.return_value = mock_enqueue_response - mock_session.return_value.__aenter__.return_value.get.return_value.__aenter__.return_value = mock_secrets_response + # Worker path uses GET (secrets) and POST (notify executed) + mock_session.return_value.__aenter__.return_value.get.return_value.__aenter__.return_value = mock_secrets_response + mock_session.return_value.__aenter__.return_value.post.return_value.__aenter__.return_value = mock_executed_response
278-309: Same patching issue here; target runtime’s imported ClientSession.Ensure tests don’t accidentally hit the network.
- with patch('aiohttp.ClientSession') as mock_session: + with patch('exospherehost.runtime.ClientSession') as mock_session:
264-266: Tests rely on a runtime logging bug: _node_mapping keyed by state_id. Remove workaround and fix runtime logging._runtime.node_mapping is keyed by node class name, not state_id. Populating by state_id in tests masks a bug in Runtime._notify_executed/_notify_errored where logs index by state_id.
- # Add state to node mapping - runtime._node_mapping[state["state_id"]] = MultiOutputNodeProposed fix in runtime (outside this file) to log without indexing by state_id:
--- a/python-sdk/exospherehost/runtime.py +++ b/python-sdk/exospherehost/runtime.py @@ - if response.status != 200: - logger.error(f"Failed to notify executed state {state_id}: {res}") - - logger.info(f"Notified executed state {state_id} with outputs: {outputs} for node {self._node_mapping[state_id].__name__}") + if response.status != 200: + logger.error(f"Failed to notify executed state {state_id}: {res}") + logger.info(f"Notified executed state {state_id} with outputs: {outputs}") @@ - if response.status != 200: - logger.error(f"Failed to notify errored state {state_id}: {res}") - - logger.info(f"Notified errored state {state_id} with error: {error} for node {self._node_mapping[state_id].__name__}") + if response.status != 200: + logger.error(f"Failed to notify errored state {state_id}: {res}") + logger.info(f"Notified errored state {state_id} with error: {error}")I can open a follow-up PR to adjust runtime and then simplify these tests.
379-381: Same mapping-by-state_id issue; remove the workaround.- # Add state to node mapping - runtime._node_mapping[state["state_id"]] = IntegrationTestNode
503-506: Same mapping-by-state_id issue across multiple states; remove this block.- # Add states to node mapping - for state in states: - runtime._node_mapping[state["state_id"]] = IntegrationTestNodepython-sdk/tests/test_package_init.py (2)
23-25: Don’t over-constrain all; prefer subset check.As the public surface evolves, forbidding additional exports is brittle. Assert that expected symbols are present.
- # Check that __all__ doesn't contain unexpected exports - for export in __all__: - assert export in expected_exports, f"Unexpected export: {export}" + # Optional: avoid forbidding additional public exports + assert set(expected_exports).issubset(set(__all__)), "__all__ must include the expected exports"
44-46: Loosen the abstract instantiation error assertion.Exact TypeError messages vary across Python versions. Assert the exception type only.
- with pytest.raises(TypeError, match="Can't instantiate abstract class"): + with pytest.raises(TypeError): BaseNode() # type: ignorepython-sdk/tests/test_runtime_comprehensive.py (8)
10-41: Extract duplicated aiohttp session mock helper to a shared test utility.This helper is duplicated (see prior review). Centralize it (e.g., tests/utils/mock_http.py) and import where needed to reduce drift.
+# tests/utils/mock_http.py +from unittest.mock import AsyncMock, MagicMock + +def create_mock_aiohttp_session(): + """Helper function to create a properly mocked aiohttp session.""" + mock_session = AsyncMock() + mock_post_response = AsyncMock() + mock_get_response = AsyncMock() + mock_put_response = AsyncMock() + + mock_post_context = AsyncMock() + mock_post_context.__aenter__.return_value = mock_post_response + mock_post_context.__aexit__.return_value = None + + mock_get_context = AsyncMock() + mock_get_context.__aenter__.return_value = mock_get_response + mock_get_context.__aexit__.return_value = None + + mock_put_context = AsyncMock() + mock_put_context.__aenter__.return_value = mock_put_response + mock_put_context.__aexit__.return_value = None + + mock_session.post = MagicMock(return_value=mock_post_context) + mock_session.get = MagicMock(return_value=mock_get_context) + mock_session.put = MagicMock(return_value=mock_put_context) + + mock_session.__aenter__.return_value = mock_session + mock_session.__aexit__.return_value = None + return mock_session, mock_post_response, mock_get_response, mock_put_responseAnd update imports in tests:
-from ... import create_mock_aiohttp_session +from tests.utils.mock_http import create_mock_aiohttp_session
59-71: Typecountas int to avoid runtime casting and remove type-ignores.Use proper types in the Inputs model and adjust the test payload accordingly.
class MockTestNodeWithListOutput(BaseNode): class Inputs(BaseModel): - count: str + count: int @@ async def execute(self): - count = int(self.inputs.count) # type: ignore + count = self.inputs.count return [self.Outputs(numbers=str(i)) for i in range(count)]And in the test state (line 338):
- "inputs": {"count": "3"} + "inputs": {"count": 3}
74-87: Use a boolean flag forshould_fail.This is a boolean control; typing it as
boolclarifies intent and removes string comparisons/type-ignores.class MockTestNodeWithError(BaseNode): class Inputs(BaseModel): - should_fail: str + should_fail: bool @@ async def execute(self): - if self.inputs.should_fail == "true": # type: ignore + if self.inputs.should_fail: raise ValueError("Test error") return self.Outputs(result="success")Update test payloads at lines 405-406:
- "inputs": {"should_fail": "true"} + "inputs": {"should_fail": True}
300-311: Replace sleep with deterministic queue join to avoid flakiness.Relying on
asyncio.sleepraces tests. Wait for the queue to drain withjoin()then cancel the worker.- # Wait a bit for processing - await asyncio.sleep(0.1) - - # Cancel the worker - worker_task.cancel() + # Wait deterministically for processing + await runtime._state_queue.join() + # Cancel the worker once the queue is empty + worker_task.cancel() try: await worker_task except asyncio.CancelledError: pass
343-349: Same flakiness here — preferjoin()oversleep().- worker_task = asyncio.create_task(runtime._worker(1)) - await asyncio.sleep(0.1) - worker_task.cancel() + worker_task = asyncio.create_task(runtime._worker(1)) + await runtime._state_queue.join() + worker_task.cancel() try: await worker_task except asyncio.CancelledError: pass
378-384: Same pattern — usejoin()for reliable completion.- worker_task = asyncio.create_task(runtime._worker(1)) - await asyncio.sleep(0.1) - worker_task.cancel() + worker_task = asyncio.create_task(runtime._worker(1)) + await runtime._state_queue.join() + worker_task.cancel() try: await worker_task except asyncio.CancelledError: pass
410-416: Same fix — eliminatesleep()in favor of queue draining.- worker_task = asyncio.create_task(runtime._worker(1)) - await asyncio.sleep(0.1) - worker_task.cancel() + worker_task = asyncio.create_task(runtime._worker(1)) + await runtime._state_queue.join() + worker_task.cancel() try: await worker_task except asyncio.CancelledError: pass
574-607: Isolate global logging state with a fixture.Current tests mutate the root logger without restoring it, potentially leaking state to other tests. Use a fixture to snapshot and restore handlers/level.
+import pytest +import logging + +@pytest.fixture +def clean_logger(): + logger = logging.getLogger() + original_handlers = logger.handlers.copy() + original_level = logger.level + try: + yield logger + finally: + logger.handlers = original_handlers + logger.setLevel(original_level)Use in tests:
- def test_setup_default_logging_with_existing_handlers(self): + def test_setup_default_logging_with_existing_handlers(self, clean_logger): - logger = logging.getLogger() + logger = clean_logger @@ - def test_setup_default_logging_with_env_disable(self, monkeypatch): + def test_setup_default_logging_with_env_disable(self, monkeypatch, clean_logger): @@ - def test_setup_default_logging_with_custom_level(self, monkeypatch): + def test_setup_default_logging_with_custom_level(self, monkeypatch, clean_logger): - logger = logging.getLogger() + logger = clean_loggerpython-sdk/tests/test_base_node_comprehensive.py (1)
306-316: Don’t reuse the same node instance across concurrent executions.
BaseNodestoresinputsandsecretson the instance; concurrent calls on a shared instance race. Create a fresh instance per task.- node = ValidNode() inputs = ValidNode.Inputs(name="test", count="1") secrets = ValidNode.Secrets(api_key="key", token="token") # Run multiple concurrent executions tasks = [ - node._execute(inputs, secrets) for _ in range(5) # type: ignore + ValidNode()._execute(inputs, secrets) for _ in range(5) # type: ignore ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
python-sdk/exospherehost/_version.py(1 hunks)python-sdk/tests/test_base_node_comprehensive.py(1 hunks)python-sdk/tests/test_integration.py(1 hunks)python-sdk/tests/test_package_init.py(1 hunks)python-sdk/tests/test_runtime_comprehensive.py(1 hunks)python-sdk/tests/test_statemanager_comprehensive.py(1 hunks)python-sdk/tests/test_version.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
python-sdk/tests/test_statemanager_comprehensive.py (1)
python-sdk/exospherehost/statemanager.py (8)
StateManager(42-227)TriggerState(10-39)_get_trigger_state_endpoint(61-62)_get_upsert_graph_endpoint(64-65)_get_get_graph_endpoint(67-68)trigger(70-133)get_graph(135-168)upsert_graph(170-227)
python-sdk/tests/test_package_init.py (3)
python-sdk/exospherehost/runtime.py (1)
Runtime(45-424)python-sdk/exospherehost/node/BaseNode.py (1)
BaseNode(6-100)python-sdk/exospherehost/statemanager.py (2)
StateManager(42-227)TriggerState(10-39)
python-sdk/tests/test_version.py (1)
python-sdk/tests/test_package_init.py (1)
test_version_import(73-80)
python-sdk/tests/test_runtime_comprehensive.py (2)
python-sdk/exospherehost/runtime.py (13)
Runtime(45-424)_setup_default_logging(14-42)_get_enque_endpoint(133-137)_get_executed_endpoint(139-143)_get_errored_endpoint(145-149)_get_register_endpoint(151-155)_get_secrets_endpoint(157-161)_register(163-198)_enqueue_call(200-219)_worker(356-392)_notify_executed(240-259)_notify_errored(261-280)_get_secrets(282-297)python-sdk/exospherehost/node/BaseNode.py (1)
BaseNode(6-100)
python-sdk/tests/test_integration.py (3)
python-sdk/exospherehost/runtime.py (4)
Runtime(45-424)_register(163-198)_worker(356-392)_enqueue_call(200-219)python-sdk/exospherehost/node/BaseNode.py (1)
BaseNode(6-100)python-sdk/exospherehost/statemanager.py (4)
StateManager(42-227)TriggerState(10-39)upsert_graph(170-227)trigger(70-133)
python-sdk/tests/test_base_node_comprehensive.py (1)
python-sdk/exospherehost/node/BaseNode.py (2)
BaseNode(6-100)_execute(70-83)
🪛 GitHub Actions: Python SDK Tests
python-sdk/tests/test_base_node_comprehensive.py
[warning] 123-123: PytestUnknownMarkWarning: Unknown pytest.mark.asyncio - is this a typo? You can register custom marks to avoid this warning.
[warning] 137-137: PytestUnknownMarkWarning: Unknown pytest.mark.asyncio - is this a typo? You can register custom marks to avoid this warning.
🔇 Additional comments (1)
python-sdk/exospherehost/_version.py (1)
1-1: Version bump looks good.The pre-release bump to 0.0.7b9 is PEP 440–compliant and aligns with the rest of the tests asserting VERSION consistency.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
python-sdk/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
python-sdk/pyproject.toml(1 hunks)
🔇 Additional comments (1)
python-sdk/pyproject.toml (1)
36-36: LGTM: Adding pytest-asyncio aligns with new asyncio-based testsThe dependency is appropriate given the async tests introduced in this PR and Python 3.12/3.13 support.
- Updated the integration tests in `test_integration.py` to use `exospherehost.runtime.ClientSession` for mocking HTTP responses, enhancing clarity and maintainability. - Simplified the mocking of HTTP responses by consolidating the setup process, ensuring consistent behavior across tests. - Fixed a bug in the runtime by adding `state_id` to the node mapping, improving the functionality of the state management system.
- Updated logging format in `runtime.py` to include the logger's name for better context in log messages. - Changed info log statements to debug level for state manager URI and API key fallback, reducing log verbosity. - Added pytest configuration options in `pyproject.toml` to specify asyncio mode and test paths, improving test execution setup.
1 participant
No description provided.