[MSD-263][feature] Use a single connection to Tescan from fibsemOS #3306
Conversation
ilyushkin
requested review from
Copilot,
pieleric and
tmoerkerken
and removed request for
Copilot
📝 WalkthroughWalkthroughThe PR adds a module-level Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller
participant RM as run_milling_tasks_fibsemos
participant MNG as FibsemOSMillingTaskManager\n(_persistent_millmng)
participant Future as Future/TaskHandle
participant MIC as Microscope
Note over RM,MNG: prepare or reuse persistent manager
Caller->>RM: call run_milling_tasks_fibsemos(future, tasks, path)
alt manager absent
RM->>MNG: create manager (init with path)
MNG->>MIC: create/connect microscope
else manager present
RM->>MNG: pass future, tasks, path
end
RM->>MNG: call MNG.async_run(future, tasks, path)
RM->>Future: attach task canceller = MNG.cancel
MNG->>MNG: acquire _lock, set _active, clear _cancel_requested
MNG->>MIC: execute milling stages (sequential)
MIC-->>MNG: emit progress/events
alt cancel requested
MNG->>MIC: request stop
MIC-->>MNG: acknowledge stop
MNG-->>Future: mark cancelled/failed
else normal completion
MNG-->>Future: mark completed
end
MNG->>MNG: release _lock, clear _active
Future-->>Caller: notify status
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/odemis/acq/milling/fibsemos.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: tmoerkerken
Repo: delmic/odemis PR: 3297
File: src/odemis/driver/tescan.py:119-162
Timestamp: 2025-12-17T19:02:06.675Z
Learning: In src/odemis/driver/tescan.py, each DeviceHandler instance creates an independent connection to the Tescan API with its own sem.Sem() device object and per-connection lock. Multiple connections are independent and do not require cross-connection synchronization.
🧬 Code graph analysis (1)
src/odemis/acq/milling/fibsemos.py (6)
src/odemis/model/_components.py (1)
model(570-571)src/odemis/model/_futures.py (2)
ProgressiveFuture(371-507)set_end_time(455-462)src/odemis/acq/milling/tasks.py (1)
MillingTaskSettings(69-107)src/odemis/acq/feature.py (1)
CryoFeature(144-240)src/odemis/acq/milling/millmng.py (5)
cancel(89-104)cancel(322-336)estimate_milling_time(106-111)run(220-245)run(338-391)src/odemis/util/__init__.py (1)
executeAsyncTask(806-822)
🪛 GitHub Actions: Linting
src/odemis/acq/milling/fibsemos.py
[error] 1-1: PNG metadata check detected forbidden chunks in the file (contains forbidden metadata chunks).
🪛 Ruff (0.14.8)
src/odemis/acq/milling/fibsemos.py
222-222: Undefined name CryoFeature
(F821)
224-224: Undefined name Path
(F821)
313-313: Local variable millmng is assigned to but never used
Remove assignment to unused variable millmng
(F841)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (python)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
ilyushkin
force-pushed
the
msd-263-single-fibsemos-connection-to-tescan
branch
2 times, most recently
from
4d74d9f to
11967ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/odemis/acq/milling/fibsemos.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: tmoerkerken
Repo: delmic/odemis PR: 3297
File: src/odemis/driver/tescan.py:119-162
Timestamp: 2025-12-17T19:02:13.022Z
Learning: In src/odemis/driver/tescan.py, each DeviceHandler instance creates an independent connection to the Tescan API with its own sem.Sem() device object and per-connection lock. Multiple connections are independent and do not require cross-connection synchronization.
🧬 Code graph analysis (1)
src/odemis/acq/milling/fibsemos.py (2)
src/odemis/acq/milling/millmng.py (5)
cancel(89-104)cancel(322-336)estimate_milling_time(106-111)run(220-245)run(338-391)src/odemis/util/__init__.py (1)
executeAsyncTask(806-822)
🪛 GitHub Actions: Linting
src/odemis/acq/milling/fibsemos.py
[error] 1-1: PNG metadata check failed: Contains forbidden metadata chunks (tEXt, zTXt, iTXt, eXIf, tIME).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (ubuntu-24.04)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/odemis/acq/milling/fibsemos.py (1)
291-301: Thread-safety and concurrent operation conflicts.The singleton pattern has race conditions:
Check-then-act race: Two concurrent calls can both see
_persistent_millmng is Noneand create separate instances.Mid-execution state corruption: If
run()is executing asynchronously (line 311 usesexecuteAsyncTaskwhich spawns a thread), a second call torun_milling_tasks_fibsemoswill overwrite_futureandmilling_stages(lines 295-296) while the first operation is still iterating over them inrun().Inconsistent
pathattribute:__init__(lines 200-220) never initializesself.path, but line 297 assigns_persistent_millmng.path. This creates inconsistent object state between first and subsequent calls.If milling is expected to be strictly sequential (only one operation at a time), consider adding a guard to reject concurrent calls or queue them. If the goal is purely to reuse the microscope connection, the approach suggested in the past review (shared microscope, new manager per operation) would be safer.
🔎 Minimal fix: add a lock and busy-check
+_persistent_lock = threading.Lock() _persistent_millmng: Optional["FibsemOSMillingTaskManager"] = None def run_milling_tasks_fibsemos(tasks: List[MillingTaskSettings], path: Optional[str] = None) -> futures.Future: # Reuse a single FibsemOSMillingTaskManager (single microscope connection). global _persistent_millmng + with _persistent_lock: + if _persistent_millmng is not None and _persistent_millmng._future._task_state == RUNNING: + raise RuntimeError("A milling operation is already in progress") + # Create a progressive future with running sub future. future = model.ProgressiveFuture() # Create once, then reuse the same microscope connection afterwards. - if _persistent_millmng is None: - # create milling task - _persistent_millmng = FibsemOSMillingTaskManager(future, tasks, path) - else: - _persistent_millmng._future = future - _persistent_millmng.milling_stages = convert_milling_tasks_to_milling_stages(tasks) - _persistent_millmng.path = path or os.getcwd() - _persistent_millmng.microscope._last_imaging_settings.path = _persistent_millmng.path - - future.running_subf = model.InstantaneousFuture() - future._task_lock = threading.Lock() + with _persistent_lock: + if _persistent_millmng is None: + _persistent_millmng = FibsemOSMillingTaskManager(future, tasks, path) + else: + _persistent_millmng._future = future + _persistent_millmng.milling_stages = convert_milling_tasks_to_milling_stages(tasks) + effective_path = path or os.getcwd() + _persistent_millmng.microscope._last_imaging_settings.path = effective_path + + future.running_subf = model.InstantaneousFuture() + future._task_lock = threading.Lock()
🧹 Nitpick comments (2)
src/odemis/acq/milling/fibsemos.py (2)
294-301: PR description mentionsreconfigure()method, but it's not implemented.The PR description states: "Add a
reconfigure()method to allow modification of the object's settings when needed, instead of repeated instantiation." However, the code directly manipulates internal attributes (_future,milling_stages,path) rather than using an encapsulated method.Consider extracting this logic into a
reconfigure()method onFibsemOSMillingTaskManagerfor better encapsulation and consistency with the PR objectives.🔎 Suggested refactor
Add to
FibsemOSMillingTaskManager:def reconfigure(self, future: futures.Future, tasks: List[MillingTaskSettings], path: Optional[str] = None): """Reconfigure the manager for a new milling operation while reusing the microscope connection.""" self._future = future self.milling_stages = convert_milling_tasks_to_milling_stages(tasks) effective_path = path or os.getcwd() self.microscope._last_imaging_settings.path = effective_path future.running_subf = model.InstantaneousFuture() future._task_lock = threading.Lock()Then in
run_milling_tasks_fibsemos:else: - _persistent_millmng._future = future - _persistent_millmng.milling_stages = convert_milling_tasks_to_milling_stages(tasks) - _persistent_millmng.path = path or os.getcwd() - _persistent_millmng.microscope._last_imaging_settings.path = _persistent_millmng.path - - future.running_subf = model.InstantaneousFuture() - future._task_lock = threading.Lock() + _persistent_millmng.reconfigure(future, tasks, path)
291-293: Minor: Inconsistent indentation.Line 292 has an extra leading space before the comment.
if _persistent_millmng is None: - # create milling task + # create milling task _persistent_millmng = FibsemOSMillingTaskManager(future, tasks, path)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/odemis/acq/milling/fibsemos.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: tmoerkerken
Repo: delmic/odemis PR: 3297
File: src/odemis/driver/tescan.py:119-162
Timestamp: 2025-12-17T19:02:13.022Z
Learning: In src/odemis/driver/tescan.py, each DeviceHandler instance creates an independent connection to the Tescan API with its own sem.Sem() device object and per-connection lock. Multiple connections are independent and do not require cross-connection synchronization.
🧬 Code graph analysis (1)
src/odemis/acq/milling/fibsemos.py (2)
src/odemis/acq/milling/millmng.py (5)
cancel(89-104)cancel(322-336)estimate_milling_time(106-111)run(220-245)run(338-391)src/odemis/util/__init__.py (1)
executeAsyncTask(806-822)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (1)
src/odemis/acq/milling/fibsemos.py (1)
45-46: LGTM on the type-annotated global declaration.The syntax is correct for Python 3.8+ compatibility per the earlier review feedback.
ilyushkin
force-pushed
the
msd-263-single-fibsemos-connection-to-tescan
branch
2 times, most recently
from
889c97b to
1382de0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/odemis/acq/milling/fibsemos.py (2)
238-260: LGTM! Robust worker implementation with proper cleanup.The
_run()method correctly checks for cancellation between stages and uses a finally block to ensure state cleanup. The defensive check on line 241 prevents silent failures.The static analysis tool flags the long exception message on line 245 as a style concern. Consider defining a custom exception class if this message needs to be programmatically handled.
261-287: LGTM! Thread-safe singleton run method with proper serialization.The
async_runmethod correctly enforces single-operation semantics with the_activeflag and protects all state updates with a lock. The exception handling on lines 282-286 ensures cleanup if thread creation fails.The static analysis tool flags the long exception message on line 271. Consider defining a custom exception class if programmatic handling is needed.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/odemis/acq/milling/fibsemos.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: tmoerkerken
Repo: delmic/odemis PR: 3297
File: src/odemis/driver/tescan.py:119-162
Timestamp: 2025-12-17T19:02:13.022Z
Learning: In src/odemis/driver/tescan.py, each DeviceHandler instance creates an independent connection to the Tescan API with its own sem.Sem() device object and per-connection lock. Multiple connections are independent and do not require cross-connection synchronization.
🧬 Code graph analysis (1)
src/odemis/acq/milling/fibsemos.py (2)
src/odemis/model/_futures.py (4)
end_time(395-396)InstantaneousFuture(235-282)set_end_time(455-462)ProgressiveFuture(371-507)src/odemis/util/__init__.py (1)
executeAsyncTask(806-822)
🪛 GitHub Actions: Linting
src/odemis/acq/milling/fibsemos.py
[error] 214-214: flake8: W293 blank line contains whitespace (command: 'flake8 . --select W291,W292,W293,W391,E999 --exclude src/odemis/gui/main_xrc.py,src/odemis/gui/win/dialog_xrc.py')
[error] 236-236: flake8: W291 trailing whitespace (command: 'flake8 . --select W291,W292,W293,W391,E999 --exclude src/odemis/gui/main_xrc.py,src/odemis/gui/win/dialog_xrc.py')
🪛 Ruff (0.14.10)
src/odemis/acq/milling/fibsemos.py
245-245: Avoid specifying long messages outside the exception class
(TRY003)
271-271: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (6)
src/odemis/acq/milling/fibsemos.py (6)
8-8: LGTM! Proper typing for singleton pattern.The
Unionimport and theOptionaltype hint on the module-level singleton address previous review feedback and ensure Python 3.8 compatibility.Also applies to: 45-46
48-115: LGTM! Clear documentation for microscope creation.The updated docstrings and Union return type annotation improve code clarity. The stage-version-based delegation logic is straightforward.
117-194: LGTM! Consistent documentation for conversion utilities.The new docstrings clearly describe each conversion function's purpose. The logic remains unchanged and correct.
196-213: LGTM! Well-structured singleton initialization.The refactored
__init__correctly creates the persistent microscope connection once and initializes proper threading primitives. Separating per-run state from persistent state is a good design choice.
215-232: LGTM! Correct cancellation with proper locking.The method correctly uses the lock to check and set
_cancel_requested, then releases it before callingmicroscope.stop_milling()to avoid holding the lock during a potentially blocking operation.
290-298: LGTM! Correct singleton pattern with lazy initialization.The entry point properly initializes the singleton on first call and delegates to
async_run. The global singleton ensures a single persistent microscope connection is reused across multiple milling operations.
ilyushkin
force-pushed
the
msd-263-single-fibsemos-connection-to-tescan
branch
from
1382de0 to
0279a1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/odemis/acq/milling/fibsemos.py (1)
290-298: Race condition in singleton initialization.If two threads call
run_milling_tasks_fibsemosconcurrently when_persistent_millmngisNone, both could pass the check on line 294 and create separateFibsemOSMillingTaskManagerinstances. This defeats the singleton purpose (preventing multiple connections).Consider adding a module-level lock for initialization:
🔎 Proposed fix
+_init_lock = threading.Lock() _persistent_millmng: Optional["FibsemOSMillingTaskManager"] = Nonedef run_milling_tasks_fibsemos(tasks: List[MillingTaskSettings], path: Optional[str] = None) -> futures.Future: """Run the given milling tasks asynchronously using a persistent fibsemOS manager.""" global _persistent_millmng - if _persistent_millmng is None: - _persistent_millmng = FibsemOSMillingTaskManager() + with _init_lock: + if _persistent_millmng is None: + _persistent_millmng = FibsemOSMillingTaskManager() future = model.ProgressiveFuture() return _persistent_millmng.async_run(future=future, tasks=tasks, path=path)
🧹 Nitpick comments (1)
src/odemis/acq/milling/fibsemos.py (1)
277-277:InstantaneousFuturecannot be cancelled—consider using a different subfuture.
InstantaneousFuture.cancel()always returnsFalse(see relevant snippet from_futures.py). This means thesubf.cancel()call in thecancel()method (line 227) has no effect.While the overall cancellation still works via
_cancel_requestedflag andstop_milling(), therunning_subfattribute doesn't serve its intended purpose. If cancellation through the subfuture is not needed, consider removing therunning_subfhandling entirely to avoid confusion. Otherwise, consider tracking the actual milling subfuture if one exists.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/odemis/acq/milling/fibsemos.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tmoerkerken
Repo: delmic/odemis PR: 3297
File: src/odemis/driver/tescan.py:119-162
Timestamp: 2025-12-17T19:02:13.022Z
Learning: In src/odemis/driver/tescan.py, each DeviceHandler instance creates an independent connection to the Tescan API with its own sem.Sem() device object and per-connection lock. Multiple connections are independent and do not require cross-connection synchronization.
📚 Learning: 2025-10-20T10:00:09.185Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/main_xrc.py:11982-11982
Timestamp: 2025-10-20T10:00:09.185Z
Learning: In `src/odemis/gui/main_xrc.py`, extra spaces in wxStaticText labels (such as "POI size " and "Use Current Z ") are intentionally added for GUI alignment purposes and should not be flagged as formatting issues.
Applied to files:
src/odemis/acq/milling/fibsemos.py
🧬 Code graph analysis (1)
src/odemis/acq/milling/fibsemos.py (3)
src/odemis/acq/milling/tasks.py (1)
MillingTaskSettings(69-107)src/odemis/model/_futures.py (4)
end_time(395-396)InstantaneousFuture(235-282)set_end_time(455-462)ProgressiveFuture(371-507)src/odemis/util/__init__.py (1)
executeAsyncTask(806-822)
🪛 GitHub Actions: Linting
src/odemis/acq/milling/fibsemos.py
[error] 1-1: PNG metadata check detected forbidden metadata chunks in the file.
🪛 Ruff (0.14.10)
src/odemis/acq/milling/fibsemos.py
245-245: Avoid specifying long messages outside the exception class
(TRY003)
271-271: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (6)
src/odemis/acq/milling/fibsemos.py (6)
45-46: LGTM!The singleton declaration with proper type annotation and forward reference is correctly implemented. This addresses the Python 3.8 compatibility concern from the previous review.
101-103: LGTM!The
Unionreturn type annotation accurately reflects the two possible return types and improves type safety.
196-213: LGTM!The refactored
__init__correctly separates the persistent microscope connection from per-run state. Thread synchronization primitives (_lock,_active,_cancel_requested) are properly initialized. The per-run state is correctly initialized to empty/None and will be populated inasync_run.
215-232: LGTM!The cancellation logic is well-designed:
- Lock protects state transitions without being held during blocking
stop_milling()call- Idempotent behavior (returns
Trueif already cancelled)- Defensive
getattrforrunning_subfhandles edge cases gracefully
269-287: Verify lock scope during thread spawning.The lock is held while calling
executeAsyncTask(lines 282-286), which spawns a thread that will immediately try to acquire the same lock in_run. This is safe but causes the worker thread to block briefly untilasync_runreleases the lock.This design ensures all state is atomically configured before the worker can access it, which is reasonable. Just confirming this is intentional.
130-160: LGTM!The conversion helper functions (
_convert_rectangle_pattern,_convert_trench_pattern,_convert_microexpansion_pattern) have improved docstrings and unchanged logic.
pieleric
changed the title
2 participants
Use a singleton instance of the
FibsemOSMillingTaskManagerclass to prevent fibsemOS from creating a new connection for each instance.