[fix] METEOR PostureManager ensure current_posture is up-to-date when the posture switch ends#3468
Conversation
📝 WalkthroughWalkthroughThis PR adds post-move posture state synchronization to four posture manager classes in the cryo switching logic. After a Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (1)
src/odemis/acq/move.py (1)
1209-1213: ⚡ Quick winLog full traceback when post-move posture refresh fails.
These blocks swallow the refresh error but only log the exception message, which makes intermittent posture-sync failures hard to diagnose. Keep the non-failing behavior, but switch to traceback logging (
logging.exception) for actionable diagnostics.Proposed change
- except Exception as e: - logging.error("Failed to update posture after move: %s", e) + except Exception: + logging.exception("Failed to update posture after move")- except Exception as e: - logging.warning("Failed to update posture after move: %s", e) + except Exception: + logging.exception("Failed to update posture after move")Also applies to: 1702-1706, 2219-2223, 2480-2484
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/move.py` around lines 1209 - 1213, Replace the current except handlers that call logging.error when refresh of posture fails with logging.exception so the full traceback is captured while preserving the non-failing behavior; specifically update the try/except blocks around calls to self._update_posture(self.stage.position.value) (and the other analogous blocks that catch Exception and log only the message) to use logging.exception("Failed to update posture after move") instead of logging.error(...), leaving the try/except structure and not re-raising the exception.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/odemis/acq/move.py`:
- Around line 1209-1213: Replace the current except handlers that call
logging.error when refresh of posture fails with logging.exception so the full
traceback is captured while preserving the non-failing behavior; specifically
update the try/except blocks around calls to
self._update_posture(self.stage.position.value) (and the other analogous blocks
that catch Exception and log only the message) to use logging.exception("Failed
to update posture after move") instead of logging.error(...), leaving the
try/except structure and not re-raising the exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 741589d5-ffa7-4c3b-aba0-f88ad7248100
📒 Files selected for processing (2)
src/odemis/acq/move.pysrc/odemis/acq/test/move_tfs3_test.py
💤 Files with no reviewable changes (1)
- src/odemis/acq/test/move_tfs3_test.py
There was a problem hiding this comment.
Pull request overview
This PR addresses an intermittent race where current_posture can lag behind the completed posture-switch move, by forcing a posture recomputation at the point the switch Future finishes (helping prevent consumers/tests from observing the previous posture immediately after future.result()).
Changes:
- Recompute/update
current_posturein_doCryoSwitchSamplePosition()finally:blocks so the Future completion aligns with an up-to-date posture value. - Minor test cleanup in
test_switching_movements(removal of stray blank/whitespace-only lines).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/odemis/acq/move.py |
Forces a posture recompute when posture-switch tasks finish, aiming to eliminate stale current_posture reads immediately after the move Future resolves. |
src/odemis/acq/test/move_tfs3_test.py |
Removes stray blank/whitespace-only lines in the posture switching test. |
Comments suppressed due to low confidence (2)
src/odemis/acq/move.py:2222
- As written, this logs only the exception string and then continues, which loses the traceback and can leave
current_posturestale if_update_posture()fails. Consider usinglogging.exception(...)(orexc_info=True) and settingself.current_posture.value = UNKNOWNon failure to avoid reporting an incorrect posture.
try:
self._update_posture(self.stage.position.value)
except Exception as e:
logging.error("Failed to update posture after move: %s", e)
src/odemis/acq/move.py:2483
- Same as above: this handler drops the traceback and, on failure, leaves
current_postureunchanged (potentially incorrect). Consider logging with traceback (logging.exception/exc_info=True) and settingself.current_posture.value = UNKNOWNif_update_posture()raises.
try:
self._update_posture(self.stage.position.value)
except Exception as e:
logging.error("Failed to update posture after move: %s", e)
| except Exception as e: | ||
| logging.error("Failed to update posture after move: %s", e) |
| try: | ||
| self._update_posture(self.stage.position.value) | ||
| except Exception as e: | ||
| logging.warning("Failed to update posture after move: %s", e) |
… the posture switch ends After the posture switch ends, the stage position is updated, and the current_posture VA gets updated. However, as this happens in the background, sometimes reading current_posture immediately after the switch still indicates the previous posture. This can cause some error in code that immediately check the posture after the posture switch. => Force to recompute the posture just when the posture Future ends. In particular, this fixes intermittent failure in test_switching_movements.
pieleric
force-pushed
the
fix-meteor-posturemanager-ensure-current_posture-is-up-to-date-when-the-posture-switch-ends
branch
from
e48b18e to
88e22b1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/odemis/acq/move.py (1)
1209-1213: ⚡ Quick winConsider using
logging.exception()to preserve traceback information.The current implementation uses
logging.warning()with the exception message, which loses the full traceback. Usinglogging.exception()would preserve the stack trace and make debugging easier if this defensive path is ever hit. This pattern is also used elsewhere in the codebase (e.g., line 273).♻️ Proposed improvement
try: self._update_posture(self.stage.position.value) -except Exception as e: - logging.warning("Failed to update posture after move: %s", e) +except Exception: + logging.exception("Failed to update posture after move")This change should be applied to all four implementations (TFS1/TFS3 at line 1209, Zeiss1 at line 1702, Tescan1 at line 2219, and Jeol1 at line 2480).
Also applies to: 1702-1706, 2219-2223, 2480-2484
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/move.py` around lines 1209 - 1213, Replace the logging.warning(...) calls inside the except handlers that catch exceptions from self._update_posture(self.stage.position.value) with logging.exception(...) so the full traceback is preserved; specifically update the four occurrences around the _update_posture call in the TFS1/TFS3 block, the Zeiss1 handler, the Tescan1 handler, and the Jeol1 handler (the except blocks currently logging "Failed to update posture after move: %s") to call logging.exception with a clear message instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/odemis/acq/move.py`:
- Around line 1209-1213: Replace the logging.warning(...) calls inside the
except handlers that catch exceptions from
self._update_posture(self.stage.position.value) with logging.exception(...) so
the full traceback is preserved; specifically update the four occurrences around
the _update_posture call in the TFS1/TFS3 block, the Zeiss1 handler, the Tescan1
handler, and the Jeol1 handler (the except blocks currently logging "Failed to
update posture after move: %s") to call logging.exception with a clear message
instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b670bd4-27f9-40a5-815f-a48dff400252
📒 Files selected for processing (2)
src/odemis/acq/move.pysrc/odemis/acq/test/move_tfs3_test.py
💤 Files with no reviewable changes (1)
- src/odemis/acq/test/move_tfs3_test.py
After the posture switch ends, the stage position is updated, and the
current_posture VA gets updated. However, as this happens in the
background, sometimes reading current_posture immediately after the
switch still indicates the previous posture. This can cause some error
in code that immediately check the posture after the posture switch.
=> Force to recompute the posture just when the posture Future ends.
In particular, this fixes intermittent failure in test_switching_movements.