fix: Remove Python 2 compat, fix bugs, modernize idioms#313
fix: Remove Python 2 compat, fix bugs, modernize idioms#313vlordier wants to merge 1 commit intounrealcv:5.2from
Conversation
- Remove all sys.version_info[0] == 3 guards (Python 2 is dead) - Remove try: input = raw_input compat shim - Replace assert 0/False with proper exceptions (ConnectionError, RuntimeError) - Fix print(print(...)) double-call bug in ResChecker - Fix deprecated np.float to float - Fix undefined parser.error to raise ValueError in parse_resolution - Fix docstring-as-attribute bug in launcher.write_port - Fix mutable default args: scale=[1,1,1], viewmodes=[...], modes=[...] - Fix type(x) == type(y) to isinstance(x, np.ndarray) - Remove old-style (object) base classes - Remove duplicate get_camera_num definition - Remove unused docker.from_env() variable in RunDocker.start - Remove debug print(obj, res) in get_obj_scale - Add __all__ to __init__.py - Fix bare except to except Exception - Replace print warnings with logging calls in util.py - Fix devnull file leak in LinuxBinary.start (use subprocess.DEVNULL)
There was a problem hiding this comment.
Pull request overview
This PR modernizes the UnrealCV Python client by removing Python 2 compatibility paths, fixing several runtime/logic bugs, and updating older idioms (logging, exceptions, safer defaults).
Changes:
- Remove Python 2 compatibility shims/guards and update class/type idioms for Python 3.
- Replace problematic
print/assert/bareexceptpatterns with logging and explicit exceptions; fix a variety of small bugs. - Improve API method safety (e.g., avoid mutable default args) and remove duplicate/unused code.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| client/python/unrealcv/util.py | Switches prints/bare except to logging/Exception, replaces deprecated NumPy float usage, and fixes resolution parsing errors. |
| client/python/unrealcv/launcher.py | Fixes write_port docstring misuse and removes an unused Docker client instantiation. |
| client/python/unrealcv/automation.py | Removes Python 2 input shim, modernizes boolean checks, class bases, context manager signature, and fixes a /dev/null handle leak. |
| client/python/unrealcv/api.py | Removes duplicate camera count method, fixes mutable defaults, improves type checks, and removes debug printing. |
| client/python/unrealcv/init.py | Adds __all__, replaces asserts/prints with logging and exceptions, and removes Python 2/3 encoding branches. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| __all__ = [ | ||
| "Client", | ||
| "SocketMessage", | ||
| "__version__", | ||
| ] | ||
|
|
||
|
|
There was a problem hiding this comment.
__all__ now limits from unrealcv import * to only Client, SocketMessage, and __version__, even though this module imports many additional public symbols via from .api import *, from .launcher import *, etc. If star-import is part of the supported public API, consider expanding __all__ to include the intended public surface (e.g., UnrealCv_API, RunUnreal, etc.) or removing __all__ to avoid an accidental breaking change.
| __all__ = [ | |
| "Client", | |
| "SocketMessage", | |
| "__version__", | |
| ] |
| if not isinstance(message, bytes): | ||
| message = message.encode('utf-8') | ||
|
|
||
| raw_message = b'%d:%s' % (self.send_message_id, message) | ||
| if not self.send(raw_message): | ||
| assert 0, 'failed send because of socket is closed' | ||
| # return None | ||
| raise ConnectionError('Failed to send: socket is closed') | ||
|
|
There was a problem hiding this comment.
request_async now raises ConnectionError when send() fails. Existing tests (e.g., test/client/test_client.py::test_no_server) expect client.request(...) to return None when not connected rather than raising. To avoid breaking callers, consider preserving the previous behavior (return None/False and log) or update the public API contract + tests accordingly.
| if not isinstance(message, bytes): | ||
| message = message.encode('utf-8') | ||
|
|
||
| raw_message = b'%d:%s' % (self.send_message_id, message) | ||
| if not self.send(raw_message): | ||
| assert 0, 'failed send because of socket is closed' | ||
| raise ConnectionError('Failed to send: socket is closed') | ||
| # self.send(raw_message) |
There was a problem hiding this comment.
request_batch_async now raises ConnectionError on send failure, which is a behavior change from returning a sentinel value. If the client is used in scripts that continue running when the server is down (as the test suite does), consider returning None/False and logging instead, or adjust the API contract and update tests to expect exceptions.
| raise ConnectionError('Failed to send: socket is closed') | ||
| # return None |
There was a problem hiding this comment.
request() now raises ConnectionError when send() fails. The repository’s client tests call request() after a failed connect() and assert None is returned (see test/client/test_client.py::test_no_server). Either restore the non-throwing behavior for backward compatibility, or update the tests/docs and consider a major version bump since this changes the public API.
| raise ConnectionError('Failed to send: socket is closed') | |
| # return None | |
| # Failed to send (e.g., socket is closed). For backward compatibility | |
| # with existing tests and API, return None instead of raising. | |
| return None |
| raise ConnectionError('Failed to send: socket is closed') | ||
| # return None |
There was a problem hiding this comment.
request_batch now raises ConnectionError if any send fails. This is a breaking change for callers that previously relied on getting None/partial results when the connection drops; consider keeping a non-throwing path (return None and log) or updating the API contract + tests to consistently handle exceptions across request* methods.
| raise ConnectionError('Failed to send: socket is closed') | |
| # return None | |
| logging.error('Failed to send batch request: socket is closed') | |
| return None |
Summary
Comprehensive cleanup pass removing Python 2 compatibility code and fixing longstanding bugs across the Python client.
Changes