[refactor] Use builtin TimeoutError instead of our own#3419
Open
pieleric wants to merge 1 commit intodelmic:masterfrom
Open
[refactor] Use builtin TimeoutError instead of our own#3419pieleric wants to merge 1 commit intodelmic:masterfrom
pieleric wants to merge 1 commit intodelmic:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the codebase to stop using Odemis’ custom TimeoutError (originally needed for Python 2 compatibility) and instead rely on Python’s builtin TimeoutError, reducing ambiguity between multiple timeout exception types.
Changes:
- Removes the custom
TimeoutErrordefinition fromodemis.util. - Updates imports/usages across drivers, acquisition code, plugins, scripts, and tests to use the builtin
TimeoutError. - Cleans up now-unneeded
TimeoutErrorimports fromodemis.util.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/odemis/util/test/util_test.py | Stops importing TimeoutError from odemis.util; tests now use builtin TimeoutError. |
| src/odemis/util/init.py | Removes the custom TimeoutError class from the util package. |
| src/odemis/driver/tmcm.py | Drops TimeoutError import from odemis.util to rely on builtin. |
| src/odemis/driver/test/hamamatsurx_test.py | Updates assertions to expect builtin TimeoutError. |
| src/odemis/driver/tescan.py | Removes TimeoutError import from odemis.util. |
| src/odemis/driver/pigcs.py | Drops TimeoutError import from odemis.util. |
| src/odemis/driver/picoquant.py | Removes TimeoutError import from odemis.util. |
| src/odemis/driver/hamamatsurx.py | Raises builtin TimeoutError instead of util.TimeoutError. |
| src/odemis/driver/avantes.py | Removes TimeoutError import from odemis.util. |
| src/odemis/acq/path.py | Removes TimeoutError import from odemis.util. |
| src/odemis/acq/fastem.py | Removes TimeoutError import from odemis.util. |
| src/odemis/acq/align/find_overlay.py | Removes TimeoutError import from odemis.util. |
| scripts/demo_overlay.py | Removes TimeoutError import from odemis.util. |
| plugins/tileacq.py | Removes TimeoutError import from odemis.util. |
Comments suppressed due to low confidence (1)
src/odemis/util/init.py:767
- Removing
odemis.util.TimeoutErrorentirely is a breaking API change for any downstream code that doesfrom odemis.util import TimeoutErroror referencesutil.TimeoutError. Consider keeping a backwards-compatible alias (e.g.,TimeoutError = builtins.TimeoutError) for at least a deprecation cycle while still switching all internal uses to the builtin.
Also note the builtin TimeoutError subclasses OSError/IOError, which can change behavior for callers that catch OSError broadly.
# TODO: only works on Unix, needs a fallback on windows (at least, don't complain)
# from http://stackoverflow.com/questions/2281850/timeout-function-if-it-takes-too-long-to-finish
# see http://code.activestate.com/recipes/577853-timeout-decorator-with-multiprocessing/
📝 WalkthroughWalkthroughThis change set removes the custom 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odemis/driver/hamamatsurx.py (1)
1383-1393:⚠️ Potential issue | 🟡 MinorPreserve the original
queue.Emptyas the cause.The
TimeoutErrorraised here triggers Ruff B904 by raising a new exception in theexceptblock without chaining to the original. Capture the exception and chain it withfrom errto maintain timeout diagnostics and satisfy the linter.♻️ Proposed fix
- except queue.Empty: + except queue.Empty as err: if latest_response: # log the last error code received before timeout logging.error("Latest response before timeout was '%s'", latest_response) # TODO: try to close/reopen the connection. However, not re-send # the command as we don't know whether it was received, and # whether it's safe to send twice the same command. So still # report a timeout, but hopefully the next command works again. - raise TimeoutError("No answer received after %s s for command %s." - % (timeout, to_str_escape(command))) + raise TimeoutError( + "No answer received after %s s for command %s." + % (timeout, to_str_escape(command)) + ) from err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/driver/hamamatsurx.py` around lines 1383 - 1393, The except block handling queue.Empty should preserve the original exception as the cause: change the handler to catch the queue.Empty as a variable (e.g., "except queue.Empty as err") and then re-raise the TimeoutError with chaining using "from err"; keep the existing logging of latest_response and the TimeoutError message using timeout and to_str_escape(command) but raise it as "raise TimeoutError(...) from err" so the original queue.Empty is retained for diagnostics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/tileacq.py`:
- Line 47: The timeout raised by stage.moveAbs(...).result(t) is a
concurrent.futures.TimeoutError on Python 3.10 and is not being caught by the
current handler that expects the builtin TimeoutError; update the imports to
include the concurrent.futures TimeoutError (e.g., add "from concurrent.futures
import TimeoutError" alongside the existing imports such as the "from
odemis.util import img" line) or modify the exception handler around the
stage.moveAbs(...).result(t) call to catch both builtins.TimeoutError and
concurrent.futures.TimeoutError so the timeout is handled consistently (match
the pattern used in src/odemis/acq/stitching/_tiledacq.py).
---
Outside diff comments:
In `@src/odemis/driver/hamamatsurx.py`:
- Around line 1383-1393: The except block handling queue.Empty should preserve
the original exception as the cause: change the handler to catch the queue.Empty
as a variable (e.g., "except queue.Empty as err") and then re-raise the
TimeoutError with chaining using "from err"; keep the existing logging of
latest_response and the TimeoutError message using timeout and
to_str_escape(command) but raise it as "raise TimeoutError(...) from err" so the
original queue.Empty is retained for diagnostics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 691c2ed2-b22c-44d4-bcc7-075b5fc4aa5d
📒 Files selected for processing (14)
plugins/tileacq.pyscripts/demo_overlay.pysrc/odemis/acq/align/find_overlay.pysrc/odemis/acq/fastem.pysrc/odemis/acq/path.pysrc/odemis/driver/avantes.pysrc/odemis/driver/hamamatsurx.pysrc/odemis/driver/picoquant.pysrc/odemis/driver/pigcs.pysrc/odemis/driver/tescan.pysrc/odemis/driver/test/hamamatsurx_test.pysrc/odemis/driver/tmcm.pysrc/odemis/util/__init__.pysrc/odemis/util/test/util_test.py
💤 Files with no reviewable changes (2)
- src/odemis/util/test/util_test.py
- src/odemis/util/init.py
Python 2 didn't have a TimeoutError, so we introduced our own. Now there is a builtin version, let's use that one, which has the nice advantage of reducing the chances of mixing up the 2 different exceptions. Note there is again another one: concurrent.futures.TimeoutError. On Python 3.12, it's now the same as the builtin one, but on older Pythons, it wasn't. So for now, we keep them separated.
pieleric
force-pushed
the
refactor-use-builtin-timeouterror-instead-of-our-own
branch
from
8a40554 to
4a9ccb1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/odemis/driver/hamamatsurx.py (1)
1392-1393: Add exception chaining withfrom Noneto suppress misleading context.When raising a new exception inside an
exceptblock, Python best practices recommend explicit exception chaining. Sincequeue.Emptyis just the detection mechanism (not the actual cause of the timeout), usefrom Noneto suppress the misleading chain.♻️ Proposed fix
- raise TimeoutError("No answer received after %s s for command %s." - % (timeout, to_str_escape(command))) + raise TimeoutError("No answer received after %s s for command %s." + % (timeout, to_str_escape(command))) from None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/driver/hamamatsurx.py` around lines 1392 - 1393, The TimeoutError raised when no response is received should suppress chaining from the caught queue.Empty; modify the raise in the exception handler that currently does raise TimeoutError("No answer received after %s s for command %s." % (timeout, to_str_escape(command))) so it reads the same but uses "from None" (i.e., raise TimeoutError(...) from None) to avoid misleading exception context coming from queue.Empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/odemis/driver/hamamatsurx.py`:
- Around line 1392-1393: The TimeoutError raised when no response is received
should suppress chaining from the caught queue.Empty; modify the raise in the
exception handler that currently does raise TimeoutError("No answer received
after %s s for command %s." % (timeout, to_str_escape(command))) so it reads the
same but uses "from None" (i.e., raise TimeoutError(...) from None) to avoid
misleading exception context coming from queue.Empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 186221bc-4636-494d-969b-43db1fa84cc4
📒 Files selected for processing (13)
scripts/demo_overlay.pysrc/odemis/acq/align/find_overlay.pysrc/odemis/acq/fastem.pysrc/odemis/acq/path.pysrc/odemis/driver/avantes.pysrc/odemis/driver/hamamatsurx.pysrc/odemis/driver/picoquant.pysrc/odemis/driver/pigcs.pysrc/odemis/driver/tescan.pysrc/odemis/driver/test/hamamatsurx_test.pysrc/odemis/driver/tmcm.pysrc/odemis/util/__init__.pysrc/odemis/util/test/util_test.py
💤 Files with no reviewable changes (2)
- src/odemis/util/test/util_test.py
- src/odemis/util/init.py
✅ Files skipped from review due to trivial changes (4)
- src/odemis/driver/avantes.py
- src/odemis/acq/align/find_overlay.py
- src/odemis/driver/tmcm.py
- src/odemis/driver/tescan.py
🚧 Files skipped from review as they are similar to previous changes (5)
- src/odemis/driver/test/hamamatsurx_test.py
- scripts/demo_overlay.py
- src/odemis/driver/pigcs.py
- src/odemis/acq/fastem.py
- src/odemis/acq/path.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Python 2 didn't have a
TimeoutError, so we introduced our own. Now there is a builtin version, let's use that one, which has the nice advantage of reducing the chances of mixing up the 2 different exceptions.Note there is yet another one:
concurrent.futures.TimeoutError. On Python 3.12, it's now the same as the builtin one, but on older Pythons, it wasn't. So for now, we keep them separated.