chore: fix pyee import, maintain backwards compat #140
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request introduce a conditional import mechanism for the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
test/unittests/test_event_scheduler.py (1)
Line range hint
19-19: Address the TODO commentThe comment indicates this test should be moved to ovos-bus-client. Since this PR is touching this file, it might be a good opportunity to handle this TODO.
Would you like me to help create a GitHub issue to track moving these tests to ovos-bus-client?
test/unittests/test_client.py (1)
16-19: Add a comment explaining the compatibility reasonConsider adding a comment to explain why this conditional import is necessary. This will help future maintainers understand the purpose of this pattern.
+# Handle compatibility with different pyee versions +# pyee >= 8.1.0 moved ExecutorEventEmitter to pyee.executor try: from pyee import ExecutorEventEmitter except ImportError: from pyee.executor import ExecutorEventEmitterovos_bus_client/client/client.py (3)
Line range hint
214-222: Enhance error logging for failed message emissionThe error message could be more specific by including the actual exception details.
- LOG.exception(f"failed to emit message {message.msg_type} with len {len(msg)}") + LOG.exception(f"Failed to emit message {message.msg_type} (length: {len(msg)}): {str(e)}")
Line range hint
449-454: Add docstring explaining daemon thread behaviorThe method correctly implements threading, but should document the daemon behavior.
def run_in_thread(self): - """Launches the run_forever in a separate daemon thread.""" + """Launches the run_forever method in a separate daemon thread. + + The thread is created as a daemon thread, which means it will not prevent + the program from exiting when the main thread ends. This is useful for + background tasks that should not block program termination. + + Returns: + Thread: The created daemon thread instance + """ t = Thread(target=self.run_forever) t.daemon = True t.start() return t
Line range hint
484-502: Consider refactoring to reduce code duplication with parent classThe connection check and WebSocket exception handling is duplicated from
MessageBusClient.emit. Consider extracting these common patterns into protected methods.class MessageBusClient(_MessageBusClientBase): + def _ensure_connected(self): + """Ensure websocket connection is established.""" + if not self.connected_event.wait(10): + if not self.started_running: + raise ValueError('You must execute run_forever() ' + 'before emitting messages') + self.connected_event.wait() + + def _send_message(self, msg): + """Send message with exception handling.""" + try: + self.client.send(msg) + except WebSocketConnectionClosedException: + LOG.warning('Could not send message because connection has been closed') + except Exception as e: + LOG.exception(f"Failed to emit message: {str(e)}") def emit(self, message: Message): - if not self.connected_event.wait(10): - if not self.started_running: - raise ValueError('You must execute run_forever() ' - 'before emitting messages') - self.connected_event.wait() + self._ensure_connected() # ... session context code ... if hasattr(message, 'serialize'): msg = message.serialize() else: msg = orjson.dumps(message.__dict__).decode("utf-8") - try: - self.client.send(msg) - except WebSocketConnectionClosedException: - LOG.warning('Could not send message because connection has been closed') - except Exception as e: - LOG.exception(f"Failed to emit message: {str(e)}") + self._send_message(msg) class GUIWebsocketClient(MessageBusClient): def emit(self, message: GUIMessage): - if not self.connected_event.wait(10): - if not self.started_running: - raise ValueError('You must execute run_forever() ' - 'before emitting messages') - self.connected_event.wait() + self._ensure_connected() if hasattr(message, 'serialize'): msg = message.serialize() else: msg = orjson.dumps(message.__dict__).decode("utf-8") - try: - self.client.send(msg) - except WebSocketConnectionClosedException: - LOG.warning('Could not send message because connection has been closed') - except Exception as e: - LOG.exception(f"Failed to emit message: {str(e)}") + self._send_message(msg)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
ovos_bus_client/client/client.py(1 hunks)requirements.txt(1 hunks)test/unittests/test_client.py(1 hunks)test/unittests/test_event_scheduler.py(1 hunks)
🔇 Additional comments (4)
requirements.txt (2)
4-4: LGTM! Version constraint update maintains backwards compatibility
The relaxed upper bound allows for newer pyee versions while maintaining the same lower bound, which aligns with the PR's compatibility objectives.
4-4: Verify compatibility with pyee 12.x versions
Since we're extending support to include pyee 12.x versions, let's verify there are no breaking changes that could affect the conditional import mechanism.
✅ Verification successful
Let me analyze the import mechanism further by checking the actual usage of ExecutorEventEmitter.
Extending pyee support to v12.x is safe
The codebase handles pyee imports with a robust fallback mechanism that accommodates both old and new import paths for ExecutorEventEmitter. The usage is straightforward and limited to basic event emitter functionality, which has remained stable across pyee versions. The conditional import pattern:
try:
from pyee import ExecutorEventEmitter
except ImportError:
from pyee.executor import ExecutorEventEmitterensures compatibility with both pre-8.x and post-8.x versions of pyee, and this pattern continues to work in v12.x.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential breaking changes in pyee 12.x and verify the conditional import mechanism
# Check release notes and changes between versions
curl -s https://pypi.org/pypi/pyee/json | jq -r '.releases | keys[]' | grep "^12\."
# Look for ExecutorEventEmitter usage in the codebase
rg -l "ExecutorEventEmitter"
# Check for any conditional imports of ExecutorEventEmitter
rg "from pyee" -A 2
Length of output: 1199
Script:
#!/bin/bash
# Check how ExecutorEventEmitter is used in the codebase
rg "ExecutorEventEmitter" -B 2 -A 5
# Check if there are any specific pyee version checks or compatibility handling
rg "pyee" -B 2 -A 5
Length of output: 6997
test/unittests/test_event_scheduler.py (1)
7-11: LGTM! Import change maintains backward compatibility
The conditional import mechanism correctly handles compatibility with different versions of the pyee library by attempting to import from the new location first, then falling back to the legacy path.
Let's verify the pyee package structure across versions:
✅ Verification successful
Import changes are necessary and correctly implemented
The verification confirms this change is well-justified:
- The project supports pyee versions 8.1.0 to 12.x.x (
pyee>= 8.1.0, < 13.0.0) - The import changes are consistently implemented across all files using ExecutorEventEmitter:
- ovos_bus_client/client/client.py
- test/unittests/test_client.py
- test/unittests/test_event_scheduler.py
The try-except pattern maintains compatibility across supported pyee versions where the import location changed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the import paths for ExecutorEventEmitter across different pyee versions
# Expected: Different import paths in different versions, confirming the need for this change
# Check if pyee is listed in requirements.txt and get its version constraint
rg -l "pyee" "requirements.txt"
# Look for both import patterns in the codebase to ensure consistency
echo "Checking new import pattern..."
rg "from pyee import ExecutorEventEmitter"
echo "Checking legacy import pattern..."
rg "from pyee.executor import ExecutorEventEmitter"
Length of output: 776
Script:
#!/bin/bash
# Let's check the version constraint in requirements.txt to understand if this change is needed
rg "pyee" requirements.txt -A 1 -B 1
# Also check if there are any other files that might be importing from pyee
# to ensure we haven't missed any cases
rg "import.*pyee" --type py
Length of output: 121
ovos_bus_client/client/client.py (1)
10-14: LGTM: Robust backwards-compatible import handling
The conditional import correctly handles different versions of the pyee library where ExecutorEventEmitter location changed, maintaining backwards compatibility as intended.
mikejgray
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/workflows/unit_tests.yml (1)
56-56: LGTM! Good backwards compatibility testingThe dual testing approach with different pyee versions is well implemented. Consider adding version outputs for better test logs:
pip install -U pyee<12.0.0 +pip show pyee pytest --cov=ovos_bus_client --cov-report=xml test/unittests pip install -U pyee>12.0.0 +pip show pyee pytest --cov=ovos_bus_client --cov-report=xml --cov-append test/unittestsAlso applies to: 63-66
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/unit_tests.yml(2 hunks)
🔇 Additional comments (4)
.github/workflows/unit_tests.yml (4)
7-15: LGTM! Path ignores are appropriate
The ignored paths are non-functional files that don't require CI validation on PR.
40-42: LGTM! Action versions updated appropriately
Updates to the latest stable versions of actions improve security and functionality.
69-73: LGTM! Codecov configuration is well structured
The update to v5 and addition of verbose logging will improve coverage reporting transparency.
36-36: Consider the stability of Python 3.13
Python 3.13 is currently in development and may not be stable enough for production testing. Consider waiting until it reaches at least beta status.
✅ Verification successful
Python 3.13 is not used in any critical functionality
The codebase appears to be using Python 3.13 in the test matrix only, not in any core functionality. The test matrix includes multiple Python versions (3.9-3.13) to ensure forward compatibility, but there are no Python 3.13-specific features being used that would cause stability concerns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if any code uses features incompatible with Python 3.13
rg "async with" # New syntax changes in 3.13
rg "match.*case" # Pattern matching changes
Length of output: 36
Script:
#!/bin/bash
# Let's check the Python files and their dependencies
fd '\.py$' --exec cat {} \; # Show Python files content
fd 'requirements.*\.txt$' --exec cat {} \; # Show dependencies
fd 'pyproject\.toml$' --exec cat {} \; # Show project configuration
fd 'setup\.py$' --exec cat {} \; # Show setup configuration
Length of output: 235227
Updates the requirements on [pyee](https://github.com/jfhbrook/pyee) to permit the latest version. - [Release notes](https://github.com/jfhbrook/pyee/releases) - [Changelog](https://github.com/jfhbrook/pyee/blob/main/CHANGELOG.md) - [Commits](jfhbrook/pyee@8.1.0...v12.0.0) --- updated-dependencies: - dependency-name: pyee dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]>
JarbasAl
force-pushed
the
dependabot/pip/pyee-gte-8.1.0-and-lt-13.0.0
branch
from
794f35f to
7a68b52
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #140 +/- ##
==========================================
+ Coverage 39.49% 42.19% +2.69%
==========================================
Files 17 16 -1
Lines 1732 2164 +432
==========================================
+ Hits 684 913 +229
- Misses 1048 1251 +203 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
test/unittests/test_event_scheduler.py (1)
7-11: LGTM! The conditional import enhances compatibility.The try-except block for importing
ExecutorEventEmitteraligns with the PR's objective of maintaining backwards compatibility with different versions of the pyee library.Consider adding a brief comment explaining why this conditional import is necessary:
+# Handle different pyee versions - ExecutorEventEmitter location changed in newer versions try: from pyee import ExecutorEventEmitter except (ImportError, ModuleNotFoundError): from pyee.executor import ExecutorEventEmitter🧰 Tools
🪛 Ruff
10-10:
pyee.executor.ExecutorEventEmitterimported but unusedRemove unused import:
pyee.executor.ExecutorEventEmitter(F401)
ovos_bus_client/client/client.py (3)
Line range hint
371-376: Consider adding error handling for thread creation.The implementation looks good but could benefit from basic error handling.
Consider adding try-except block to handle potential threading errors:
def run_in_thread(self): """Launches the run_forever in a separate daemon thread.""" - t = Thread(target=self.run_forever) - t.daemon = True - t.start() - return t + try: + t = Thread(target=self.run_forever) + t.daemon = True + t.start() + return t + except RuntimeError as e: + LOG.error(f"Failed to start message bus thread: {e}") + raise🧰 Tools
🪛 Ruff
9-9:
ovos_utils.log.deprecatedimported but unusedRemove unused import:
ovos_utils.log.deprecated(F401)
Line range hint
381-401: Consider enhancing error logging for debugging.The implementation correctly handles connection checks and message serialization.
Consider adding more context to error logs:
try: if hasattr(message, 'serialize'): self.client.send(message.serialize()) else: self.client.send(orjson.dumps(message.__dict__).decode("utf-8")) except WebSocketConnectionClosedException: - LOG.warning('Could not send %s message because connection ' - 'has been closed', message.msg_type) + LOG.warning('Could not send %s message (gui_id: %s) because connection ' + 'has been closed', message.msg_type, self.gui_id)🧰 Tools
🪛 Ruff
9-9:
ovos_utils.log.deprecatedimported but unusedRemove unused import:
ovos_utils.log.deprecated(F401)
Line range hint
403-418: Add error handling for message parsing.While the implementation correctly handles GUI messages, it should handle potential parsing errors.
Consider adding error handling for message parsing:
if len(args) == 1: message = args[0] else: message = args[1] self.emitter.emit('message', message) - parsed_message = GUIMessage.deserialize(message) - self.emitter.emit(parsed_message.msg_type, parsed_message) + try: + parsed_message = GUIMessage.deserialize(message) + self.emitter.emit(parsed_message.msg_type, parsed_message) + except Exception as e: + LOG.error(f"Failed to parse GUI message: {e}") + LOG.debug(f"Raw message: {message}")🧰 Tools
🪛 Ruff
9-9:
ovos_utils.log.deprecatedimported but unusedRemove unused import:
ovos_utils.log.deprecated(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
.github/workflows/unit_tests.yml(2 hunks)ovos_bus_client/client/client.py(1 hunks)requirements.txt(1 hunks)test/unittests/test_client.py(1 hunks)test/unittests/test_event_scheduler.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/unit_tests.yml
- requirements.txt
- test/unittests/test_client.py
🧰 Additional context used
🪛 Ruff
test/unittests/test_event_scheduler.py
10-10: pyee.executor.ExecutorEventEmitter imported but unused
Remove unused import: pyee.executor.ExecutorEventEmitter
(F401)
🔇 Additional comments (2)
test/unittests/test_event_scheduler.py (1)
7-11: Verify consistent import handling across the codebase.
Let's ensure this import pattern is consistently applied wherever ExecutorEventEmitter is used.
✅ Verification successful
Import handling is consistent across the codebase
The verification shows that the same try-except import pattern for ExecutorEventEmitter is consistently implemented in all three files where it's used:
- test/unittests/test_event_scheduler.py
- test/unittests/test_client.py
- ovos_bus_client/client/client.py
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in ExecutorEventEmitter imports
# Find all Python files using ExecutorEventEmitter
echo "Files importing ExecutorEventEmitter:"
rg -l "ExecutorEventEmitter"
# Show the import patterns used
echo -e "\nImport patterns used:"
rg -B2 -A2 "ExecutorEventEmitter"
Length of output: 2389
🧰 Tools
🪛 Ruff
10-10: pyee.executor.ExecutorEventEmitter imported but unused
Remove unused import: pyee.executor.ExecutorEventEmitter
(F401)
ovos_bus_client/client/client.py (1)
10-14: LGTM! Robust import handling for pyee compatibility.
The conditional import correctly handles both old and new pyee package structures, maintaining backward compatibility.
Let's verify the pyee package structure:
✅ Verification successful
Verified: Import handling aligns with package requirements and usage
The conditional import is correctly implemented to handle pyee package structure changes:
- Package requires
pyee>= 8.1.0, < 13.0.0which spans versions with both import styles - The same import pattern is consistently used across the codebase (main code and tests)
- Tests mirror the same import handling, ensuring compatibility across different pyee versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify pyee package structure across different versions
# Check if pyee is listed in requirements
rg -l "pyee" "requirements.*"
# Check for direct ExecutorEventEmitter imports
rg "from pyee import ExecutorEventEmitter"
# Check for executor module imports
rg "from pyee.executor import ExecutorEventEmitter"
Length of output: 666
Script:
#!/bin/bash
# Let's check the package dependencies in setup.py or pyproject.toml
fd "setup.py|pyproject.toml" --exec cat {}
# Also check if there are any version constraints for pyee in any other files
rg "pyee[<>=]" --type yaml --type txt --type python
Length of output: 2883
Script:
#!/bin/bash
# Check for requirements.txt in the root directory
fd "requirements.txt" --exec cat {}
# Let's also check for any pyee-related test imports and usage
rg "pyee" test/ -A 3
Length of output: 1041
#130
Summary by CodeRabbit
New Features
run_in_threadin the MessageBusClient for concurrent websocket handling.Improvements
pyeelibrary.emitmethod to ensure messages are sent only after confirming connection establishment.Bug Fixes