[MSD-351][perf] METEOR: Lazy-load streams with per-feature cleanup and async loading#3391
Merged
pieleric merged 1 commit intodelmic:masterfrom Mar 10, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds lazy-loading and per-feature memory management for acquired streams. A new public function, load_feature_streams_from_disk(feature, path), loads streams from disk into a feature on demand. The acquisition pipeline accepts an optional per_feature_callback that is invoked after each feature is acquired. GUI stream controllers use background loading for evicted feature streams and provide routines to clear non-current feature streams from memory. Eager loading of all feature streams at project load is removed. New tests validate stream clearing and raw-data cleanup behaviors. Sequence Diagram(s)sequenceDiagram
participant User
participant AcqTask as CryoFeatureAcquisitionTask
participant Callback as per_feature_callback
participant StreamCtrl as CryoAcquiController
User->>AcqTask: acquire_at_features(features, ..., per_feature_callback)
loop For each feature
AcqTask->>AcqTask: move stage / focus / acquire
AcqTask->>Callback: invoke per_feature_callback()
Callback->>StreamCtrl: _on_per_feature_acquired()
StreamCtrl->>StreamCtrl: _clear_all_feature_streams_except(current)
end
AcqTask->>StreamCtrl: final completion
StreamCtrl->>StreamCtrl: _on_feature_acquisition_done() (cleanup except current)
sequenceDiagram
participant User as Feature selector
participant StreamCtrl as CryoAcquiredStreamsController
participant BGLoader as Background thread
participant Disk as Disk (feature streams)
participant UI as Main/UI thread
User->>StreamCtrl: _on_current_feature_changes(new_feature)
StreamCtrl->>StreamCtrl: _clear_all_feature_streams_except(new_feature)
alt streams present in memory
StreamCtrl->>UI: _display_feature_streams(new_feature)
UI->>UI: add streams to tab model / refresh
else streams evicted
StreamCtrl->>BGLoader: _load_feature_streams_bg(new_feature)
BGLoader->>Disk: load_feature_streams_from_disk(new_feature)
BGLoader->>UI: _display_feature_streams(new_feature)
UI->>UI: add streams to tab model / refresh
end
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/gui/cont/acquisition/cryo_acq.py`:
- Around line 447-449: The loop currently calls
_tab._acquired_stream_controller.clear_feature_streams(feature) for each
non-current feature which triggers expensive full UI cleanup; instead, perform a
model-only eviction for each non-current feature (e.g. call a new or existing
model-only method like
_acquired_stream_controller.evict_feature_streams_model(feature) or mark them in
a list) and only once after the loop perform the full UI cleanup pass (e.g.
_acquired_stream_controller.clear_feature_streams_for_features(evicted_list) or
a single _acquired_stream_controller.trim_ui() call). Update the code around
self._tab_data.main.features.value and current_feature to collect non-current
features, use a model-only eviction API inside the loop, and then call one full
cleanup on the aggregated list (or implement the model-only eviction method if
missing).
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2197-2213: Both _load_feature_streams and clear_feature_streams
are missing type hints; update their signatures to include parameter and return
types (e.g. def _load_feature_streams(self, feature: CryoFeature) -> None and
def clear_feature_streams(self, feature: Optional[CryoFeature] = None) -> None),
add an Optional import from typing if needed, and ensure CryoFeature is imported
or forward-referenced so the annotations resolve.
In `@src/odemis/gui/model/stream_view.py`:
- Around line 606-609: The current code unconditionally clears node.image.value
which can blank the underlying stream for other views; change this to only clear
the image for projection nodes that are owned by this view. Concretely, in the
removal path where node.image.value = None is set, guard that assignment with a
check that the node is a per-view projection node (e.g., if isinstance(node,
ProjectionNode) and node.owner is self, or if node in self.projection_nodes),
and only then set node.image.value = None; otherwise leave shared stream images
untouched.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/odemis/acq/acqmng.pysrc/odemis/acq/feature.pysrc/odemis/gui/cont/acquisition/cryo_acq.pysrc/odemis/gui/cont/stream_bar.pysrc/odemis/gui/cont/tabs/cryo_chamber_tab.pysrc/odemis/gui/model/stream_view.py
💤 Files with no reviewable changes (1)
- src/odemis/gui/cont/tabs/cryo_chamber_tab.py
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/odemis/gui/cont/stream_bar.py (1)
2200-2295: 🛠️ Refactor suggestion | 🟠 MajorAdd type annotations to all new method signatures.
The following methods lack parameter and return type annotations, violating the coding guideline requiring type hints:
_load_feature_streams_bg(self, feature)→_load_feature_streams_bg(self, feature: "CryoFeature") -> None_display_feature_streams(self, feature)→_display_feature_streams(self, feature: Optional["CryoFeature"]) -> None_clear_all_feature_streams_except(self, current_feature)→_clear_all_feature_streams_except(self, current_feature: Optional["CryoFeature"]) -> None_load_feature_streams(self, feature)→_load_feature_streams(self, feature: "CryoFeature") -> Noneclear_feature_streams(self, feature=None)→clear_feature_streams(self, feature: Optional["CryoFeature"] = None) -> None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/stream_bar.py` around lines 2200 - 2295, Add missing type annotations on the listed methods: annotate _load_feature_streams_bg(self, feature: "CryoFeature") -> None, _display_feature_streams(self, feature: Optional["CryoFeature"]) -> None (keep `@call_in_wx_main`), _clear_all_feature_streams_except(self, current_feature: Optional["CryoFeature"]) -> None, _load_feature_streams(self, feature: "CryoFeature") -> None, and clear_feature_streams(self, feature: Optional["CryoFeature"] = None) -> None; import typing.Optional at top if not present and use forward-reference strings ("CryoFeature") to avoid circular imports, ensuring signatures match the review examples and preserve existing docstrings and behavior.
🧹 Nitpick comments (1)
src/odemis/gui/cont/stream_bar.py (1)
2351-2354: Consider logging instead of silently passing onmalloc_trimfailure.While silently skipping is acceptable for this optional optimization, logging at debug level would aid diagnosability without adding noise:
try: ctypes.CDLL("libc.so.6").malloc_trim(0) except Exception: - pass # Not on Linux, or libc not found — silently skip + logging.debug("malloc_trim not available (non-Linux or libc not found)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/stream_bar.py` around lines 2351 - 2354, Replace the silent except in the malloc_trim call with a debug log so failures are visible during troubleshooting: in stream_bar.py where ctypes.CDLL("libc.so.6").malloc_trim(0) is invoked, catch Exception as e and call the module/class logger (or a logger obtained via logging.getLogger) with logger.debug("malloc_trim skipped or failed: %s", e) instead of using pass; keep the behavior unchanged (no raise) so non-Linux or missing-libc cases remain non-fatal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/gui/cont/acquisition/cryo_acq.py`:
- Around line 440-441: The file calls gc.collect() (see gc.collect() near the
end of the acquisition routine in cryo_acq.py) but never imports the gc module;
add a top-level import for the garbage-collector module (import gc) alongside
the other standard library imports so gc.collect() resolves and no NameError is
raised.
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2200-2293: Whitespace lint errors are caused by trailing spaces
and inconsistent blank lines in the methods _load_feature_streams_bg,
_display_feature_streams, _clear_all_feature_streams_except, and
_load_feature_streams; run the prescribed formatter (autopep8 --in-place
--select W291,W292,W293,W391 src/odemis/gui/cont/stream_bar.py) or manually
remove trailing spaces and fix extra/missing blank lines at the flagged
locations (around the start of _load_feature_streams_bg, inside its
except/finally blocks, and near the ends of _display_feature_streams and
_load_feature_streams) so the file conforms to the W29x/W391 rules.
- Around line 2200-2240: The background worker _load_feature_streams_bg
currently calls _load_feature_streams which directly mutates shared model lists
(load_feature_streams_from_disk populates CryoFeature.streams and code appends
to self._tab_data_model.streams.value) causing a race; fix by changing
_load_feature_streams/_load_feature_streams_bg so the background thread only
reads/parses stream data into local structures and then uses call_in_wx_main to
perform all mutations (assign/extend CryoFeature.streams.value and append to
self._tab_data_model.streams.value) or alternatively protect mutations with a
dedicated threading.Lock, ensuring _display_feature_streams is also invoked via
call_in_wx_main after the safe mutation.
- Around line 2227-2240: The finally block currently calls
_display_feature_streams(feature) even after an exception or early return from
the preceding except, causing displays after failed loads; move the display
logic out of the finally and into the successful path (i.e., only call
_display_feature_streams(feature) when no exception occurred and after verifying
self._current_feature is feature and feature in
self._tab_data_model.main.features.value), remove the display call from the
finally, and ensure any temporary imports (import sys) are moved to the module
top-level imports; reference _display_feature_streams, self._current_feature,
and self._tab_data_model.main.features.value to find and update the logic.
---
Duplicate comments:
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2200-2295: Add missing type annotations on the listed methods:
annotate _load_feature_streams_bg(self, feature: "CryoFeature") -> None,
_display_feature_streams(self, feature: Optional["CryoFeature"]) -> None (keep
`@call_in_wx_main`), _clear_all_feature_streams_except(self, current_feature:
Optional["CryoFeature"]) -> None, _load_feature_streams(self, feature:
"CryoFeature") -> None, and clear_feature_streams(self, feature:
Optional["CryoFeature"] = None) -> None; import typing.Optional at top if not
present and use forward-reference strings ("CryoFeature") to avoid circular
imports, ensuring signatures match the review examples and preserve existing
docstrings and behavior.
---
Nitpick comments:
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2351-2354: Replace the silent except in the malloc_trim call with
a debug log so failures are visible during troubleshooting: in stream_bar.py
where ctypes.CDLL("libc.so.6").malloc_trim(0) is invoked, catch Exception as e
and call the module/class logger (or a logger obtained via logging.getLogger)
with logger.debug("malloc_trim skipped or failed: %s", e) instead of using pass;
keep the behavior unchanged (no raise) so non-Linux or missing-libc cases remain
non-fatal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1222d4af-55e4-44c7-a59c-eaf8d269f7ef
📒 Files selected for processing (3)
src/odemis/acq/acqmng.pysrc/odemis/gui/cont/acquisition/cryo_acq.pysrc/odemis/gui/cont/stream_bar.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/odemis/acq/acqmng.py
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/odemis/gui/cont/stream_bar.py (2)
2217-2234:⚠️ Potential issue | 🔴 CriticalKeep background loading read-only; apply model mutations on the main thread.
_load_feature_streams_bgruns in a worker thread but_load_feature_streamsmutatesfeature.streams.valueandself._tab_data_model.streams.value. Those objects are also used by GUI-thread callbacks, so this introduces a race during feature switches/clears.Suggested fix direction
- self._load_feature_streams(feature) + loaded_streams = self._read_feature_streams(feature) # background I/O only + wx.CallAfter(self._apply_loaded_feature_streams, feature, loaded_streams)Then keep
_apply_loaded_feature_streams(...)responsible for updating:
feature.streams.valueself._tab_data_model.streams.value- final display call
Also applies to: 2293-2302
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/stream_bar.py` around lines 2217 - 2234, _background worker _load_feature_streams_bg currently calls _load_feature_streams which mutates shared GUI state (feature.streams.value and self._tab_data_model.streams.value) from a background thread; change the flow so _load_feature_streams only performs read-only data loading and returns the loaded streams, then implement or use _apply_loaded_feature_streams to perform all mutations (assigning feature.streams.value, updating self._tab_data_model.streams.value and invoking _display_feature_streams) on the main thread after verifying self._current_feature is still the same and feature is in self._tab_data_model.main.features.value; ensure the background except/finally block passes the loaded result to the main-thread applier and remove any direct mutations from _load_feature_streams and from the background thread (also apply same fix to the other occurrence around lines 2293-2302).
2200-2200:⚠️ Potential issue | 🟡 MinorAdd type hints to the new stream-loading/clearing methods.
These new/modified method signatures are missing parameter and return annotations.
Proposed fix
- def _load_feature_streams_bg(self, feature): + def _load_feature_streams_bg(self, feature: "CryoFeature") -> None: @@ - def _display_feature_streams(self, feature): + def _display_feature_streams(self, feature: Optional["CryoFeature"]) -> None: @@ - def _load_feature_streams(self, feature): + def _load_feature_streams(self, feature: "CryoFeature") -> None: @@ - def clear_feature_streams(self, feature): + def clear_feature_streams(self, feature: "CryoFeature") -> None:As per coding guidelines
**/*.py: Always use type hints for function parameters and return types in Python code.Also applies to: 2243-2243, 2278-2278, 2304-2304
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/stream_bar.py` at line 2200, The methods for loading/clearing streams (e.g. _load_feature_streams_bg and the other new/modified stream methods referenced at lines near 2243, 2278, 2304) lack parameter and return type annotations; update each function signature to include precise type hints for parameters (use the actual model type if available, otherwise typing.Any or a forward reference) and for the return type (most likely -> None or the appropriate coroutine/Task type), and add any required imports from typing (e.g., Any, Optional, Coroutine) so the file complies with the repository's type-hinting guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/acq/feature.py`:
- Line 463: Add explicit type hints for the per_feature_callback parameter in
both function signatures where it appears (the two functions that currently
declare per_feature_callback = None); import typing helpers and change the
parameter from an untyped default to something like per_feature_callback:
Optional[Callable[[/*args you expect e.g. Feature, Dict[str, Any]*/], Any]] =
None (or Optional[Callable[..., None]] if it returns nothing) and update imports
to include Optional, Callable, Any from typing; ensure any internal uses are
consistent with the chosen callback signature so type checkers can validate call
sites.
In `@src/odemis/gui/cont/acquisition/cryo_acq.py`:
- Around line 425-436: The callback _on_per_feature_acquired currently calls
self._tab._acquired_stream_controller._clear_all_feature_streams() from a worker
thread; schedule that call to run on the wx main thread instead (e.g., using
wx.CallAfter or equivalent) so GUI/controller state is mutated only on the main
thread, and keep the try/except around the scheduled call (or handle exceptions
inside the scheduled function) to log errors via the existing logger; update
references in _on_per_feature_acquired to use
wx.CallAfter(self._tab._acquired_stream_controller._clear_all_feature_streams)
(or a small wrapper method) rather than calling _clear_all_feature_streams()
directly.
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2445-2446: The call to an undefined helper
_clear_all_feature_streams_except in CryoFIBAcquiredStreamsController causes
AttributeError when switching features; add an implementation on
CryoFIBAcquiredStreamsController that iterates existing feature stream entries
and removes/clears them except for the supplied feature (or, if a shared
mixin/parent already has intended logic, import or inherit that mixin instead);
ensure the method signature matches _clear_all_feature_streams_except(self,
feature) and that any internal helpers referenced (e.g., the controller's stream
storage attribute or remove/clear methods) are used so the call succeeds without
raising AttributeError.
---
Duplicate comments:
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2217-2234: _background worker _load_feature_streams_bg currently
calls _load_feature_streams which mutates shared GUI state
(feature.streams.value and self._tab_data_model.streams.value) from a background
thread; change the flow so _load_feature_streams only performs read-only data
loading and returns the loaded streams, then implement or use
_apply_loaded_feature_streams to perform all mutations (assigning
feature.streams.value, updating self._tab_data_model.streams.value and invoking
_display_feature_streams) on the main thread after verifying
self._current_feature is still the same and feature is in
self._tab_data_model.main.features.value; ensure the background except/finally
block passes the loaded result to the main-thread applier and remove any direct
mutations from _load_feature_streams and from the background thread (also apply
same fix to the other occurrence around lines 2293-2302).
- Line 2200: The methods for loading/clearing streams (e.g.
_load_feature_streams_bg and the other new/modified stream methods referenced at
lines near 2243, 2278, 2304) lack parameter and return type annotations; update
each function signature to include precise type hints for parameters (use the
actual model type if available, otherwise typing.Any or a forward reference) and
for the return type (most likely -> None or the appropriate coroutine/Task
type), and add any required imports from typing (e.g., Any, Optional, Coroutine)
so the file complies with the repository's type-hinting guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31bb8a7a-11c4-4521-8a10-e36143401f0a
📒 Files selected for processing (3)
src/odemis/acq/feature.pysrc/odemis/gui/cont/acquisition/cryo_acq.pysrc/odemis/gui/cont/stream_bar.py
tmoerkerken
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odemis/gui/cont/stream_bar.py (1)
2467-2479:⚠️ Potential issue | 🟡 MinorDo not mutate
self.stream_controllerswhile iterating it.Removing entries from the same list being iterated can skip controllers and leave stale panels/streams behind.
Safe iteration fix
- for sc in self.stream_controllers: + for sc in self.stream_controllers.copy(): if not isinstance(sc.stream, StaticSEMStream): logging.warning("Unexpected non static stream: %s", sc.stream) continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/stream_bar.py` around lines 2467 - 2479, The loop mutates self.stream_controllers while iterating it causing skipped items; change the iteration to use a snapshot or collect removals first (e.g., iterate over list(self.stream_controllers) or build a to_remove list) and then perform removals so all controllers are handled; update the block that checks isinstance(sc.stream, StaticSEMStream), skips overview streams via _tab_data_model.overviewStreams.value, calls _stream_bar.remove_stream_panel(sc.stream_panel) and v.removeStream(sc.stream) if present, and finally removes controllers from self.stream_controllers only in the separate removal pass (avoid calling self.stream_controllers.remove(sc) inside the iteration).
♻️ Duplicate comments (2)
src/odemis/gui/cont/stream_bar.py (2)
2200-2200:⚠️ Potential issue | 🟡 MinorAdd type annotations to the newly introduced helper methods.
These new/updated method signatures still miss parameter/return annotations (
_load_feature_streams_bg,_display_feature_streams,_clear_all_feature_streams_except,_load_feature_streams,clear_feature_streams).Proposed signature updates
- def _load_feature_streams_bg(self, feature): + def _load_feature_streams_bg(self, feature: "CryoFeature") -> None: - def _display_feature_streams(self, feature): + def _display_feature_streams(self, feature: Optional["CryoFeature"]) -> None: - def _clear_all_feature_streams_except(self, current_feature) -> None: + def _clear_all_feature_streams_except(self, current_feature: Optional["CryoFeature"]) -> None: - def _load_feature_streams(self, feature): + def _load_feature_streams(self, feature: "CryoFeature") -> None: - def clear_feature_streams(self, feature): + def clear_feature_streams(self, feature: "CryoFeature") -> None:As per coding guidelines
**/*.py: Always use type hints for function parameters and return types in Python code.Also applies to: 2243-2243, 2256-2256, 2278-2278, 2304-2304
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/stream_bar.py` at line 2200, The listed helper methods lack type hints; update the signatures for _load_feature_streams_bg, _display_feature_streams, _clear_all_feature_streams_except, _load_feature_streams, and clear_feature_streams to include explicit parameter and return type annotations (e.g. annotate feature parameters as the correct domain type such as str or a Feature class, annotate stream/ids collections as List/Iterable[Type] or Optional[...] as appropriate, and set return types like -> None or the actual return type). Ensure you import any required typing symbols (List, Iterable, Optional, Sequence, Union, etc.) and mirror the project’s typing conventions. Keep annotations consistent across these methods so callers and static checks know the expected types.
2217-2218:⚠️ Potential issue | 🔴 CriticalBackground thread is mutating GUI-observed model lists.
_load_feature_streams_bg()calls_load_feature_streams()from a worker thread, and that path mutatesfeature.streams.valueplusself._tab_data_model.streams.value. These collections are GUI-observed state and should be mutated on the wx main thread only.Suggested direction
- self._load_feature_streams(feature) + loaded_streams = self._read_feature_streams_from_disk(feature) + wx.CallAfter(self._apply_loaded_feature_streams, feature, loaded_streams)Also applies to: 2293-2302
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/stream_bar.py` around lines 2217 - 2218, _background thread _load_feature_streams_bg() is calling _load_feature_streams() which mutates GUI-observed collections (feature.streams.value and self._tab_data_model.streams.value); move those mutations to the wx main thread by calling the UI-mutating function via wx.CallAfter (or posting a custom wx event) instead of invoking _load_feature_streams() directly from the worker thread. Modify _load_feature_streams_bg() to perform only non-UI work and then schedule a main-thread callback (e.g. call a small wrapper like _apply_loaded_feature_streams(feature, loaded_streams) via wx.CallAfter) that updates feature.streams.value and self._tab_data_model.streams.value; apply the same change to the other background loader call sites that currently call _load_feature_streams() (the similar block around the second occurrence).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/gui/test/test_cryo_stream_cleanup.py`:
- Line 43: Run the repository cleanup for trailing whitespace and blank-line
lint errors in the new test file
src/odemis/gui/test/test_cryo_stream_cleanup.py: remove any trailing spaces and
extra blank lines (W291/W293) and reformat using autopep8 with the command
provided (autopep8 --in-place --select W291,W292,W293,W391
src/odemis/gui/test/test_cryo_stream_cleanup.py), then re-run tests/flake8 to
confirm the file (including the test functions in that file) no longer triggers
W291/W293/W292/W391 failures.
- Line 21: Add explicit return type annotations "-> None" to the unittest
lifecycle and test methods so they satisfy the project's typing rules: update
the setUp method and every test_* method (e.g., setUp, all methods named
test_...) in this test module to declare a return type of None (e.g., def
setUp(self) -> None: and def test_something(self) -> None:), keeping signatures
otherwise unchanged.
---
Outside diff comments:
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2467-2479: The loop mutates self.stream_controllers while
iterating it causing skipped items; change the iteration to use a snapshot or
collect removals first (e.g., iterate over list(self.stream_controllers) or
build a to_remove list) and then perform removals so all controllers are
handled; update the block that checks isinstance(sc.stream, StaticSEMStream),
skips overview streams via _tab_data_model.overviewStreams.value, calls
_stream_bar.remove_stream_panel(sc.stream_panel) and v.removeStream(sc.stream)
if present, and finally removes controllers from self.stream_controllers only in
the separate removal pass (avoid calling self.stream_controllers.remove(sc)
inside the iteration).
---
Duplicate comments:
In `@src/odemis/gui/cont/stream_bar.py`:
- Line 2200: The listed helper methods lack type hints; update the signatures
for _load_feature_streams_bg, _display_feature_streams,
_clear_all_feature_streams_except, _load_feature_streams, and
clear_feature_streams to include explicit parameter and return type annotations
(e.g. annotate feature parameters as the correct domain type such as str or a
Feature class, annotate stream/ids collections as List/Iterable[Type] or
Optional[...] as appropriate, and set return types like -> None or the actual
return type). Ensure you import any required typing symbols (List, Iterable,
Optional, Sequence, Union, etc.) and mirror the project’s typing conventions.
Keep annotations consistent across these methods so callers and static checks
know the expected types.
- Around line 2217-2218: _background thread _load_feature_streams_bg() is
calling _load_feature_streams() which mutates GUI-observed collections
(feature.streams.value and self._tab_data_model.streams.value); move those
mutations to the wx main thread by calling the UI-mutating function via
wx.CallAfter (or posting a custom wx event) instead of invoking
_load_feature_streams() directly from the worker thread. Modify
_load_feature_streams_bg() to perform only non-UI work and then schedule a
main-thread callback (e.g. call a small wrapper like
_apply_loaded_feature_streams(feature, loaded_streams) via wx.CallAfter) that
updates feature.streams.value and self._tab_data_model.streams.value; apply the
same change to the other background loader call sites that currently call
_load_feature_streams() (the similar block around the second occurrence).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c5a9cb5-d0c8-409a-a397-48ab3032bf72
📒 Files selected for processing (4)
src/odemis/acq/feature.pysrc/odemis/gui/cont/acquisition/cryo_acq.pysrc/odemis/gui/cont/stream_bar.pysrc/odemis/gui/test/test_cryo_stream_cleanup.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/odemis/gui/test/test_cryo_stream_cleanup.py (1)
21-21:⚠️ Potential issue | 🟡 MinorAdd explicit
-> Nonereturn annotations to lifecycle/test methods.
setUpand thetest_*methods currently miss return type hints.Proposed patch
- def setUp(self): + def setUp(self) -> None: @@ - def test_clear_feature_streams_removes_from_model(self): + def test_clear_feature_streams_removes_from_model(self) -> None: @@ - def test_clear_feature_streams_clears_raw_data(self): + def test_clear_feature_streams_clears_raw_data(self) -> None: @@ - def test_clear_all_feature_streams_except_preserves_current(self): + def test_clear_all_feature_streams_except_preserves_current(self) -> None: @@ - def test_clear_all_feature_streams_clears_everything(self): + def test_clear_all_feature_streams_clears_everything(self) -> None:As per coding guidelines
**/*.py: Always use type hints for function parameters and return types in Python code.Also applies to: 99-99, 122-122, 138-138, 156-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/test/test_cryo_stream_cleanup.py` at line 21, Add explicit return type annotations "-> None" to the lifecycle method setUp and to all test methods (test_* functions) in this test module; locate the setUp method and every function whose name starts with "test_" in src/odemis/gui/test/test_cryo_stream_cleanup.py and change their definitions to include the return annotation (e.g., def setUp(self) -> None: and def test_something(self) -> None:) so they conform to the project's type-hinting guidelines.src/odemis/gui/cont/acquisition/cryo_acq.py (1)
434-435:⚠️ Potential issue | 🔴 CriticalPer-feature cleanup currently routes through a GC-heavy global clear path.
On Line 435,
_clear_all_feature_streams()calls intoclear_feature_streams()for each feature (insrc/odemis/gui/cont/stream_bar.py), which performs expensive UI teardown andgc.collect(). Running that after every acquired feature can reintroduce the crash/stall pattern this PR is trying to avoid during async activity. Please switch this callback to a non-GC/model-only eviction path and reserve GC for a known-safe point after async loading is fully idle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/acquisition/cryo_acq.py` around lines 434 - 435, Replace the per-feature call to self._tab._acquired_stream_controller._clear_all_feature_streams() with a model-only eviction path that does not call clear_feature_streams() (which lives in src/odemis/gui/cont/stream_bar.py) or invoke gc.collect(); e.g., call a new or existing method like _evict_feature_streams_model_only() or evict_feature_streams(no_gc=True) on _acquired_stream_controller that only updates internal model/state and skips UI teardown, and ensure any gc.collect() is deferred and invoked only from a single known-safe "async idle" point after loading completes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/unit_tests_cloud.yml:
- Around line 216-220: The CI step "Run tests from odemis.gui.cont.stream_bar
(stream cleanup)" must run the new cleanup test in no-hardware mode and invoke
the test file directly using the repository convention; change the step to set
the environment variable TEST_NOHW=1 and call the test file
(src/odemis/gui/test/test_cryo_stream_cleanup.py) executing the specific
TestCase.test_method rather than using python -m unittest, e.g. use the pattern
env TEST_NOHW=1 python3 src/odemis/.../name_of_the_test_file.py
TestCaseClassName.test_method_name so the workflow runs the single test in
no-hardware mode.
---
Duplicate comments:
In `@src/odemis/gui/cont/acquisition/cryo_acq.py`:
- Around line 434-435: Replace the per-feature call to
self._tab._acquired_stream_controller._clear_all_feature_streams() with a
model-only eviction path that does not call clear_feature_streams() (which lives
in src/odemis/gui/cont/stream_bar.py) or invoke gc.collect(); e.g., call a new
or existing method like _evict_feature_streams_model_only() or
evict_feature_streams(no_gc=True) on _acquired_stream_controller that only
updates internal model/state and skips UI teardown, and ensure any gc.collect()
is deferred and invoked only from a single known-safe "async idle" point after
loading completes.
In `@src/odemis/gui/test/test_cryo_stream_cleanup.py`:
- Line 21: Add explicit return type annotations "-> None" to the lifecycle
method setUp and to all test methods (test_* functions) in this test module;
locate the setUp method and every function whose name starts with "test_" in
src/odemis/gui/test/test_cryo_stream_cleanup.py and change their definitions to
include the return annotation (e.g., def setUp(self) -> None: and def
test_something(self) -> None:) so they conform to the project's type-hinting
guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 97491308-a195-4342-b5ae-806865239dbc
📒 Files selected for processing (3)
.github/workflows/unit_tests_cloud.ymlsrc/odemis/gui/cont/acquisition/cryo_acq.pysrc/odemis/gui/test/test_cryo_stream_cleanup.py
tmoerkerken
force-pushed
the
memopt-stream-reloading
branch
from
d02d222 to
ff2dd03
Compare
tmoerkerken
changed the title
tmoerkerken
force-pushed
the
memopt-stream-reloading
branch
5 times, most recently
from
369e054 to
9ac161d
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odemis/gui/cont/stream_bar.py (1)
2445-2457:⚠️ Potential issue | 🟡 MinorDo not remove from
self.stream_controllerswhile iterating it directly.This loop mutates the same list it iterates, so entries can be skipped when more than one controller is present.
Suggested fix
- for sc in self.stream_controllers: + for sc in self.stream_controllers.copy(): if not isinstance(sc.stream, StaticSEMStream): logging.warning("Unexpected non static stream: %s", sc.stream) continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/stream_bar.py` around lines 2445 - 2457, The loop over self.stream_controllers mutates that list during iteration which can skip items; change it to iterate over a snapshot or collect controllers to remove first: e.g., build a to_remove list while checking isinstance(sc.stream, StaticSEMStream), membership in self._tab_data_model.overviewStreams.value, calling self._stream_bar.remove_stream_panel(sc.stream_panel) and v.removeStream(sc.stream) as needed, then after the loop remove each sc from self.stream_controllers (or reconstruct the list excluding those to remove) to avoid in-loop mutation; use the existing symbols stream_controllers, StaticSEMStream, _tab_data_model.overviewStreams, _stream_bar.remove_stream_panel, and v.removeStream to locate the code.
♻️ Duplicate comments (4)
src/odemis/gui/test/test_cryo_stream_cleanup.py (1)
20-169:⚠️ Potential issue | 🟡 MinorAdd explicit
-> Noneannotations to unittest lifecycle/test methods.
setUpand alltest_*methods should declare-> Noneto satisfy repository typing rules.Suggested fix
- def setUp(self): + def setUp(self) -> None: @@ - def test_clear_feature_streams_removes_from_model(self): + def test_clear_feature_streams_removes_from_model(self) -> None: @@ - def test_clear_feature_streams_clears_raw_data(self): + def test_clear_feature_streams_clears_raw_data(self) -> None: @@ - def test_clear_all_feature_streams_except_preserves_current(self): + def test_clear_all_feature_streams_except_preserves_current(self) -> None: @@ - def test_clear_all_feature_streams_clears_everything(self): + def test_clear_all_feature_streams_clears_everything(self) -> None:As per coding guidelines
**/*.py: Always use type hints for function parameters and return types in Python code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/test/test_cryo_stream_cleanup.py` around lines 20 - 169, Add explicit return type annotations "-> None" to the unittest lifecycle and test methods so they satisfy typing rules: update setUp and every test_* method (e.g., setUp, test_clear_feature_streams_removes_from_model, test_clear_feature_streams_clears_raw_data, test_clear_all_feature_streams_except_preserves_current, test_clear_all_feature_streams_clears_everything) to declare a None return type; keep signatures and bodies unchanged aside from appending "-> None" to each def line.src/odemis/gui/model/stream_view.py (1)
606-609:⚠️ Potential issue | 🟠 MajorGuard frame clearing to projection-owned nodes only.
This currently clears
image.valuefor any node with an image, which can blank a shared stream image still used by other views. Restrict the frame drop to per-view projection nodes.Suggested fix
if hasattr(node, "image"): node.image.unsubscribe(self._onNewImage) # Drop the rendered frame immediately so the DataArray # can be GC'd without waiting for the projection thread # to wake up and release its reference. - node.image.value = None + if isinstance(node, DataProjection): + node.image.value = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/model/stream_view.py` around lines 606 - 609, The code unconditionally clears node.image.value which can blank shared images; only drop frames for projection nodes owned by this view. Change the unconditional assignment to a guarded clear: check that the node is one of this view's projection-owned nodes (e.g. if node in self._projection_nodes or if getattr(node, "projection_owner", None) is self) before setting node.image.value = None so shared stream images used by other views remain intact.src/odemis/gui/cont/stream_bar.py (2)
2213-2213:⚠️ Potential issue | 🔴 CriticalAvoid mutating feature stream collections from the background thread.
The worker path still mutates
feature.streams.valueviaload_feature_streams_from_disk(...); cleanup/display paths run on the GUI thread, so this is a shared-state race.Suggested direction
- self._load_feature_streams(feature) + loaded_streams = self._read_feature_streams(feature) # pure I/O, no model mutation + wx.CallAfter(self._apply_loaded_feature_streams, feature, loaded_streams)def _read_feature_streams(self, feature): # return List[StaticStream], do not touch feature.streams.value `@call_in_wx_main` def _apply_loaded_feature_streams(self, feature, streams): if self._current_feature is feature and feature in self._tab_data_model.main.features.value: feature.streams.value.extend(streams) self._display_feature_streams(feature)Also applies to: 2289-2290
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/stream_bar.py` at line 2213, The background worker currently mutates shared GUI state by calling load_feature_streams_from_disk which alters feature.streams.value inside _load_feature_streams; instead, refactor so the background thread only reads and returns a plain list of streams (e.g., implement _read_feature_streams or have load_feature_streams_from_disk return List[StaticStream] without touching feature.streams.value), then use a `@call_in_wx_main` method (e.g., _apply_loaded_feature_streams) to check _current_feature and membership in _tab_data_model.main.features.value and safely extend feature.streams.value and call _display_feature_streams on the GUI thread; apply the same pattern for the other occurrence around lines 2289-2290.
2171-2171:⚠️ Potential issue | 🟡 MinorAdd type hints to newly introduced/updated method signatures.
These methods currently miss parameter/return annotations, which violates the repository typing rule for Python files.
As per coding guidelines
**/*.py: Always use type hints for function parameters and return types in Python code.Also applies to: 2194-2194, 2234-2234, 2273-2273, 2296-2296
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/stream_bar.py` at line 2171, The method _on_current_feature_changes currently lacks type annotations; update its signature to include a parameter type for feature (e.g., Optional[FeatureType] or the appropriate class) and an explicit return type (likely -> None), and do the same for the other newly introduced/updated methods referenced in the review (add parameter and return type hints consistent with existing project types and imports); ensure to import any typing names used (e.g., Optional, List, Dict) and adjust annotations to match the actual types used in the method bodies.
🧹 Nitpick comments (2)
src/odemis/acq/feature.py (1)
760-763: Log callback failures with traceback.Switch this warning to
logging.exception(...)so failures in per-feature cleanup hooks remain diagnosable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/feature.py` around lines 760 - 763, The except block around the per-feature cleanup hook currently uses logging.warning and drops the traceback; change the handler in the try/except around self._per_feature_callback() to call logging.exception(...) instead so the exception message plus full traceback from the per-feature cleanup hook (self._per_feature_callback) is logged for diagnosability.src/odemis/gui/cont/acquisition/cryo_acq.py (1)
432-436: Keep traceback when per-feature cleanup fails.Use
logging.exception(...)instead of interpolatingExceptioninto a warning string; otherwise, debugging intermittent cleanup failures is harder.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/acquisition/cryo_acq.py` around lines 432 - 436, The except block that currently calls logging.warning with the interpolated exception should instead call logging.exception to preserve the traceback; update the handler around self._tab._acquired_stream_controller._clear_all_feature_streams() so it uses logging.exception("Error clearing streams after feature acquisition") (or similar message) rather than logging.warning(f"...{e}") to ensure full stack trace is recorded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2445-2457: The loop over self.stream_controllers mutates that list
during iteration which can skip items; change it to iterate over a snapshot or
collect controllers to remove first: e.g., build a to_remove list while checking
isinstance(sc.stream, StaticSEMStream), membership in
self._tab_data_model.overviewStreams.value, calling
self._stream_bar.remove_stream_panel(sc.stream_panel) and
v.removeStream(sc.stream) as needed, then after the loop remove each sc from
self.stream_controllers (or reconstruct the list excluding those to remove) to
avoid in-loop mutation; use the existing symbols stream_controllers,
StaticSEMStream, _tab_data_model.overviewStreams,
_stream_bar.remove_stream_panel, and v.removeStream to locate the code.
---
Duplicate comments:
In `@src/odemis/gui/cont/stream_bar.py`:
- Line 2213: The background worker currently mutates shared GUI state by calling
load_feature_streams_from_disk which alters feature.streams.value inside
_load_feature_streams; instead, refactor so the background thread only reads and
returns a plain list of streams (e.g., implement _read_feature_streams or have
load_feature_streams_from_disk return List[StaticStream] without touching
feature.streams.value), then use a `@call_in_wx_main` method (e.g.,
_apply_loaded_feature_streams) to check _current_feature and membership in
_tab_data_model.main.features.value and safely extend feature.streams.value and
call _display_feature_streams on the GUI thread; apply the same pattern for the
other occurrence around lines 2289-2290.
- Line 2171: The method _on_current_feature_changes currently lacks type
annotations; update its signature to include a parameter type for feature (e.g.,
Optional[FeatureType] or the appropriate class) and an explicit return type
(likely -> None), and do the same for the other newly introduced/updated methods
referenced in the review (add parameter and return type hints consistent with
existing project types and imports); ensure to import any typing names used
(e.g., Optional, List, Dict) and adjust annotations to match the actual types
used in the method bodies.
In `@src/odemis/gui/model/stream_view.py`:
- Around line 606-609: The code unconditionally clears node.image.value which
can blank shared images; only drop frames for projection nodes owned by this
view. Change the unconditional assignment to a guarded clear: check that the
node is one of this view's projection-owned nodes (e.g. if node in
self._projection_nodes or if getattr(node, "projection_owner", None) is self)
before setting node.image.value = None so shared stream images used by other
views remain intact.
In `@src/odemis/gui/test/test_cryo_stream_cleanup.py`:
- Around line 20-169: Add explicit return type annotations "-> None" to the
unittest lifecycle and test methods so they satisfy typing rules: update setUp
and every test_* method (e.g., setUp,
test_clear_feature_streams_removes_from_model,
test_clear_feature_streams_clears_raw_data,
test_clear_all_feature_streams_except_preserves_current,
test_clear_all_feature_streams_clears_everything) to declare a None return type;
keep signatures and bodies unchanged aside from appending "-> None" to each def
line.
---
Nitpick comments:
In `@src/odemis/acq/feature.py`:
- Around line 760-763: The except block around the per-feature cleanup hook
currently uses logging.warning and drops the traceback; change the handler in
the try/except around self._per_feature_callback() to call
logging.exception(...) instead so the exception message plus full traceback from
the per-feature cleanup hook (self._per_feature_callback) is logged for
diagnosability.
In `@src/odemis/gui/cont/acquisition/cryo_acq.py`:
- Around line 432-436: The except block that currently calls logging.warning
with the interpolated exception should instead call logging.exception to
preserve the traceback; update the handler around
self._tab._acquired_stream_controller._clear_all_feature_streams() so it uses
logging.exception("Error clearing streams after feature acquisition") (or
similar message) rather than logging.warning(f"...{e}") to ensure full stack
trace is recorded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f11a019-f9c5-49a0-8ea1-8560cb313f1c
📒 Files selected for processing (8)
.github/workflows/unit_tests_cloud.ymlsrc/odemis/acq/acqmng.pysrc/odemis/acq/feature.pysrc/odemis/gui/cont/acquisition/cryo_acq.pysrc/odemis/gui/cont/stream_bar.pysrc/odemis/gui/cont/tabs/cryo_chamber_tab.pysrc/odemis/gui/model/stream_view.pysrc/odemis/gui/test/test_cryo_stream_cleanup.py
💤 Files with no reviewable changes (1)
- src/odemis/gui/cont/tabs/cryo_chamber_tab.py
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/unit_tests_cloud.yml
- src/odemis/acq/acqmng.py
tmoerkerken
force-pushed
the
memopt-stream-reloading
branch
from
9ac161d to
c27d4d5
Compare
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/gui/cont/stream_bar.py (1)
2471-2484:⚠️ Potential issue | 🟠 MajorDon’t mutate
self.stream_controllerswhile iterating it.Removing entries from the list being iterated can skip controllers and leave stale panels/streams behind.
Suggested patch
- for sc in self.stream_controllers: + for sc in self.stream_controllers.copy():🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/stream_bar.py` around lines 2471 - 2484, The loop mutates self.stream_controllers while iterating it which can skip items; change the logic to first collect controllers-to-remove (e.g., iterate over list(self.stream_controllers) or build a to_remove list) using the same guards (isinstance(sc.stream, StaticSEMStream) and sc.stream not in self._tab_data_model.overviewStreams.value), call self._stream_bar.remove_stream_panel(sc.stream_panel) and v.removeStream(sc.stream) for each collected controller, and only after the iteration remove each collected sc from self.stream_controllers (or use list(self.stream_controllers) to avoid mutating the iterated sequence). Ensure you still check hasattr(v, "removeStream") before calling and reference the same symbols: self.stream_controllers, StaticSEMStream, self._tab_data_model.overviewStreams, self._stream_bar.remove_stream_panel, v.removeStream, and sc.stream_panel.
♻️ Duplicate comments (4)
src/odemis/gui/cont/stream_bar.py (2)
2213-2222:⚠️ Potential issue | 🟠 MajorAvoid mutating feature/model stream collections from the loader thread.
_load_feature_streams_bginvokes_load_feature_streams, which callsload_feature_streams_from_disk(...)and mutatesfeature.streams.valuein a background thread. This can race with GUI-thread consumers of the same VAs.A safer pattern is: background thread only reads/parses data into local objects; main thread applies all model mutations.
Also applies to: 2293-2310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/stream_bar.py` around lines 2213 - 2222, The background loader _load_feature_streams_bg currently calls _load_feature_streams which uses load_feature_streams_from_disk and directly mutates feature.streams.value from a worker thread; change this so the background thread only reads/parses stream data into local structures (e.g., a list/dict) by calling load_feature_streams_from_disk, then schedule a switch to the main/UI thread to apply mutations to the data model (assign to feature.streams.value) — locate uses of _load_feature_streams_bg and _load_feature_streams to implement this handoff (e.g., via a thread-safe queue, main-thread executor, or GUI framework invoke/call_soon) and ensure no direct writes to feature.streams.value happen off the main thread.
2202-2202:⚠️ Potential issue | 🟡 MinorAdd type hints to the newly added method signatures.
Several new methods still miss parameter/return annotations (
_load_feature_streams_bg,_display_feature_streams,_load_feature_streams,clear_feature_streams), which violates the repository typing rule.As per coding guidelines
**/*.py: Always use type hints for function parameters and return types in Python code.Also applies to: 2242-2242, 2293-2293, 2321-2321
.github/workflows/unit_tests_cloud.yml (1)
216-220:⚠️ Potential issue | 🟡 MinorUse the repository’s no-hardware test invocation pattern in this CI step.
Please switch this step to the direct-file invocation with
TEST_NOHW=1so it matches the project convention for test execution in CI.Suggested update
- name: Run tests from odemis.gui.cont.stream_bar (stream cleanup) if: ${{ !cancelled() }} run: | export PYTHONPATH="$PWD/src:$PYTHONPATH" - python3 -m unittest src/odemis/gui/test/test_cryo_stream_cleanup.py --verbose + env TEST_NOHW=1 python3 src/odemis/gui/test/test_cryo_stream_cleanup.py TestCryoAcquiredStreamsControllerCleanupBased on learnings
Applies to **/test_*.py : Run tests using the template command: env TEST_NOHW=1 python3 src/odemis/.../name_of_the_test_file.py TestCaseClassName.test_method_name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/unit_tests_cloud.yml around lines 216 - 220, Update the CI step named "Run tests from odemis.gui.cont.stream_bar (stream cleanup)" to use the repository no-hardware test invocation pattern: run the test file directly with TEST_NOHW=1 in the environment rather than invoking unittest with -m; replace the current python3 -m unittest src/odemis/gui/test/test_cryo_stream_cleanup.py --verbose invocation with the direct-file pattern so the step exports/sets TEST_NOHW=1 and calls the test file (optionally specifying the specific TestCase.test_method if desired) to match the project's CI convention.src/odemis/gui/test/test_cryo_stream_cleanup.py (1)
20-20:⚠️ Potential issue | 🟡 MinorAdd
-> Noneannotations to test/lifecycle methods.These methods should be explicitly annotated to satisfy the project typing rule.
Suggested patch
- def setUp(self): + def setUp(self) -> None: @@ - def test_clear_feature_streams_removes_from_model(self): + def test_clear_feature_streams_removes_from_model(self) -> None: @@ - def test_clear_feature_streams_clears_raw_data(self): + def test_clear_feature_streams_clears_raw_data(self) -> None: @@ - def test_clear_all_feature_streams_except_preserves_current(self): + def test_clear_all_feature_streams_except_preserves_current(self) -> None: @@ - def test_clear_all_feature_streams_clears_everything(self): + def test_clear_all_feature_streams_clears_everything(self) -> None:As per coding guidelines
**/*.py: Always use type hints for function parameters and return types in Python code.Also applies to: 98-98, 121-121, 137-137, 155-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/test/test_cryo_stream_cleanup.py` at line 20, The test lifecycle methods lack explicit return type annotations; update each test/lifecycle method in test_cryo_stream_cleanup.py (e.g., setUp) to include -> None (for example change def setUp(self): to def setUp(self) -> None:), and do the same for other lifecycle or test helper methods flagged (those at the other locations mentioned) so all function definitions have explicit return type hints per the typing rule.
🧹 Nitpick comments (1)
src/odemis/acq/feature.py (1)
758-763: Preserve traceback when the per-feature callback fails.This path is good to keep non-fatal, but
logging.warning(...)hides stack context; uselogging.exception(...)(orexc_info=True) for actionable diagnostics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/feature.py` around lines 758 - 763, The except block that catches failures from the per-feature cleanup callback (the try/except around self._per_feature_callback) currently calls logging.warning and loses the traceback; change that to log the full exception context by using logging.exception("Error in per-feature callback") or logging.warning("Error in per-feature callback", exc_info=True) so the stacktrace is preserved 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 `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2242-2260: Asynchronous calls to _display_feature_streams can act
on a stale feature; add a guard that returns immediately if the feature passed
in is not the current feature (e.g., check feature is self.currentFeature or
compare against self._tab_data_model.main.currentFeature.value) before modifying
the UI, and add the same guard to the other async display path referenced around
the earlier occurrence (the other block that displays streams at the 2188–2200
site) so queued calls don't render a non-current feature.
---
Outside diff comments:
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2471-2484: The loop mutates self.stream_controllers while
iterating it which can skip items; change the logic to first collect
controllers-to-remove (e.g., iterate over list(self.stream_controllers) or build
a to_remove list) using the same guards (isinstance(sc.stream, StaticSEMStream)
and sc.stream not in self._tab_data_model.overviewStreams.value), call
self._stream_bar.remove_stream_panel(sc.stream_panel) and
v.removeStream(sc.stream) for each collected controller, and only after the
iteration remove each collected sc from self.stream_controllers (or use
list(self.stream_controllers) to avoid mutating the iterated sequence). Ensure
you still check hasattr(v, "removeStream") before calling and reference the same
symbols: self.stream_controllers, StaticSEMStream,
self._tab_data_model.overviewStreams, self._stream_bar.remove_stream_panel,
v.removeStream, and sc.stream_panel.
---
Duplicate comments:
In @.github/workflows/unit_tests_cloud.yml:
- Around line 216-220: Update the CI step named "Run tests from
odemis.gui.cont.stream_bar (stream cleanup)" to use the repository no-hardware
test invocation pattern: run the test file directly with TEST_NOHW=1 in the
environment rather than invoking unittest with -m; replace the current python3
-m unittest src/odemis/gui/test/test_cryo_stream_cleanup.py --verbose invocation
with the direct-file pattern so the step exports/sets TEST_NOHW=1 and calls the
test file (optionally specifying the specific TestCase.test_method if desired)
to match the project's CI convention.
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2213-2222: The background loader _load_feature_streams_bg
currently calls _load_feature_streams which uses load_feature_streams_from_disk
and directly mutates feature.streams.value from a worker thread; change this so
the background thread only reads/parses stream data into local structures (e.g.,
a list/dict) by calling load_feature_streams_from_disk, then schedule a switch
to the main/UI thread to apply mutations to the data model (assign to
feature.streams.value) — locate uses of _load_feature_streams_bg and
_load_feature_streams to implement this handoff (e.g., via a thread-safe queue,
main-thread executor, or GUI framework invoke/call_soon) and ensure no direct
writes to feature.streams.value happen off the main thread.
In `@src/odemis/gui/test/test_cryo_stream_cleanup.py`:
- Line 20: The test lifecycle methods lack explicit return type annotations;
update each test/lifecycle method in test_cryo_stream_cleanup.py (e.g., setUp)
to include -> None (for example change def setUp(self): to def setUp(self) ->
None:), and do the same for other lifecycle or test helper methods flagged
(those at the other locations mentioned) so all function definitions have
explicit return type hints per the typing rule.
---
Nitpick comments:
In `@src/odemis/acq/feature.py`:
- Around line 758-763: The except block that catches failures from the
per-feature cleanup callback (the try/except around self._per_feature_callback)
currently calls logging.warning and loses the traceback; change that to log the
full exception context by using logging.exception("Error in per-feature
callback") or logging.warning("Error in per-feature callback", exc_info=True) so
the stacktrace is preserved for diagnostics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 483d9b0e-5a26-40ad-bce8-c4be8e4c4d71
📒 Files selected for processing (8)
.github/workflows/unit_tests_cloud.ymlsrc/odemis/acq/acqmng.pysrc/odemis/acq/feature.pysrc/odemis/gui/cont/acquisition/cryo_acq.pysrc/odemis/gui/cont/stream_bar.pysrc/odemis/gui/cont/tabs/cryo_chamber_tab.pysrc/odemis/gui/model/stream_view.pysrc/odemis/gui/test/test_cryo_stream_cleanup.py
💤 Files with no reviewable changes (1)
- src/odemis/gui/cont/tabs/cryo_chamber_tab.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/odemis/gui/model/stream_view.py
- src/odemis/acq/acqmng.py
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/gui/cont/stream_bar.py (1)
2471-2483:⚠️ Potential issue | 🟠 MajorDo not mutate
self.stream_controllerswhile iterating it.On Line 2471, the loop iterates
self.stream_controllersand removes from it inside the loop. This can skip entries and leave stale stream controllers/panels behind.Suggested fix
- for sc in self.stream_controllers: + for sc in self.stream_controllers.copy(): if not isinstance(sc.stream, StaticSEMStream): logging.warning("Unexpected non static stream: %s", sc.stream) continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/stream_bar.py` around lines 2471 - 2483, The loop over self.stream_controllers mutates the list while iterating (removing controllers inside the for-loop), which can skip items; fix by iterating over a shallow copy or by collecting controllers to remove and performing removals after the loop: e.g., iterate "for sc in list(self.stream_controllers)" or build a to_remove list when you call _stream_bar.remove_stream_panel(sc.stream_panel) and v.removeStream(sc.stream), then after the loop call self.stream_controllers.remove(sc) for each sc in to_remove; ensure you still check isinstance(sc.stream, StaticSEMStream) and membership in _tab_data_model.overviewStreams.value before scheduling removal so behavior of StaticSEMStream, _stream_bar.remove_stream_panel, v.removeStream, and final removals matches original intent.
♻️ Duplicate comments (1)
src/odemis/gui/cont/stream_bar.py (1)
2202-2202:⚠️ Potential issue | 🟡 MinorAdd missing type hints on newly added method signatures.
Lines 2202, 2242, 2274, 2293, and 2321 introduce methods without full parameter/return annotations.
As per coding guidelines `**/*.py`: Always use type hints for function parameters and return types in Python code.Suggested fix
- def _load_feature_streams_bg(self, feature): + def _load_feature_streams_bg(self, feature: "CryoFeature") -> None: @@ - def _display_feature_streams(self, feature): + def _display_feature_streams(self, feature: Optional["CryoFeature"]) -> None: @@ - def _clear_all_feature_streams_except(self, current_feature) -> None: + def _clear_all_feature_streams_except(self, current_feature: Optional["CryoFeature"]) -> None: @@ - def _load_feature_streams(self, feature): + def _load_feature_streams(self, feature: "CryoFeature") -> None: @@ - def clear_feature_streams(self, feature): + def clear_feature_streams(self, feature: "CryoFeature") -> None:Also applies to: 2242-2242, 2274-2274, 2293-2293, 2321-2321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/stream_bar.py` at line 2202, The new method _load_feature_streams_bg and the other newly added methods in this file lack type annotations; update each function signature to include parameter and return type hints (e.g., specify concrete types where known or fall back to typing.Any/Optional[T], use List/Dict/Callable/Coroutine as appropriate) and add any required imports from typing at the top of the module; ensure asynchronous functions have correct Coroutine/asyncio.Task typings and synchronous functions use precise return types (or -> None) so every new method signature is fully annotated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2232-2238: The skipped-path currently only avoids display but
leaves loaded streams cached; update the branch in the try block (around
self._current_feature, _display_feature_streams, and feature_name) to also
release resources for the non-current feature by invoking a cleanup routine
(e.g., add or call a method named _cleanup_feature_streams or
_dispose_loaded_streams). That routine should remove the feature's entries from
any in-memory caches (e.g., self._loaded_streams or self._stream_cache),
cancel/stop any pending background tasks for that feature, and free related
buffers so stale loads do not persist until the next feature switch.
---
Outside diff comments:
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2471-2483: The loop over self.stream_controllers mutates the list
while iterating (removing controllers inside the for-loop), which can skip
items; fix by iterating over a shallow copy or by collecting controllers to
remove and performing removals after the loop: e.g., iterate "for sc in
list(self.stream_controllers)" or build a to_remove list when you call
_stream_bar.remove_stream_panel(sc.stream_panel) and v.removeStream(sc.stream),
then after the loop call self.stream_controllers.remove(sc) for each sc in
to_remove; ensure you still check isinstance(sc.stream, StaticSEMStream) and
membership in _tab_data_model.overviewStreams.value before scheduling removal so
behavior of StaticSEMStream, _stream_bar.remove_stream_panel, v.removeStream,
and final removals matches original intent.
---
Duplicate comments:
In `@src/odemis/gui/cont/stream_bar.py`:
- Line 2202: The new method _load_feature_streams_bg and the other newly added
methods in this file lack type annotations; update each function signature to
include parameter and return type hints (e.g., specify concrete types where
known or fall back to typing.Any/Optional[T], use List/Dict/Callable/Coroutine
as appropriate) and add any required imports from typing at the top of the
module; ensure asynchronous functions have correct Coroutine/asyncio.Task
typings and synchronous functions use precise return types (or -> None) so every
new method signature is fully annotated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c02f20fd-48c2-47dd-808c-6cf3669686d1
📒 Files selected for processing (8)
.github/workflows/unit_tests_cloud.ymlsrc/odemis/acq/acqmng.pysrc/odemis/acq/feature.pysrc/odemis/gui/cont/acquisition/cryo_acq.pysrc/odemis/gui/cont/stream_bar.pysrc/odemis/gui/cont/tabs/cryo_chamber_tab.pysrc/odemis/gui/model/stream_view.pysrc/odemis/gui/test/test_cryo_stream_cleanup.py
💤 Files with no reviewable changes (1)
- src/odemis/gui/cont/tabs/cryo_chamber_tab.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/odemis/acq/acqmng.py
- .github/workflows/unit_tests_cloud.yml
- src/odemis/gui/test/test_cryo_stream_cleanup.py
- src/odemis/gui/model/stream_view.py
tmoerkerken
force-pushed
the
memopt-stream-reloading
branch
from
c27d4d5 to
97a7e62
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odemis/gui/cont/stream_bar.py (1)
2474-2487:⚠️ Potential issue | 🟠 MajorDo not remove from
self.stream_controllerswhile iterating it.On Line 2474, iterating
self.stream_controllerswhile removing entries from it can skip controllers and leave stale streams/panels behind.Suggested fix
- for sc in self.stream_controllers: + for sc in self.stream_controllers.copy(): if not isinstance(sc.stream, StaticSEMStream): logging.warning("Unexpected non static stream: %s", sc.stream) continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/stream_bar.py` around lines 2474 - 2487, You're removing items from self.stream_controllers while iterating it, which can skip elements; change the loop to first collect controllers to remove (e.g., build a to_remove list of sc where isinstance(sc.stream, StaticSEMStream) and sc.stream not in self._tab_data_model.overviewStreams.value), then in a second loop call self._stream_bar.remove_stream_panel(sc.stream_panel), call v.removeStream(sc.stream) if hasattr(v, "removeStream"), and finally remove sc from self.stream_controllers; reference the existing symbols self.stream_controllers, StaticSEMStream, self._tab_data_model.overviewStreams.value, self._stream_bar.remove_stream_panel, v.removeStream, and sc.stream_panel when implementing the two-phase removal.
♻️ Duplicate comments (5)
src/odemis/gui/cont/acquisition/cryo_acq.py (1)
432-436:⚠️ Potential issue | 🟡 MinorUse traceback logging in per-feature cleanup errors.
On Line 436,
logging.warning(f"...{e}")hides stack context. Keep traceback so cleanup failures are diagnosable.Suggested fix
try: # Clear all feature streams immediately to free memory for the next acquisition self._tab._acquired_stream_controller._clear_all_feature_streams() - except Exception as e: - logging.warning(f"Error clearing streams after feature acquisition: {e}") + except Exception: + logging.exception("Error clearing streams after feature acquisition")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/acquisition/cryo_acq.py` around lines 432 - 436, The except block around self._tab._acquired_stream_controller._clear_all_feature_streams() currently logs only the exception message and loses the traceback; replace the logging call to include stack context (e.g., use logging.exception("Error clearing streams after feature acquisition") or logging.warning("...", exc_info=True)) so the full traceback is recorded for diagnostics while keeping the same descriptive text and the same try/except structure.src/odemis/gui/model/stream_view.py (1)
606-609:⚠️ Potential issue | 🟠 MajorAvoid clearing shared stream images on per-view removal.
On Line 609,
node.image.value = Noneis unconditional. Ifnodeis a shared stream (not a per-view projection), this can blank the image for other views that still use it.Suggested fix
if hasattr(node, "image"): node.image.unsubscribe(self._onNewImage) # Drop the rendered frame immediately so the DataArray # can be GC'd without waiting for the projection thread # to wake up and release its reference. - node.image.value = None + if isinstance(node, DataProjection): + node.image.value = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/model/stream_view.py` around lines 606 - 609, The unconditional clear of the image (node.image.value = None) can blank shared streams used by other views; change the removal logic in the stream view cleanup so you only set node.image.value = None for nodes that are per-view projections (e.g., check a property like node.is_projection, node.projection_owner == self, or node.per_view flag) and skip clearing for shared stream nodes; locate the code around node.image.value = None in the stream removal/cleanup function and gate that assignment behind the per-view/projection check so shared streams remain intact for other views.src/odemis/gui/cont/stream_bar.py (2)
2245-2258:⚠️ Potential issue | 🟠 MajorGuard async display against stale feature switches.
_display_feature_streamsis queued asynchronously (@call_in_wx_main). Without checking current selection, an older queued call can render a non-current feature.Suggested fix
def _display_feature_streams(self, feature): """ Display the feature's streams in the acquired view (must be called on main thread). Adds streams to the tab model and displays them. :param feature: (CryoFeature or None) the feature whose streams to display """ + if feature is not self._current_feature: + logging.debug("Skipping stale display request for non-current feature") + return + if not feature: return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/stream_bar.py` around lines 2245 - 2258, The queued _display_feature_streams call can act on a stale feature; after the existing existence check (feature in self._tab_data_model.main.features.value) also guard against selection drift by comparing the feature argument to the currently selected feature and aborting if they differ; e.g., retrieve the current selection from the tab model (self._tab_data_model.main.selected_feature or the equivalent current selection property) and if feature is not the same object/value, log a debug message and return so only the currently selected feature is rendered.
2217-2241:⚠️ Potential issue | 🟠 MajorAvoid mutating feature stream state from the loader thread.
_load_feature_streams_bgruns in a background thread, but Line 2221 (_load_feature_streams(...)) and Line 2240 (feature.streams.value.clear()) mutate shared feature state off the GUI thread. That can race with main-thread cleanup/display logic.Suggested direction
- self._load_feature_streams(feature) + loaded_streams = self._read_feature_streams_from_disk(feature) + wx.CallAfter(self._apply_loaded_feature_streams, feature, loaded_streams) ... - if feature and feature.streams.value: - feature.streams.value.clear() + if feature: + wx.CallAfter(self._discard_feature_streams, feature)Use the worker thread only for I/O/parsing; apply
feature.streamsmutations on the main thread.Also applies to: 2296-2313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/stream_bar.py` around lines 2217 - 2241, The background loader (_load_feature_streams_bg) is mutating shared state: it calls _load_feature_streams(...) and clears feature.streams.value in the worker thread; move all mutations to the main/UI thread. Change _load_feature_streams to return the loaded stream data (do I/O/parsing only in the worker) and then schedule a main-thread callback (via the GUI dispatch/idle mechanism used by the app) that calls _display_feature_streams(feature, streams) or assigns feature.streams.value = streams; also remove any direct feature.streams.value.clear() calls from the background path and perform clearing/assignment only inside the main-thread scheduled callback so all updates to feature.streams happen on the GUI thread.src/odemis/gui/test/test_cryo_stream_cleanup.py (1)
20-20:⚠️ Potential issue | 🟡 MinorAdd explicit
-> Nonereturn annotations to test methods.Line 20 and Lines 98, 121, 137, and 155 should include explicit
-> Nonereturn types to satisfy the project typing rule.Suggested patch
- def setUp(self): + def setUp(self) -> None: @@ - def test_clear_feature_streams_removes_from_model(self): + def test_clear_feature_streams_removes_from_model(self) -> None: @@ - def test_clear_feature_streams_clears_raw_data(self): + def test_clear_feature_streams_clears_raw_data(self) -> None: @@ - def test_clear_all_feature_streams_except_preserves_current(self): + def test_clear_all_feature_streams_except_preserves_current(self) -> None: @@ - def test_clear_all_feature_streams_clears_everything(self): + def test_clear_all_feature_streams_clears_everything(self) -> None:As per coding guidelines
**/*.py: Always use type hints for function parameters and return types in Python code.Also applies to: 98-98, 121-121, 137-137, 155-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/test/test_cryo_stream_cleanup.py` at line 20, Add explicit return type annotations "-> None" to the test methods that currently lack them: update the setUp method (function setUp) and the other test methods referenced in the review (the test functions at lines 98, 121, 137, and 155) so each function signature includes "-> None". Ensure you only modify the function definitions (e.g., "def setUp(self) -> None:") and not the bodies, to satisfy the project's typing rule for test methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/acq/feature.py`:
- Around line 758-763: The current except block around
self._per_feature_callback swallows the traceback by logging only the exception
string; change the handler to preserve stack information by using
logging.exception or logging.warning(..., exc_info=True) when catching the
Exception from self._per_feature_callback so the full traceback is recorded for
debugging.
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2175-2324: Add full type hints to the changed methods: update
_on_current_feature_changes to accept feature: Optional[CryoFeature] and return
None; _load_feature_streams_bg(feature: CryoFeature) -> None;
_display_feature_streams(feature: Optional[CryoFeature]) -> None;
_load_feature_streams(feature: CryoFeature) -> None; and
clear_feature_streams(feature: CryoFeature) -> None. Import typing.Optional (and
any CryoFeature symbol if not already imported) at the top and ensure signatures
and docstrings reflect the types; no behavioral changes required beyond
annotations.
---
Outside diff comments:
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2474-2487: You're removing items from self.stream_controllers
while iterating it, which can skip elements; change the loop to first collect
controllers to remove (e.g., build a to_remove list of sc where
isinstance(sc.stream, StaticSEMStream) and sc.stream not in
self._tab_data_model.overviewStreams.value), then in a second loop call
self._stream_bar.remove_stream_panel(sc.stream_panel), call
v.removeStream(sc.stream) if hasattr(v, "removeStream"), and finally remove sc
from self.stream_controllers; reference the existing symbols
self.stream_controllers, StaticSEMStream,
self._tab_data_model.overviewStreams.value,
self._stream_bar.remove_stream_panel, v.removeStream, and sc.stream_panel when
implementing the two-phase removal.
---
Duplicate comments:
In `@src/odemis/gui/cont/acquisition/cryo_acq.py`:
- Around line 432-436: The except block around
self._tab._acquired_stream_controller._clear_all_feature_streams() currently
logs only the exception message and loses the traceback; replace the logging
call to include stack context (e.g., use logging.exception("Error clearing
streams after feature acquisition") or logging.warning("...", exc_info=True)) so
the full traceback is recorded for diagnostics while keeping the same
descriptive text and the same try/except structure.
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2245-2258: The queued _display_feature_streams call can act on a
stale feature; after the existing existence check (feature in
self._tab_data_model.main.features.value) also guard against selection drift by
comparing the feature argument to the currently selected feature and aborting if
they differ; e.g., retrieve the current selection from the tab model
(self._tab_data_model.main.selected_feature or the equivalent current selection
property) and if feature is not the same object/value, log a debug message and
return so only the currently selected feature is rendered.
- Around line 2217-2241: The background loader (_load_feature_streams_bg) is
mutating shared state: it calls _load_feature_streams(...) and clears
feature.streams.value in the worker thread; move all mutations to the main/UI
thread. Change _load_feature_streams to return the loaded stream data (do
I/O/parsing only in the worker) and then schedule a main-thread callback (via
the GUI dispatch/idle mechanism used by the app) that calls
_display_feature_streams(feature, streams) or assigns feature.streams.value =
streams; also remove any direct feature.streams.value.clear() calls from the
background path and perform clearing/assignment only inside the main-thread
scheduled callback so all updates to feature.streams happen on the GUI thread.
In `@src/odemis/gui/model/stream_view.py`:
- Around line 606-609: The unconditional clear of the image (node.image.value =
None) can blank shared streams used by other views; change the removal logic in
the stream view cleanup so you only set node.image.value = None for nodes that
are per-view projections (e.g., check a property like node.is_projection,
node.projection_owner == self, or node.per_view flag) and skip clearing for
shared stream nodes; locate the code around node.image.value = None in the
stream removal/cleanup function and gate that assignment behind the
per-view/projection check so shared streams remain intact for other views.
In `@src/odemis/gui/test/test_cryo_stream_cleanup.py`:
- Line 20: Add explicit return type annotations "-> None" to the test methods
that currently lack them: update the setUp method (function setUp) and the other
test methods referenced in the review (the test functions at lines 98, 121, 137,
and 155) so each function signature includes "-> None". Ensure you only modify
the function definitions (e.g., "def setUp(self) -> None:") and not the bodies,
to satisfy the project's typing rule for test methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d642d4c8-02f7-472e-b79c-eaa64ab601bd
📒 Files selected for processing (8)
.github/workflows/unit_tests_cloud.ymlsrc/odemis/acq/acqmng.pysrc/odemis/acq/feature.pysrc/odemis/gui/cont/acquisition/cryo_acq.pysrc/odemis/gui/cont/stream_bar.pysrc/odemis/gui/cont/tabs/cryo_chamber_tab.pysrc/odemis/gui/model/stream_view.pysrc/odemis/gui/test/test_cryo_stream_cleanup.py
💤 Files with no reviewable changes (1)
- src/odemis/gui/cont/tabs/cryo_chamber_tab.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/unit_tests_cloud.yml
pieleric
requested changes
Mar 6, 2026
tmoerkerken
force-pushed
the
memopt-stream-reloading
branch
from
97a7e62 to
5de2d57
Compare
Contributor
Author
|
I reverted the |
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/gui/cont/stream_bar.py (1)
2474-2486:⚠️ Potential issue | 🟡 MinorIterate over a copy when removing stream controllers.
Line 2474 loops over
self.stream_controllerswhile removing entries from that same list. If more than one feature stream panel is present, removals can shift the remaining items and leave stale controllers behind.Suggested fix
- for sc in self.stream_controllers: + for sc in self.stream_controllers.copy(): if not isinstance(sc.stream, StaticSEMStream): logging.warning("Unexpected non static stream: %s", sc.stream) continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/stream_bar.py` around lines 2474 - 2486, The loop in stream_bar.py iterates self.stream_controllers while removing items which can skip or leave stale controllers; change the iteration to use a snapshot copy (e.g., iterate over list(self.stream_controllers) or self.stream_controllers[:] ) so removals inside the loop (calls to self._stream_bar.remove_stream_panel, v.removeStream, and self.stream_controllers.remove(sc)) don't affect the active iterator; keep the existing checks for StaticSEMStream, overviewStreams, and hasattr(v, "removeStream") and still guard removal with "if sc in self.stream_controllers" before calling remove.
♻️ Duplicate comments (6)
src/odemis/acq/feature.py (1)
760-765:⚠️ Potential issue | 🟡 MinorPreserve the per-feature callback traceback.
Line 765 only logs the exception message, so cleanup failures lose the stack trace and become much harder to debug.
Suggested fix
if self._per_feature_callback: try: self._per_feature_callback() - except Exception as e: - logging.warning(f"Error in per-feature callback: {e}") + except Exception: + logging.exception("Error in per-feature callback")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/feature.py` around lines 760 - 765, The per-feature callback exception currently logs only the message and loses the traceback; update the exception handling around self._per_feature_callback() (the try/except block in the same method) to log the full traceback by using logging.exception(...) or logging.warning(..., exc_info=True) inside the except block so the stack trace is preserved when cleanup fails.src/odemis/gui/cont/stream_bar.py (3)
2202-2242:⚠️ Potential issue | 🟠 MajorKeep load/discard decisions on the wx thread, not in
finally.
_load_feature_streams_bg()still readsself._current_featureand mutatesfeature.streams.valuefrom the worker thread. Because the display/cleanup branch lives infinally, it also runs after theexceptpath returns, so a failed or partial load can still be displayed or cleared off-thread.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/stream_bar.py` around lines 2202 - 2242, The background loader _load_feature_streams_bg is reading self._current_feature and mutating feature.streams.value and calling _display_feature_streams from a worker thread (including inside the finally), which can cause off-thread UI access and racey cleanup; move all decisions and UI/display/cleanup logic onto the wx/main thread: ensure _load_feature_streams_bg only performs pure background work and returns the loaded stream data (or raises), remove any access to self._current_feature and avoid mutating feature.streams.value off-thread, and schedule a main-thread callback (via wx.CallAfter or the existing mechanism) that checks self._current_feature, verifies feature is still in self._tab_data_model.main.features.value, assigns feature.streams.value or clears it there, and calls _display_feature_streams(feature) so all display/cleanup happens on the wx thread.
2447-2447:⚠️ Potential issue | 🟡 MinorFinish typing the FIB controller hooks as well.
The updated FIB-acquired controller methods are still missing explicit parameter / return annotations. As per coding guidelines
**/*.py: Always use type hints for function parameters and return types in Python code.Also applies to: 2467-2467
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/stream_bar.py` at line 2447, The method _on_current_feature_changes is missing type annotations: add a concrete type hint for the feature parameter (e.g., Feature or Optional[Feature] as appropriate) and annotate the return type as None; likewise, finish typing the other FIB-acquired controller hooks mentioned in the review (the method around line 2467) by adding explicit parameter and return type hints consistent with your controller/feature types. Ensure you import or reference the correct types (e.g., Feature, Controller, Optional) and update the function signatures accordingly.
2175-2175:⚠️ Potential issue | 🟡 MinorAdd annotations to the new feature-stream helper methods.
These changed methods still take untyped
feature/current_featureparameters and some omit-> None, so this block still violates the repo typing rule. As per coding guidelines**/*.py: Always use type hints for function parameters and return types in Python code.Also applies to: 2202-2202, 2277-2277, 2296-2296, 2324-2324
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/stream_bar.py` at line 2175, The helper methods for the feature stream (including _on_current_feature_changes and the other new helper methods at the other locations) lack type hints; update each method signature to annotate the feature/current_feature parameter with the proper type (e.g., Feature or Optional[Feature] from your domain model, or typing.Any if the concrete type isn’t yet imported) and add an explicit return annotation -> None; ensure to import Optional/Any from typing or the Feature class where needed and update all signatures that currently take untyped feature/current_feature parameters to use these annotations.src/odemis/gui/test/test_cryo_stream_cleanup.py (1)
20-20:⚠️ Potential issue | 🟡 MinorAdd
-> NonetosetUpand the new tests.The new unittest methods still omit return annotations, so this file does not satisfy the project's Python typing rule. As per coding guidelines
**/*.py: Always use type hints for function parameters and return types in Python code.Also applies to: 98-98, 121-121, 137-137, 155-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/test/test_cryo_stream_cleanup.py` at line 20, The unittests in this file are missing return type annotations; add explicit "-> None" return type annotations to the setUp method (setUp) and to every unittest method (all functions named test_*) so they comply with the project's typing rule; update each function signature to include the return annotation (e.g., def setUp(self) -> None: and def test_xyz(self) -> None:) and run the type checker to confirm no other missing return annotations remain.src/odemis/gui/cont/acquisition/cryo_acq.py (1)
432-436:⚠️ Potential issue | 🟡 MinorKeep the cleanup traceback here too.
Line 436 only logs the exception string. If per-feature cleanup fails, this strips the stack trace from exactly the new lifecycle path this PR is adding.
Suggested fix
try: # Clear all feature streams immediately to free memory for the next acquisition self._tab._acquired_stream_controller._clear_all_feature_streams() - except Exception as e: - logging.warning(f"Error clearing streams after feature acquisition: {e}") + except Exception: + logging.exception("Error clearing streams after feature acquisition")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/acquisition/cryo_acq.py` around lines 432 - 436, The except block around the call to self._tab._acquired_stream_controller._clear_all_feature_streams() should log the full traceback instead of only the exception string; change the handler to include the stack (for example, use logging.exception(...) or append traceback.format_exc()) so the cleanup traceback from the new lifecycle path is preserved when _clear_all_feature_streams() fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/acq/feature.py`:
- Around line 373-381: The code collecting stream_filenames can produce
duplicates because overlapping glob patterns (built from glob_path and
formats_flat) match the same file multiple times; before iterating and calling
open_acquisition/data_to_static_streams and extending feature.streams.value,
deduplicate the filename list while preserving deterministic order (e.g., use an
ordered-unique approach on stream_filenames or convert to a
dict.fromkeys/OrderedDict keys) so each file is opened and converted exactly
once.
---
Outside diff comments:
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2474-2486: The loop in stream_bar.py iterates
self.stream_controllers while removing items which can skip or leave stale
controllers; change the iteration to use a snapshot copy (e.g., iterate over
list(self.stream_controllers) or self.stream_controllers[:] ) so removals inside
the loop (calls to self._stream_bar.remove_stream_panel, v.removeStream, and
self.stream_controllers.remove(sc)) don't affect the active iterator; keep the
existing checks for StaticSEMStream, overviewStreams, and hasattr(v,
"removeStream") and still guard removal with "if sc in self.stream_controllers"
before calling remove.
---
Duplicate comments:
In `@src/odemis/acq/feature.py`:
- Around line 760-765: The per-feature callback exception currently logs only
the message and loses the traceback; update the exception handling around
self._per_feature_callback() (the try/except block in the same method) to log
the full traceback by using logging.exception(...) or logging.warning(...,
exc_info=True) inside the except block so the stack trace is preserved when
cleanup fails.
In `@src/odemis/gui/cont/acquisition/cryo_acq.py`:
- Around line 432-436: The except block around the call to
self._tab._acquired_stream_controller._clear_all_feature_streams() should log
the full traceback instead of only the exception string; change the handler to
include the stack (for example, use logging.exception(...) or append
traceback.format_exc()) so the cleanup traceback from the new lifecycle path is
preserved when _clear_all_feature_streams() fails.
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2202-2242: The background loader _load_feature_streams_bg is
reading self._current_feature and mutating feature.streams.value and calling
_display_feature_streams from a worker thread (including inside the finally),
which can cause off-thread UI access and racey cleanup; move all decisions and
UI/display/cleanup logic onto the wx/main thread: ensure
_load_feature_streams_bg only performs pure background work and returns the
loaded stream data (or raises), remove any access to self._current_feature and
avoid mutating feature.streams.value off-thread, and schedule a main-thread
callback (via wx.CallAfter or the existing mechanism) that checks
self._current_feature, verifies feature is still in
self._tab_data_model.main.features.value, assigns feature.streams.value or
clears it there, and calls _display_feature_streams(feature) so all
display/cleanup happens on the wx thread.
- Line 2447: The method _on_current_feature_changes is missing type annotations:
add a concrete type hint for the feature parameter (e.g., Feature or
Optional[Feature] as appropriate) and annotate the return type as None;
likewise, finish typing the other FIB-acquired controller hooks mentioned in the
review (the method around line 2467) by adding explicit parameter and return
type hints consistent with your controller/feature types. Ensure you import or
reference the correct types (e.g., Feature, Controller, Optional) and update the
function signatures accordingly.
- Line 2175: The helper methods for the feature stream (including
_on_current_feature_changes and the other new helper methods at the other
locations) lack type hints; update each method signature to annotate the
feature/current_feature parameter with the proper type (e.g., Feature or
Optional[Feature] from your domain model, or typing.Any if the concrete type
isn’t yet imported) and add an explicit return annotation -> None; ensure to
import Optional/Any from typing or the Feature class where needed and update all
signatures that currently take untyped feature/current_feature parameters to use
these annotations.
In `@src/odemis/gui/test/test_cryo_stream_cleanup.py`:
- Line 20: The unittests in this file are missing return type annotations; add
explicit "-> None" return type annotations to the setUp method (setUp) and to
every unittest method (all functions named test_*) so they comply with the
project's typing rule; update each function signature to include the return
annotation (e.g., def setUp(self) -> None: and def test_xyz(self) -> None:) and
run the type checker to confirm no other missing return annotations remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5dc026ed-3fef-4f3a-8c41-55bea666b810
📒 Files selected for processing (6)
.github/workflows/unit_tests_cloud.ymlsrc/odemis/acq/feature.pysrc/odemis/gui/cont/acquisition/cryo_acq.pysrc/odemis/gui/cont/stream_bar.pysrc/odemis/gui/cont/tabs/cryo_chamber_tab.pysrc/odemis/gui/test/test_cryo_stream_cleanup.py
💤 Files with no reviewable changes (1)
- src/odemis/gui/cont/tabs/cryo_chamber_tab.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/unit_tests_cloud.yml
pieleric
reviewed
Mar 10, 2026
pieleric
requested changes
Mar 10, 2026
…d async loading - Lazy-load streams from disk - Per-feature cleanup during batch acquisition - Async background loading - Fixed race condition in GUI model updates - Tests added to CI
tmoerkerken
force-pushed
the
memopt-stream-reloading
branch
from
5de2d57 to
99ae654
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/odemis/gui/test/test_cryo_stream_cleanup.py (1)
20-20:⚠️ Potential issue | 🟡 MinorAdd
-> NonetosetUpand the newtest_*methods.The unittest hooks in this file still miss explicit return annotations.
Based on learnings
Applies to **/*.py : Always use type hints for function parameters and return types in Python code.Also applies to: 98-98, 121-121, 137-137, 155-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/test/test_cryo_stream_cleanup.py` at line 20, Update the unittest methods to include explicit return type annotations: add "-> None" to the setUp method and to each new test_* method in this module (e.g., setUp, test_* functions referenced in this file) so all unittest hooks and tests declare their return type; modify the function signatures accordingly (e.g., "def setUp(self) -> None:" and "def test_xxx(self) -> None:") to satisfy the project's type-hinting rule.src/odemis/gui/cont/stream_bar.py (2)
2188-2196:⚠️ Potential issue | 🟠 MajorKeep the lazy-load worker read-only and dedupe in-flight loads.
_load_feature_streams_bg()still fills and clearsfeature.streams.valuefrom the background thread, and_on_current_feature_changes()can spawn a second loader for the same feature while the first one is still running because the list stays empty until I/O completes. A quick A→B→A switch can therefore race into duplicate or stale streams. Parse in the worker, but apply/discard the result once on the wx thread behind a per-feature request token.Also applies to: 2201-2240, 2294-2310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/stream_bar.py` around lines 2188 - 2196, The background loader _load_feature_streams_bg must not mutate feature.streams.value directly or allow concurrent loads for the same feature: change the worker to perform only read-and-parse I/O and produce a result object, generate a per-feature request token/ID when spawning the worker in _on_current_feature_changes, and pass that token to the worker; when the worker finishes, post the result back to the wx main thread and only apply it (set feature.streams.value or discard) if the token still matches the feature’s current token. Also ensure spawning logic in _on_current_feature_changes checks and reuses/inhibits new threads while a valid token exists to dedupe in-flight loads for the same feature.
2175-2175:⚠️ Potential issue | 🟡 MinorFinish annotating the new stream-switching helpers.
_on_current_feature_changes,_load_feature_streams_bg,_clear_all_feature_streams_except,_load_feature_streams, and the twoclear_feature_streamsoverloads still have untyped parameters and/or missing-> None.As per coding guidelines
**/*.py: Always use type hints for function parameters and return types in Python code.Also applies to: 2201-2201, 2275-2275, 2294-2294, 2322-2322, 2445-2445, 2465-2465
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/stream_bar.py` at line 2175, Annotate the listed helper functions with explicit type hints and return types (-> None): add parameter and return type annotations for _on_current_feature_changes(feature), _load_feature_streams_bg(...), _clear_all_feature_streams_except(...), _load_feature_streams(...), and both clear_feature_streams overloads; use the concrete types already used in this module (e.g., Feature, Stream or Iterable[Stream], Optional[Feature] or Optional[...] where applicable) and typing.Overload/@overload signatures for the two clear_feature_streams variants, and ensure all functions have -> None as the return type..github/workflows/unit_tests_cloud.yml (1)
216-220:⚠️ Potential issue | 🟡 MinorRun this cleanup suite through the repo's no-hardware test entrypoint.
This step still uses
python -m unittestand never setsTEST_NOHW=1, so it doesn't match the repository's CI-safe invocation for GUI tests.Suggested update
- python3 -m unittest src/odemis/gui/test/test_cryo_stream_cleanup.py --verbose + env TEST_NOHW=1 python3 src/odemis/gui/test/test_cryo_stream_cleanup.py TestCryoAcquiredStreamsControllerCleanupBased on learnings
Applies to **/test_*.py : Run tests using the template command: env TEST_NOHW=1 python3 src/odemis/.../name_of_the_test_file.py TestCaseClassName.test_method_name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/unit_tests_cloud.yml around lines 216 - 220, The current step runs the cleanup suite with "python3 -m unittest" and doesn't set TEST_NOHW, so change the command to use the repo's no-hardware test entrypoint style: run the test file directly with TEST_NOHW=1 and target the specific test case, e.g. replace the "python3 -m unittest src/odemis/gui/test/test_cryo_stream_cleanup.py --verbose" invocation with "env TEST_NOHW=1 python3 src/odemis/gui/test/test_cryo_stream_cleanup.py TestCaseClassName.test_method_name" (adjust TestCaseClassName.test_method_name to the actual test case) so the GUI cleanup runs under the CI-safe no-hardware environment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/gui/cont/acquisition/cryo_acq.py`:
- Around line 424-436: The _on_per_feature_acquired callback currently queues
_tab._acquired_stream_controller._clear_all_feature_streams() onto the wx main
thread via `@call_in_wx_main` but does not wait, allowing the batch worker to
proceed early; change it so the worker blocks until the GUI cleanup finishes:
create a threading.Event (or concurrent.futures.Future) inside
_on_per_feature_acquired, schedule a wrapper on the wx thread that calls
_tab._acquired_stream_controller._clear_all_feature_streams() and then sets the
Event/fulfills the Future, and then call Event.wait() (or Future.result()) in
_on_per_feature_acquired so the method only returns after
_clear_all_feature_streams completes; keep references to
_on_per_feature_acquired and
_tab._acquired_stream_controller._clear_all_feature_streams in the change to
locate code.
---
Duplicate comments:
In @.github/workflows/unit_tests_cloud.yml:
- Around line 216-220: The current step runs the cleanup suite with "python3 -m
unittest" and doesn't set TEST_NOHW, so change the command to use the repo's
no-hardware test entrypoint style: run the test file directly with TEST_NOHW=1
and target the specific test case, e.g. replace the "python3 -m unittest
src/odemis/gui/test/test_cryo_stream_cleanup.py --verbose" invocation with "env
TEST_NOHW=1 python3 src/odemis/gui/test/test_cryo_stream_cleanup.py
TestCaseClassName.test_method_name" (adjust TestCaseClassName.test_method_name
to the actual test case) so the GUI cleanup runs under the CI-safe no-hardware
environment.
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2188-2196: The background loader _load_feature_streams_bg must not
mutate feature.streams.value directly or allow concurrent loads for the same
feature: change the worker to perform only read-and-parse I/O and produce a
result object, generate a per-feature request token/ID when spawning the worker
in _on_current_feature_changes, and pass that token to the worker; when the
worker finishes, post the result back to the wx main thread and only apply it
(set feature.streams.value or discard) if the token still matches the feature’s
current token. Also ensure spawning logic in _on_current_feature_changes checks
and reuses/inhibits new threads while a valid token exists to dedupe in-flight
loads for the same feature.
- Line 2175: Annotate the listed helper functions with explicit type hints and
return types (-> None): add parameter and return type annotations for
_on_current_feature_changes(feature), _load_feature_streams_bg(...),
_clear_all_feature_streams_except(...), _load_feature_streams(...), and both
clear_feature_streams overloads; use the concrete types already used in this
module (e.g., Feature, Stream or Iterable[Stream], Optional[Feature] or
Optional[...] where applicable) and typing.Overload/@overload signatures for the
two clear_feature_streams variants, and ensure all functions have -> None as the
return type.
In `@src/odemis/gui/test/test_cryo_stream_cleanup.py`:
- Line 20: Update the unittest methods to include explicit return type
annotations: add "-> None" to the setUp method and to each new test_* method in
this module (e.g., setUp, test_* functions referenced in this file) so all
unittest hooks and tests declare their return type; modify the function
signatures accordingly (e.g., "def setUp(self) -> None:" and "def test_xxx(self)
-> None:") to satisfy the project's type-hinting rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7813a99f-57ea-48a7-b62c-43f549e990ea
📒 Files selected for processing (6)
.github/workflows/unit_tests_cloud.ymlsrc/odemis/acq/feature.pysrc/odemis/gui/cont/acquisition/cryo_acq.pysrc/odemis/gui/cont/stream_bar.pysrc/odemis/gui/cont/tabs/cryo_chamber_tab.pysrc/odemis/gui/test/test_cryo_stream_cleanup.py
💤 Files with no reviewable changes (1)
- src/odemis/gui/cont/tabs/cryo_chamber_tab.py
tepals
approved these changes
Mar 10, 2026
pieleric
approved these changes
Mar 10, 2026
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
3 participants
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.
Users reported crashes for projects with a large amount of z-stacks. It was identified that these problems were memory related. After getting to know the user workflow, we determined that there is no need of displaying z-stacks of multiple features at the same time. This PR optimizes memory usage by ensuring only the z-stack data for the current feature is retained in memory.
Implements
Notes
gc.collect()call. While counterintuitive for memory optimization, I found the call does not improve memory, but it costs CPU use. Combined with our lazy-loading and per-feature cleanup, natural garbage collection is sufficient. No more silent crashes from race conditions (at least I hope 😅🤞).Tests
Verify successful clean-up of features. Independent of backend (CI compatible)
Manual verification
Switching back from
masterto this feature branch we observe that on loading a 20 feature, 15 z-stack per feature project, we see ~30gb vs ~600mb that is being kept in memory. Note that my system has 64gb of memory, but a production system with 32gb would definitely crash in these circumstances.Project loading (before changes)
Project loading (after changes)
I'm aware that we still have some persistent memory increase after switching, but that is out of scope (created a follow up issue).