[DO NOT MERGE][MSD-378][feat] Project state for cryo workflow#3402
Closed
tmoerkerken wants to merge 1 commit intodelmic:masterfrom
Closed
[DO NOT MERGE][MSD-378][feat] Project state for cryo workflow#3402tmoerkerken wants to merge 1 commit intodelmic:masterfrom
tmoerkerken wants to merge 1 commit intodelmic:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds persistent per-feature IDs and in-memory stream_records to CryoFeature with APIs to manage them. Moves feature persistence into a versioned project_state.json (FEATURES_STATE_VERSION = 2), adds migration helpers to discover legacy stream records, and supports loading/saving feature stream origins. Introduces per-feature acquisition filename and storage directory helpers, attaches per-stream origin metadata during export, and updates acquisition and multiple GUI controllers to attach per-stream persistence hooks, honor deleted streams, and rename per-feature storage when features are renamed. Sequence Diagram(s)sequenceDiagram
participant UI as UI Controller
participant Feature as CryoFeature
participant State as Project State
participant Disk as Disk I/O
UI->>Feature: read_features(project_dir)
Feature->>State: read_project_state(project_dir)
alt state contains features
State-->>Feature: return features with id + stream_records
else legacy features.json present
Feature->>Disk: discover legacy feature files
Disk-->>Feature: return legacy files
Feature->>Feature: _discover_legacy_stream_records -> stream_records
end
Feature-->>UI: return List[CryoFeature]
sequenceDiagram
participant Acq as Acquisition Controller
participant Export as Export Flow
participant Feature as CryoFeature
participant State as Project State
participant Disk as Disk I/O
Acq->>Acq: _on_acquire() set _active_acq_feature
Acq->>Export: _export_data(data)
Export->>Feature: create_feature_acquisition_filename(feature, base)
Export->>Disk: save files to feature-specific directory
Disk-->>Export: saved filenames
Export->>Feature: add_stream_record(relative_filename, stream_count)
Export->>State: save_features(project_dir, features)
State->>Disk: write project_state.json
Disk-->>State: persisted
sequenceDiagram
participant UI as Stream UI
participant Controller as Streams Controller
participant Stream as Stream Object
participant State as Project State
participant Disk as Disk I/O
UI->>Controller: showFeatureStream(stream)
Controller->>Controller: _attach_stream_state_hooks(stream)
Controller->>Stream: subscribe to origin/tint changes
Stream->>Controller: user removes stream
Controller->>State: mark_overview_stream_deleted or update feature.stream_records
State->>Disk: write project_state.json
Disk-->>State: persisted
Controller-->>UI: remove stream panel
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
src/odemis/gui/comp/stream_bar.py (1)
173-178: Add type hint for the callback parameter.Per coding guidelines, function parameters should have type hints. The callback should be typed as
Optional[Callable[[Stream], None]]or similar.Suggested type hint
+from typing import Callable, Optional + - def set_on_user_remove_callback(self, callback): + def set_on_user_remove_callback(self, callback: Optional[Callable] = None) -> None: """Set callback called on explicit user stream-panel removal. :param callback: Callable receiving the stream instance. """ self._on_user_remove_callback = callback🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/comp/stream_bar.py` around lines 173 - 178, Add a type hint to set_on_user_remove_callback so its parameter is typed as Optional[Callable[[Stream], None]]: update the signature of set_on_user_remove_callback to accept callback: Optional[Callable[[Stream], None]] and ensure you import Optional and Callable from typing and the Stream type used by the callback; also update the attribute _on_user_remove_callback's annotation if present to match.src/odemis/gui/cont/tabs/localization_tab.py (1)
439-447: Consider using a more specific type hint thanobject.The parameter
featureis typed asOptional[object], but based on usage it should beOptional[CryoFeature]. This would provide better type checking and IDE support.Suggested improvement
+from odemis.acq.feature import CryoFeature + `@call_in_wx_main` - def display_acquired_data(self, data, feature: Optional[object]): + def display_acquired_data(self, data, feature: Optional[CryoFeature]): """ Display the acquired streams on the top right view🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/tabs/localization_tab.py` around lines 439 - 447, The parameter type for feature in the method display_acquired_data is too generic (Optional[object]); change its annotation to Optional[CryoFeature] so callers and IDEs get proper type checks and autocompletion, updating any imports to reference CryoFeature and ensuring the function signature and any related docstring references reflect the new type.
🤖 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 866-880: The code computes project_dir from filename returned by
_export_data(), which is a feature subfolder, causing relative_filename and
save_features(project_dir, ...) to write state into the feature folder; change
the logic to derive the real project root (e.g., from the original acquisition
filename or an explicit project_root argument) before calling os.path.relpath
and save_features so relative_filename is computed against the project root and
save_features is called with the project root; update uses in
site.add_stream_record, set_stream_origin, and save_features to use that
derived/explicit project_root rather than os.path.dirname(filename).
- Around line 721-724: feature_storage_dirname currently only replaces '/' and
'\' so names like "." or ".." still produce unsafe directory paths; update
feature_storage_dirname to split the input on path separators, filter out any
segments that are empty or dot-only ('.' or '..'), and join the remaining safe
segments with an underscore (or generate a deterministic fallback such as
"Feature_<short-uuid>" when no valid segments remain); ensure the function still
strips whitespace and returns a non-empty, filesystem-safe string (e.g.,
"Feature" or the generated fallback) so acquisitions cannot escape the project
tree.
- Around line 599-603: The current logic in read_project_state/feature discovery
uses a top-level check (features_state and has_persisted_feature_state
referencing "feature_list") and returns False early, which prevents on-disk
discovery for individual features when project_state.json is partially
migrated/corrupted; remove the global early return and instead perform fallback
checks per feature (when iterating features later) by verifying each feature's
own keys such as "stream_records" (or absence thereof) and only skip in-memory
discovery for that specific feature if its persisted representation is complete;
update the code around read_project_state / features_state /
has_persisted_feature_state to stop returning False globally and add per-feature
guards that consult feature["stream_records"] (or equivalent) before deciding to
avoid on-disk discovery.
In `@src/odemis/acq/project_state.py`:
- Around line 119-129: The read_project_state function should treat unreadable
or malformed state files as missing state: wrap the open/json.load call for
filename (from get_state_filename) in try/except catching IOErrors/OSErrors and
json.JSONDecodeError, and if any exception occurs return {}. After loading,
validate that the top-level value is a mapping (dict); if it's not a dict,
return {} as well. Ensure callers still get an empty mapping on error or invalid
shape rather than letting exceptions propagate.
- Around line 138-140: The current save logic using get_state_filename(...)
writes project_state.json in-place and can produce partial or interleaved files;
change the write in the function that calls get_state_filename to write to a
temporary file (use tempfile.NamedTemporaryFile or write to filename + ".tmp"
atomically), flush and fsync the temp file, then call os.replace(temp_path,
filename) to atomically replace the target; also protect the whole write/replace
sequence with a module-level threading.Lock (e.g., project_state_lock) to
serialize concurrent writers and import os, tempfile, threading as needed.
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2081-2083: Add explicit type hints to the newly added overrides:
annotate CryoStreamsController.__init__(self, tab_data: <appropriate type>,
*args: Any, **kwargs: Any) -> None and annotate both removeStream(self,
stream_id: <appropriate type>) -> None (or the correct return type) so they
follow the project's typing guidelines; apply the same fix to the other two
override locations referenced (around the removeStream overrides at the other
ranges) and import Any/typing types as needed.
- Around line 2085-2092: The method _get_project_path currently falls back to
guiconf.get_acqui_conf().pj_last_path and "" which allows handlers to persist
state to the wrong file; change _get_project_path to only return the active
project's explicit path (i.e. self._tab_data_model.main.project_path.value or
self._main_data_model.project_path.value) and remove the fallback to
guiconf.get_acqui_conf().pj_last_path and the final or "" so it returns None (or
empty/False) when no current project_path is set; update callers of
_get_project_path/_tab_data_model.main.project_path.value/_main_data_model.project_path.value
to bail out early (no write) when the returned path is falsy.
---
Nitpick comments:
In `@src/odemis/gui/comp/stream_bar.py`:
- Around line 173-178: Add a type hint to set_on_user_remove_callback so its
parameter is typed as Optional[Callable[[Stream], None]]: update the signature
of set_on_user_remove_callback to accept callback: Optional[Callable[[Stream],
None]] and ensure you import Optional and Callable from typing and the Stream
type used by the callback; also update the attribute _on_user_remove_callback's
annotation if present to match.
In `@src/odemis/gui/cont/tabs/localization_tab.py`:
- Around line 439-447: The parameter type for feature in the method
display_acquired_data is too generic (Optional[object]); change its annotation
to Optional[CryoFeature] so callers and IDEs get proper type checks and
autocompletion, updating any imports to reference CryoFeature and ensuring the
function signature and any related docstring references reflect the new type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e321fed-2da5-4a1b-867c-5923136cb505
📒 Files selected for processing (12)
src/odemis/acq/feature.pysrc/odemis/acq/project_state.pysrc/odemis/acq/test/feature_test.pysrc/odemis/acq/test/test-features.jsonsrc/odemis/gui/comp/stream_bar.pysrc/odemis/gui/cont/acquisition/cryo_acq.pysrc/odemis/gui/cont/features.pysrc/odemis/gui/cont/stream_bar.pysrc/odemis/gui/cont/tabs/cryo_chamber_tab.pysrc/odemis/gui/cont/tabs/fibsem_tab.pysrc/odemis/gui/cont/tabs/localization_tab.pysrc/odemis/gui/win/acquisition.py
tmoerkerken
force-pushed
the
project-state
branch
from
11ce1a9 to
b874fd2
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a persistent project_state.json mechanism to decouple cryo workflow stream persistence from ad-hoc filename conventions, keeping feature–stream links stable across renames and persisting user deletions so streams don’t reappear.
Changes:
- Adds a central
project_state.jsonstate layer for features + overview stream tracking (including deleted stream indices). - Updates cryo GUI tabs/controllers to attach stream origin metadata and to filter/persist deleted overview/feature streams.
- Updates acquisition/feature filename generation to use per-feature folders and removes feature-name coupling; expands unit tests for the new behavior.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/odemis/gui/win/acquisition.py | Annotates exported data with stream-origin metadata (filename/index). |
| src/odemis/gui/cont/tabs/localization_tab.py | Filters deleted overviews on load and binds acquired streams to the correct feature snapshot. |
| src/odemis/gui/cont/tabs/fibsem_tab.py | Same overview filtering/registration behavior as localization. |
| src/odemis/gui/cont/tabs/cryo_chamber_tab.py | Ensures overview streams inherit origin metadata from raw data. |
| src/odemis/gui/cont/stream_bar.py | Persists user removals (feature + overview) and syncs overview removals across tabs. |
| src/odemis/gui/cont/features.py | Renames feature acquisition folders and rewrites persisted paths on feature rename. |
| src/odemis/gui/cont/correlation.py | Filters out deleted overview streams when syncing into localization/fibsem tabs. |
| src/odemis/gui/cont/acquisition/cryo_acq.py | Writes per-feature acquisition records into project state and passes feature snapshot through async export/display. |
| src/odemis/gui/comp/stream_bar.py | Adds a callback hook for explicit user stream-panel removal. |
| src/odemis/acq/test/test-features.json | Updates fixture to include feature IDs + stream record fields. |
| src/odemis/acq/test/feature_test.py | Adds tests for legacy decoding, filename generation, and stream-origin metadata extraction. |
| src/odemis/acq/project_state.py | New module implementing project_state.json I/O + overview stream registration/deletion tracking. |
| src/odemis/acq/feature.py | Migrates features persistence to project_state.json, adds stream-record model, and updates acquisition filename strategy. |
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+445
to
+449
| for record in feature.stream_records: | ||
| filename = record.get("filename") | ||
| if not isinstance(filename, str): | ||
| continue | ||
| normalized = os.path.normpath(filename) |
Comment on lines
+577
to
+581
| project_dir = self._config.pj_last_path | ||
| rel_filename = os.path.relpath(filename, project_dir) | ||
| current_feature.add_stream_record(rel_filename, len(data)) | ||
| for stream_index, data_array in enumerate(data): | ||
| data_array.metadata[STREAM_ORIGIN_FILENAME_MD] = rel_filename |
Comment on lines
+2219
to
+2223
| project_path = self._get_project_path() | ||
| if feature and stream in feature.streams.value: | ||
| if feature.mark_stream_deleted(filename, stream_index): | ||
| save_features(project_path, self._tab_data_model.main.features.value) | ||
| elif is_overview_stream: |
Comment on lines
+104
to
+114
| def read_project_state(project_dir: str) -> Dict[str, Any]: | ||
| """Load global project state from disk. | ||
|
|
||
| :param project_dir: Project directory. | ||
| :return: Parsed state dictionary or empty mapping when absent. | ||
| """ | ||
| filename = get_state_filename(project_dir) | ||
| if not os.path.exists(filename): | ||
| return {} | ||
| with open(filename, "r") as jsonfile: | ||
| return json.load(jsonfile) |
Comment on lines
+117
to
+125
| def write_project_state(project_dir: str, state: Dict[str, Any]) -> None: | ||
| """Persist global project state to disk. | ||
|
|
||
| :param project_dir: Project directory. | ||
| :param state: State mapping. | ||
| """ | ||
| filename = get_state_filename(project_dir) | ||
| with open(filename, "w") as jsonfile: | ||
| json.dump(state, jsonfile) |
Comment on lines
+176
to
+182
| continue | ||
| if stream_index not in record["stream_indices"]: | ||
| record["stream_indices"].append(stream_index) | ||
| record["stream_indices"].sort() | ||
| if stream_index not in record["deleted_stream_indices"]: | ||
| record["deleted_stream_indices"].append(stream_index) | ||
| record["deleted_stream_indices"].sort() |
tmoerkerken
force-pushed
the
project-state
branch
from
b874fd2 to
fc1754a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
src/odemis/gui/cont/stream_bar.py (2)
2078-2079:⚠️ Potential issue | 🟡 MinorAdd missing type annotations on new/updated overrides.
The new/updated overrides are unannotated (
__init__, bothremoveStreammethods), which violates the Python typing rule for this repo.#!/bin/bash python - <<'PY' import ast from pathlib import Path path = Path("src/odemis/gui/cont/stream_bar.py") tree = ast.parse(path.read_text()) targets = { ("CryoStreamsController", "__init__"), ("CryoAcquiredStreamsController", "removeStream"), ("CryoFIBAcquiredStreamsController", "removeStream"), } for cls in [n for n in tree.body if isinstance(n, ast.ClassDef)]: for fn in [n for n in cls.body if isinstance(n, ast.FunctionDef)]: if (cls.name, fn.name) in targets: missing_args = [a.arg for a in fn.args.args[1:] if a.annotation is None] print(f"{cls.name}.{fn.name}: return_annotated={fn.returns is not None}, missing_args={missing_args}") PYAs per coding guidelines, "Always use type hints for function parameters and return types in Python code".
Also applies to: 2407-2418, 2616-2622
🤖 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 2078 - 2079, The listed overrides lack type hints; add parameter and return type annotations for CryoStreamsController.__init__ (annotate tab_data with the correct type or use typing.Any if unknown and set return type to None) and for CryoAcquiredStreamsController.removeStream and CryoFIBAcquiredStreamsController.removeStream (annotate their parameters and set return types to None). Ensure you import Any or the concrete types used, update both method signatures and any overridden signatures to match repo typing conventions, and apply the same fixes to the other similar overrides in this file mentioned in the comment.
2081-2088:⚠️ Potential issue | 🟠 MajorPersist only to the active project path; remove fallback writes.
Line 2086-2087 can redirect writes to a stale project or
./project_state.json. This can corrupt unrelated project state from delete/tint handlers.Proposed fix
-from odemis.gui import conf as guiconf @@ - def _get_project_path(self) -> str: - """Resolve project path for state persistence.""" + def _get_project_path(self) -> Optional[str]: + """Resolve active project path for state persistence.""" return ( self._tab_data_model.main.project_path.value or self._main_data_model.project_path.value - or guiconf.get_acqui_conf().pj_last_path - or "" ) @@ - project_path = self._get_project_path() + project_path = self._get_project_path() + if not project_path: + logging.debug("Skipping stream deletion persistence: no active project path") + return @@ - project_path = self._get_project_path() + project_path = self._get_project_path() + if not project_path: + logging.debug("Skipping stream deletion persistence: no active project path") + returnAlso applies to: 2219-2225, 2535-2537
🤖 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 2081 - 2088, The current _get_project_path method (and the similar code at the other locations) falls back to _main_data_model.project_path.value, guiconf.get_acqui_conf().pj_last_path, or "" which allows writes to stale projects or ./project_state.json; change _get_project_path to return only the active project path from self._tab_data_model.main.project_path.value (and if that is missing, return None or raise/short-circuit so callers do not write), and update the other occurrences at the same pattern (the blocks around lines referenced) to use only self._tab_data_model.main.project_path.value to prevent fallback writes to unrelated project state.src/odemis/acq/project_state.py (1)
104-115:⚠️ Potential issue | 🟠 MajorTreat malformed/non-mapping state as missing state.
Line 114 can raise
JSONDecodeError, and returning non-dict payloads breaks laterstate.get(...)usage. This blocks the intended corrupted-state fallback path.Suggested fix
def read_project_state(project_dir: str) -> Dict[str, Any]: @@ filename = get_state_filename(project_dir) if not os.path.exists(filename): return {} - with open(filename, "r") as jsonfile: - return json.load(jsonfile) + try: + with open(filename, "r", encoding="utf-8") as jsonfile: + state = json.load(jsonfile) + except (OSError, json.JSONDecodeError): + return {} + return state if isinstance(state, dict) else {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/project_state.py` around lines 104 - 115, The read_project_state function should treat malformed or non-mapping state as missing: wrap the json.load call in a try/except that catches json.JSONDecodeError (and optionally ValueError/OSError) and return {} on failure, and after loading ensure the returned value is a dict (if not, return {}); update the logic around get_state_filename/read_project_state so callers always receive a mapping rather than a parsed non-dict or an exception.src/odemis/acq/feature.py (2)
674-677:⚠️ Potential issue | 🟠 MajorHarden
feature_storage_dirnameagainst dot-segment directory escapes.Line 676 currently allows
"."/".."(and dot-only path segments), which can escape intended project subdirectories when combined increate_feature_acquisition_filename.Proposed fix
def feature_storage_dirname(feature_name: str) -> str: """Return filesystem-safe directory name for feature acquisitions.""" - dirname = feature_name.replace("/", "_").replace("\\", "_").strip() - return dirname or "Feature" + normalized = feature_name.replace("\\", "/").strip() + parts = [ + p.strip() + for p in normalized.split("/") + if p.strip() and p.strip() not in {".", ".."} + ] + dirname = "_".join(parts) + return dirname or "Feature"🤖 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 674 - 677, The function feature_storage_dirname currently allows dot-only names like "." or ".." which can escape directories; update feature_storage_dirname to sanitize dot-segments by splitting the cleaned name on path separators, replace any segment matching only dots (regex ^\.+$) with a safe token (e.g., "_" or "Feature"), then rejoin and strip; if the final dirname is empty or still unsafe, return the fallback "Feature". Ensure this hardening is applied so calls like create_feature_acquisition_filename cannot be tricked into escaping intended subdirectories.
558-563:⚠️ Potential issue | 🟠 MajorKeep fallback discovery decision per feature, not by top-level state presence.
On Line 561–562, this early return suppresses on-disk discovery for a feature whose own
stream_recordsare missing/corrupt, as soon asfeatures.feature_listexists in state. That breaks partial-recovery behavior.Proposed fix
if not records: - state = read_project_state(path) - features_state = state.get("features") - has_persisted_feature_state = isinstance(features_state, dict) and "feature_list" in features_state - if has_persisted_feature_state: - return False - + # Decide fallback per feature: if this feature has no records, + # attempt disk discovery (legacy/missing/corrupt per-feature state). feature.stream_records.extend(_discover_legacy_stream_records(feature, path)) records = feature.stream_records migrated = bool(records)🤖 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 558 - 563, The early return that checks has_persisted_feature_state (using read_project_state and features_state with "feature_list") prevents on-disk discovery for individual features whose own persisted data (e.g., their stream_records) are missing or corrupt; remove this top-level short-circuit and instead perform the fallback decision per feature: in the discovery path that calls/read_project_state, inspect the specific feature entry (by feature name/id from feature_list) and verify that its own persisted keys like "stream_records" are present and valid; if a given feature is missing/invalid, allow that single feature to run on-disk discovery/fallback, but keep the persisted state for other features intact (i.e., replace the global has_persisted_feature_state check with per-feature validation logic).
🤖 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/project_state.py`:
- Around line 63-76: The comprehension for computing stream_indices and
deleted_stream_indices assumes record.get(...) returns an iterable and will fail
on null or scalar values; change the code around the two comprehensions (the
occurrences of stream_indices and deleted_stream_indices) to first coerce the
raw value into a safe iterable (e.g., raw = record.get("stream_indices"); if not
isinstance(raw, (list, tuple, set)): raw = [raw] if isinstance(raw, int) else
[]; then use the comprehension over that safe iterable) so only non-negative
ints are converted and sorted; do the same for
record.get("deleted_stream_indices").
In `@src/odemis/gui/cont/acquisition/cryo_acq.py`:
- Around line 101-103: _active_acq_feature is a shared mutable field that can be
overwritten by a new acquisition before async export/display tasks read it;
instead capture a per-task snapshot of the CryoFeature and use that snapshot
inside any async/export/display/stream-persist logic so the task uses the
correct feature even if self._active_acq_feature changes. Concretely, where you
currently reference self._active_acq_feature inside async calls or when
scheduling export/display/stream record persistence (the export/display
coroutines and the stream-persistence path), assign local variable
feature_snapshot = self._active_acq_feature (or a shallow copy) before creating
the async task and pass that snapshot explicitly into the task/coroutine or
closure; stop relying on reading self._active_acq_feature inside the async
worker and avoid reassigning/clearing the shared field from those tasks. Ensure
any code that re-enables acquisition does not race with exports by binding the
snapshot at task creation and optionally await or track task completion to clear
UI state if needed.
In `@src/odemis/gui/cont/features.py`:
- Around line 445-490: The code currently mutates feature.stream_records
filenames and in-memory stream origins (via get_stream_origin/set_stream_origin
using old_prefix/new_prefix) before checking for conflicts and performing the
actual on-disk rename (os.rename of old_dir -> new_dir built from project_dir
and old_dirname/new_dirname); move the two rewrite loops so they run only after
the conflict check and successful os.rename (i.e., after verifying new_dir does
not exist and os.rename completed without exception), leaving filenames and
stream origins untouched if the rename is rejected or fails.
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2078-2079: Add a reStructuredText-style docstring to the
CryoStreamsController.__init__ method that explains the constructor’s purpose,
describes the parameters (tab_data and *args, **kwargs) and any side effects or
important initialization behavior, and omits type annotations per project
guidelines; locate the __init__ implementation in CryoStreamsController and
insert the docstring immediately after the def line following the repository’s
docstring conventions.
---
Duplicate comments:
In `@src/odemis/acq/feature.py`:
- Around line 674-677: The function feature_storage_dirname currently allows
dot-only names like "." or ".." which can escape directories; update
feature_storage_dirname to sanitize dot-segments by splitting the cleaned name
on path separators, replace any segment matching only dots (regex ^\.+$) with a
safe token (e.g., "_" or "Feature"), then rejoin and strip; if the final dirname
is empty or still unsafe, return the fallback "Feature". Ensure this hardening
is applied so calls like create_feature_acquisition_filename cannot be tricked
into escaping intended subdirectories.
- Around line 558-563: The early return that checks has_persisted_feature_state
(using read_project_state and features_state with "feature_list") prevents
on-disk discovery for individual features whose own persisted data (e.g., their
stream_records) are missing or corrupt; remove this top-level short-circuit and
instead perform the fallback decision per feature: in the discovery path that
calls/read_project_state, inspect the specific feature entry (by feature name/id
from feature_list) and verify that its own persisted keys like "stream_records"
are present and valid; if a given feature is missing/invalid, allow that single
feature to run on-disk discovery/fallback, but keep the persisted state for
other features intact (i.e., replace the global has_persisted_feature_state
check with per-feature validation logic).
In `@src/odemis/acq/project_state.py`:
- Around line 104-115: The read_project_state function should treat malformed or
non-mapping state as missing: wrap the json.load call in a try/except that
catches json.JSONDecodeError (and optionally ValueError/OSError) and return {}
on failure, and after loading ensure the returned value is a dict (if not,
return {}); update the logic around get_state_filename/read_project_state so
callers always receive a mapping rather than a parsed non-dict or an exception.
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2078-2079: The listed overrides lack type hints; add parameter and
return type annotations for CryoStreamsController.__init__ (annotate tab_data
with the correct type or use typing.Any if unknown and set return type to None)
and for CryoAcquiredStreamsController.removeStream and
CryoFIBAcquiredStreamsController.removeStream (annotate their parameters and set
return types to None). Ensure you import Any or the concrete types used, update
both method signatures and any overridden signatures to match repo typing
conventions, and apply the same fixes to the other similar overrides in this
file mentioned in the comment.
- Around line 2081-2088: The current _get_project_path method (and the similar
code at the other locations) falls back to _main_data_model.project_path.value,
guiconf.get_acqui_conf().pj_last_path, or "" which allows writes to stale
projects or ./project_state.json; change _get_project_path to return only the
active project path from self._tab_data_model.main.project_path.value (and if
that is missing, return None or raise/short-circuit so callers do not write),
and update the other occurrences at the same pattern (the blocks around lines
referenced) to use only self._tab_data_model.main.project_path.value to prevent
fallback writes to unrelated project state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d44a006a-7fe6-435d-a172-e5b9e4445006
📒 Files selected for processing (13)
src/odemis/acq/feature.pysrc/odemis/acq/project_state.pysrc/odemis/acq/test/feature_test.pysrc/odemis/acq/test/test-features.jsonsrc/odemis/gui/comp/stream_bar.pysrc/odemis/gui/cont/acquisition/cryo_acq.pysrc/odemis/gui/cont/correlation.pysrc/odemis/gui/cont/features.pysrc/odemis/gui/cont/stream_bar.pysrc/odemis/gui/cont/tabs/cryo_chamber_tab.pysrc/odemis/gui/cont/tabs/fibsem_tab.pysrc/odemis/gui/cont/tabs/localization_tab.pysrc/odemis/gui/win/acquisition.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/odemis/gui/win/acquisition.py
- src/odemis/gui/cont/tabs/cryo_chamber_tab.py
- src/odemis/gui/comp/stream_bar.py
Comment on lines
+63
to
+76
| stream_indices = sorted( | ||
| { | ||
| int(index) | ||
| for index in record.get("stream_indices", []) | ||
| if isinstance(index, int) and index >= 0 | ||
| } | ||
| ) | ||
| deleted_stream_indices = sorted( | ||
| { | ||
| int(index) | ||
| for index in record.get("deleted_stream_indices", []) | ||
| if isinstance(index, int) and index >= 0 | ||
| } | ||
| ) |
There was a problem hiding this comment.
Normalize index fields defensively before iterating.
Line 66/Line 73 assume stream_indices and deleted_stream_indices are iterable. Corrupt values like null or 1 will raise TypeError during normalization.
Suggested fix
- stream_indices = sorted(
+ raw_stream_indices = record.get("stream_indices", [])
+ if not isinstance(raw_stream_indices, (list, tuple, set)):
+ raw_stream_indices = []
+ stream_indices = sorted(
{
int(index)
- for index in record.get("stream_indices", [])
+ for index in raw_stream_indices
if isinstance(index, int) and index >= 0
}
)
- deleted_stream_indices = sorted(
+ raw_deleted_indices = record.get("deleted_stream_indices", [])
+ if not isinstance(raw_deleted_indices, (list, tuple, set)):
+ raw_deleted_indices = []
+ deleted_stream_indices = sorted(
{
int(index)
- for index in record.get("deleted_stream_indices", [])
+ for index in raw_deleted_indices
if isinstance(index, int) and index >= 0
}
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/acq/project_state.py` around lines 63 - 76, The comprehension for
computing stream_indices and deleted_stream_indices assumes record.get(...)
returns an iterable and will fail on null or scalar values; change the code
around the two comprehensions (the occurrences of stream_indices and
deleted_stream_indices) to first coerce the raw value into a safe iterable
(e.g., raw = record.get("stream_indices"); if not isinstance(raw, (list, tuple,
set)): raw = [raw] if isinstance(raw, int) else []; then use the comprehension
over that safe iterable) so only non-negative ints are converted and sorted; do
the same for record.get("deleted_stream_indices").
Comment on lines
+445
to
+490
| for record in feature.stream_records: | ||
| filename = record.get("filename") | ||
| if not isinstance(filename, str): | ||
| continue | ||
| normalized = os.path.normpath(filename) | ||
| # Only rewrite files currently under the old feature folder. | ||
| if not normalized.startswith(old_prefix): | ||
| continue | ||
| # Keep trailing file path unchanged: | ||
| # "Feature-1/acq-001.ome.tiff" -> "Feature-A/acq-001.ome.tiff" | ||
| suffix = normalized[len(old_prefix):] | ||
| record["filename"] = os.path.normpath(new_prefix + suffix) | ||
|
|
||
| # Also update origins on already loaded in-memory streams so UI actions | ||
| # (delete/tint/save) keep pointing to renamed files. | ||
| for stream in feature.streams.value: | ||
| filename, stream_index = get_stream_origin(stream) | ||
| if not isinstance(filename, str) or not isinstance(stream_index, int): | ||
| continue | ||
| normalized = os.path.normpath(filename) | ||
| if not normalized.startswith(old_prefix): | ||
| continue | ||
| suffix = normalized[len(old_prefix):] | ||
| set_stream_origin(stream, os.path.normpath(new_prefix + suffix), stream_index) | ||
|
|
||
| old_dir = os.path.join(project_dir, old_dirname) | ||
| new_dir = os.path.join(project_dir, new_dirname) | ||
| if os.path.isdir(old_dir): | ||
| # Do not overwrite an existing target folder; keep old folder and warn. | ||
| if os.path.exists(new_dir): | ||
| logging.warning( | ||
| "Cannot rename feature acquisition folder %s -> %s: target exists.", | ||
| old_dir, | ||
| new_dir, | ||
| ) | ||
| return False | ||
| # Final on-disk rename for acquisition files. | ||
| try: | ||
| os.rename(old_dir, new_dir) | ||
| except OSError: | ||
| logging.exception( | ||
| "Cannot rename feature acquisition folder %s -> %s.", | ||
| old_dir, | ||
| new_dir, | ||
| ) | ||
| return False |
There was a problem hiding this comment.
Apply record/origin rewrites only after rename succeeds.
On Line 445 and Line 460, paths are rewritten before the conflict check (Line 474) and os.rename (Line 483). If rename is rejected/fails, the method returns False but paths are already mutated.
Suggested fix
- for record in feature.stream_records:
- filename = record.get("filename")
- if not isinstance(filename, str):
- continue
- normalized = os.path.normpath(filename)
- if not normalized.startswith(old_prefix):
- continue
- suffix = normalized[len(old_prefix):]
- record["filename"] = os.path.normpath(new_prefix + suffix)
-
- for stream in feature.streams.value:
- filename, stream_index = get_stream_origin(stream)
- if not isinstance(filename, str) or not isinstance(stream_index, int):
- continue
- normalized = os.path.normpath(filename)
- if not normalized.startswith(old_prefix):
- continue
- suffix = normalized[len(old_prefix):]
- set_stream_origin(stream, os.path.normpath(new_prefix + suffix), stream_index)
-
old_dir = os.path.join(project_dir, old_dirname)
new_dir = os.path.join(project_dir, new_dirname)
+ if os.path.isdir(old_dir) and os.path.exists(new_dir):
+ logging.warning(
+ "Cannot rename feature acquisition folder %s -> %s: target exists.",
+ old_dir,
+ new_dir,
+ )
+ return False
if os.path.isdir(old_dir):
- if os.path.exists(new_dir):
- logging.warning(
- "Cannot rename feature acquisition folder %s -> %s: target exists.",
- old_dir,
- new_dir,
- )
- return False
try:
os.rename(old_dir, new_dir)
except OSError:
logging.exception(
"Cannot rename feature acquisition folder %s -> %s.",
old_dir,
new_dir,
)
return False
+
+ for record in feature.stream_records:
+ filename = record.get("filename")
+ if not isinstance(filename, str):
+ continue
+ normalized = os.path.normpath(filename)
+ if not normalized.startswith(old_prefix):
+ continue
+ suffix = normalized[len(old_prefix):]
+ record["filename"] = os.path.normpath(new_prefix + suffix)
+
+ for stream in feature.streams.value:
+ filename, stream_index = get_stream_origin(stream)
+ if not isinstance(filename, str) or not isinstance(stream_index, int):
+ continue
+ normalized = os.path.normpath(filename)
+ if not normalized.startswith(old_prefix):
+ continue
+ suffix = normalized[len(old_prefix):]
+ set_stream_origin(stream, os.path.normpath(new_prefix + suffix), stream_index)
return True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/gui/cont/features.py` around lines 445 - 490, The code currently
mutates feature.stream_records filenames and in-memory stream origins (via
get_stream_origin/set_stream_origin using old_prefix/new_prefix) before checking
for conflicts and performing the actual on-disk rename (os.rename of old_dir ->
new_dir built from project_dir and old_dirname/new_dirname); move the two
rewrite loops so they run only after the conflict check and successful os.rename
(i.e., after verifying new_dir does not exist and os.rename completed without
exception), leaving filenames and stream origins untouched if the rename is
rejected or fails.
Comment on lines
+2078
to
+2079
| def __init__(self, tab_data, *args, **kwargs): | ||
| super().__init__(tab_data, *args, **kwargs) |
There was a problem hiding this comment.
Add a docstring to the new CryoStreamsController.__init__.
Line 2078-2079 introduces a new function without a docstring, while this repository requires docstrings on all functions.
As per coding guidelines, "Include docstrings for all functions and classes, following the reStructuredText style guide (without type information)".
🤖 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 2078 - 2079, Add a
reStructuredText-style docstring to the CryoStreamsController.__init__ method
that explains the constructor’s purpose, describes the parameters (tab_data and
*args, **kwargs) and any side effects or important initialization behavior, and
omits type annotations per project guidelines; locate the __init__
implementation in CryoStreamsController and insert the docstring immediately
after the def line following the repository’s docstring conventions.
fc1754a to
326d7a0
Compare
tmoerkerken
force-pushed
the
project-state
branch
from
326d7a0 to
3bc9d34
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
src/odemis/gui/cont/features.py (1)
445-490:⚠️ Potential issue | 🟠 MajorDelay the stream-path rewrites until the directory rename succeeds.
feature.stream_recordsand loaded stream origins are rewritten before the target-exists check andos.rename(). If the rename is rejected or fails,_on_cmb_feature_name_change()keeps the old feature name, but follow-up delete/tint/save actions now point at a folder that was never created.Also applies to: 501-504
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/features.py` around lines 445 - 490, The code currently updates feature.stream_records and in-memory stream origins before checking target existence and performing os.rename, which can leave references pointing to a non-existent folder if rename fails; move the loops that rewrite record["filename"] and call set_stream_origin (the sections iterating feature.stream_records and for stream in feature.streams.value using get_stream_origin / set_stream_origin) so they run only after the existence check and after os.rename succeeds (i.e., after the block that verifies not os.path.exists(new_dir) and after the try/except that calls os.rename(old_dir, new_dir) and returns False on exception); keep the early exits (return False) and logging as-is so no in-memory rewrites occur when the on-disk rename is rejected or fails.src/odemis/gui/cont/acquisition/cryo_acq.py (1)
101-103:⚠️ Potential issue | 🔴 CriticalPass a per-export feature snapshot through the async path.
Acquisition is re-enabled before the export worker runs, but
_create_cryo_filename(),_export_data()and_display_acquired_data()still read the shared_active_acq_feature. Starting a second acquisition can therefore save the first dataset under the wrong feature and attach the displayed streams to the wrong feature record.🐛 Proposed fix
- scheduled_future = executor.submit(self._export_data, data) + feature_snapshot = self._active_acq_feature + scheduled_future = executor.submit(self._export_data, data, feature_snapshot) scheduled_future.add_done_callback(self._on_export_data_done) @@ - def _display_acquired_data(self, data): + def _display_acquired_data( + self, + data, + feature: Optional[CryoFeature], + ) -> None: """ Shows the acquired image on the view """ if self.acqui_mode is guimod.AcquiMode.FLM: - self._tab.display_acquired_data(data, feature=self._active_acq_feature) + self._tab.display_acquired_data(data, feature=feature) @@ - def _export_data(self, data: List[model.DataArray], thumb_nail=None): + def _export_data( + self, + data: List[model.DataArray], + feature: Optional[CryoFeature], + thumb_nail=None, + ) -> tuple[List[model.DataArray], Optional[CryoFeature]]: @@ - current_feature = self._active_acq_feature + current_feature = feature @@ - return data + return data, feature @@ - data = future.result() - self._display_acquired_data(data) + data, feature = future.result() + self._display_acquired_data(data, feature) self._active_acq_feature = NoneAlso applies to: 360-361, 515-526, 575-583
🤖 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 101 - 103, The export/display path currently reads the shared mutable _active_acq_feature which can change before async tasks run; capture a per-export snapshot of the feature and pass it down the async chain instead of reading _active_acq_feature inside _create_cryo_filename, _export_data and _display_acquired_data; modify the call sites that start the async work to bind a local feature_snapshot = self._active_acq_feature and add a feature parameter to those functions (or include it on the export task object) so each async worker uses that immutable snapshot rather than the shared attribute.src/odemis/acq/project_state.py (1)
53-76:⚠️ Potential issue | 🟠 MajorNormalize malformed records before iterating their fields.
The corrupt-state fallback still breaks here: a stray list entry like
null/1will hitrecord.get(...), andstream_indices: nullor1will raise while iterating. That makesread_overview_records()fail on exactly the kind of damaged state this module is supposed to tolerate.🛡️ Proposed fix
def normalize_stream_record(record: Dict[str, Any]) -> Dict[str, Any]: """Normalize one persisted stream-record mapping. @@ + if not isinstance(record, dict): + return {"filename": "", "stream_indices": [], "deleted_stream_indices": []} + filename = record.get("filename") if not isinstance(filename, str): return {"filename": "", "stream_indices": [], "deleted_stream_indices": []} + raw_stream_indices = record.get("stream_indices", []) + if not isinstance(raw_stream_indices, (list, tuple, set)): + raw_stream_indices = [] stream_indices = sorted( { int(index) - for index in record.get("stream_indices", []) + for index in raw_stream_indices if isinstance(index, int) and index >= 0 } ) + raw_deleted_indices = record.get("deleted_stream_indices", []) + if not isinstance(raw_deleted_indices, (list, tuple, set)): + raw_deleted_indices = [] deleted_stream_indices = sorted( { int(index) - for index in record.get("deleted_stream_indices", []) + for index in raw_deleted_indices if isinstance(index, int) and index >= 0 } )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/project_state.py` around lines 53 - 76, The normalize_stream_record function currently assumes record.get("stream_indices") and record.get("deleted_stream_indices") are iterable lists and will crash on non-list values; before iterating, validate/coerce these fields to lists (e.g., treat None, numbers, strings, or other non-iterables as empty lists) and only iterate when the value is a sequence type; apply this defensive check to both "stream_indices" and "deleted_stream_indices" in normalize_stream_record and then proceed to filter/convert each element to int and >=0 as currently done.src/odemis/gui/cont/stream_bar.py (2)
2081-2088:⚠️ Potential issue | 🟠 MajorOnly resolve the active project's path here.
Falling back to
guiconf.get_acqui_conf().pj_last_pathand then""lets the delete-state handlers write another project'sproject_state.json, or./project_state.json, whenever the current tab has not populatedproject_pathyet. Return only the explicit active path here and have the callers skip persistence when it is missing.🤖 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 2081 - 2088, The _get_project_path function should only return an explicitly set active project path to avoid writing another project's state; change _get_project_path to return only self._tab_data_model.main.project_path.value or self._main_data_model.project_path.value (no fallback to guiconf.get_acqui_conf().pj_last_path or ""), and update the delete-state handlers (call sites that call _get_project_path) to check for a truthy path and skip persistence/write when it is missing; reference _get_project_path and the delete-state handler functions/methods that currently call it to implement this conditional early-return behavior.
2078-2079:⚠️ Potential issue | 🟡 MinorFinish the typing/docstrings on the new cryo overrides.
The new constructor override still misses parameter/return annotations and a docstring, and both new
removeStream()overrides still miss parameter/return annotations.As per coding guidelines, "Always use type hints for function parameters and return types in Python code" and "Include docstrings for all functions and classes, following the reStructuredText style guide (without type information)".
Also applies to: 2411-2412, 2624-2625
🤖 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 2078 - 2079, Add missing type hints and reStructuredText docstrings to the new overrides: annotate __init__(self, tab_data: TabDataType, *args: Any, **kwargs: Any) -> None and provide a one-paragraph docstring describing purpose, parameters and behavior (no type info in docstring); do the same for the two removeStream(self, stream_id: str) -> None overrides (or use the actual parameter names/types used elsewhere in the module) and include brief reST-style docstrings for each explaining what they remove and any side effects or exceptions; ensure imports for Any and the TabDataType (or correct type) are added or referenced from existing module types.src/odemis/acq/feature.py (1)
557-567:⚠️ Potential issue | 🟠 MajorDon't make fallback discovery all-or-nothing.
Fallback discovery only runs when
feature.stream_recordsis completely empty, and it is skipped entirely once the top-levelfeaturesblock exists. A partially migrated/corrupt feature with missing or incomplete records will never rediscover its files, so its streams disappear on reload.Suggested merge-oriented fix
- records = feature.stream_records - if not records: - state = read_project_state(path) - features_state = state.get("features") - has_persisted_feature_state = isinstance(features_state, dict) and "feature_list" in features_state - if has_persisted_feature_state: - return False - - feature.stream_records.extend(_discover_legacy_stream_records(feature, path)) - records = feature.stream_records - migrated = bool(records) + records = feature.stream_records + discovered_records = _discover_legacy_stream_records(feature, path) + if discovered_records: + known_filenames = {record["filename"] for record in records} + for record in discovered_records: + if record["filename"] not in known_filenames: + feature.stream_records.append(record) + migrated = True + records = feature.stream_records🤖 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 557 - 567, The current logic skips fallback discovery whenever a top-level "features" block exists, causing partially migrated features to never rediscover their streams; change the check to perform per-feature fallback discovery: read the project state via read_project_state(path), inspect features_state (state.get("features")) for an entry specific to this feature (e.g., features_state.get(feature.name) or similar), and only skip discovery if that per-feature entry already contains a valid "feature_list"; otherwise call _discover_legacy_stream_records(feature, path) and extend feature.stream_records (using feature.stream_records.extend(...)) so missing records are added without requiring the entire top-level features block to be absent, and set migrated = bool(records) accordingly.
🧹 Nitpick comments (1)
src/odemis/gui/cont/tabs/localization_tab.py (1)
461-469: Use the concrete feature type in this API.This method dereferences
stream_recordsandstreams, soOptional[object]hides the real contract and drops useful type checking on the new entry point. Please narrow this toOptional["CryoFeature"]and add-> None.As per coding guidelines, "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/cont/tabs/localization_tab.py` around lines 461 - 469, The display_acquired_data signature uses a vague Optional[object] for feature while the method dereferences stream_records and streams; change the parameter type to Optional["CryoFeature"] (or the actual CryoFeature symbol) and add an explicit return annotation -> None on display_acquired_data to restore type checking; update any necessary forward reference imports or quotes around CryoFeature to avoid import cycles and run type checks to ensure stream_records/streams usages now type-check against CryoFeature.
🤖 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 467-473: This code performs an unlocked read/modify/write of
project_state.json and can race with save_overview_records(); switch to the
single locked/merge update path exposed by the project_state module instead of
calling read_project_state/write_project_state directly: call the module's
atomic merge function (the same mechanism used by save_overview_records()) and,
inside its update callback, set state["state_version"]=PROJECT_STATE_VERSION and
state["features"]=get_features_dict(features") while leaving/merging existing
keys such as "overview_records" intact; reference get_features_dict,
read_project_state/write_project_state only to remove them and use the
project_state merge helper (the same API used by save_overview_records) to
serialize updates.
---
Duplicate comments:
In `@src/odemis/acq/feature.py`:
- Around line 557-567: The current logic skips fallback discovery whenever a
top-level "features" block exists, causing partially migrated features to never
rediscover their streams; change the check to perform per-feature fallback
discovery: read the project state via read_project_state(path), inspect
features_state (state.get("features")) for an entry specific to this feature
(e.g., features_state.get(feature.name) or similar), and only skip discovery if
that per-feature entry already contains a valid "feature_list"; otherwise call
_discover_legacy_stream_records(feature, path) and extend feature.stream_records
(using feature.stream_records.extend(...)) so missing records are added without
requiring the entire top-level features block to be absent, and set migrated =
bool(records) accordingly.
In `@src/odemis/acq/project_state.py`:
- Around line 53-76: The normalize_stream_record function currently assumes
record.get("stream_indices") and record.get("deleted_stream_indices") are
iterable lists and will crash on non-list values; before iterating,
validate/coerce these fields to lists (e.g., treat None, numbers, strings, or
other non-iterables as empty lists) and only iterate when the value is a
sequence type; apply this defensive check to both "stream_indices" and
"deleted_stream_indices" in normalize_stream_record and then proceed to
filter/convert each element to int and >=0 as currently done.
In `@src/odemis/gui/cont/acquisition/cryo_acq.py`:
- Around line 101-103: The export/display path currently reads the shared
mutable _active_acq_feature which can change before async tasks run; capture a
per-export snapshot of the feature and pass it down the async chain instead of
reading _active_acq_feature inside _create_cryo_filename, _export_data and
_display_acquired_data; modify the call sites that start the async work to bind
a local feature_snapshot = self._active_acq_feature and add a feature parameter
to those functions (or include it on the export task object) so each async
worker uses that immutable snapshot rather than the shared attribute.
In `@src/odemis/gui/cont/features.py`:
- Around line 445-490: The code currently updates feature.stream_records and
in-memory stream origins before checking target existence and performing
os.rename, which can leave references pointing to a non-existent folder if
rename fails; move the loops that rewrite record["filename"] and call
set_stream_origin (the sections iterating feature.stream_records and for stream
in feature.streams.value using get_stream_origin / set_stream_origin) so they
run only after the existence check and after os.rename succeeds (i.e., after the
block that verifies not os.path.exists(new_dir) and after the try/except that
calls os.rename(old_dir, new_dir) and returns False on exception); keep the
early exits (return False) and logging as-is so no in-memory rewrites occur when
the on-disk rename is rejected or fails.
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2081-2088: The _get_project_path function should only return an
explicitly set active project path to avoid writing another project's state;
change _get_project_path to return only
self._tab_data_model.main.project_path.value or
self._main_data_model.project_path.value (no fallback to
guiconf.get_acqui_conf().pj_last_path or ""), and update the delete-state
handlers (call sites that call _get_project_path) to check for a truthy path and
skip persistence/write when it is missing; reference _get_project_path and the
delete-state handler functions/methods that currently call it to implement this
conditional early-return behavior.
- Around line 2078-2079: Add missing type hints and reStructuredText docstrings
to the new overrides: annotate __init__(self, tab_data: TabDataType, *args: Any,
**kwargs: Any) -> None and provide a one-paragraph docstring describing purpose,
parameters and behavior (no type info in docstring); do the same for the two
removeStream(self, stream_id: str) -> None overrides (or use the actual
parameter names/types used elsewhere in the module) and include brief reST-style
docstrings for each explaining what they remove and any side effects or
exceptions; ensure imports for Any and the TabDataType (or correct type) are
added or referenced from existing module types.
---
Nitpick comments:
In `@src/odemis/gui/cont/tabs/localization_tab.py`:
- Around line 461-469: The display_acquired_data signature uses a vague
Optional[object] for feature while the method dereferences stream_records and
streams; change the parameter type to Optional["CryoFeature"] (or the actual
CryoFeature symbol) and add an explicit return annotation -> None on
display_acquired_data to restore type checking; update any necessary forward
reference imports or quotes around CryoFeature to avoid import cycles and run
type checks to ensure stream_records/streams usages now type-check against
CryoFeature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac588668-f657-416c-8428-6e704b7a2802
📒 Files selected for processing (13)
src/odemis/acq/feature.pysrc/odemis/acq/project_state.pysrc/odemis/acq/test/feature_test.pysrc/odemis/acq/test/test-features.jsonsrc/odemis/gui/comp/stream_bar.pysrc/odemis/gui/cont/acquisition/cryo_acq.pysrc/odemis/gui/cont/correlation.pysrc/odemis/gui/cont/features.pysrc/odemis/gui/cont/stream_bar.pysrc/odemis/gui/cont/tabs/cryo_chamber_tab.pysrc/odemis/gui/cont/tabs/fibsem_tab.pysrc/odemis/gui/cont/tabs/localization_tab.pysrc/odemis/gui/win/acquisition.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/odemis/gui/comp/stream_bar.py
- src/odemis/gui/cont/correlation.py
- src/odemis/gui/cont/tabs/cryo_chamber_tab.py
pieleric
requested changes
Mar 24, 2026
|
|
||
| from odemis.acq.stream import Stream | ||
|
|
||
| PROJECT_STATE_FILENAME = "project_state.json" |
Member
There was a problem hiding this comment.
Let's name name the file simply project.json, because the state is the most logical thing that would be saved in a file called "project". This python module can stay called "project_state.json".
|
|
||
| :param feature: the feature whose streams to load | ||
| :param path: path to the project directory containing the stream files | ||
| :return: ``True`` when legacy name-based discovery was migrated to persisted records. |
Member
There was a problem hiding this comment.
I would rather avoid markdown in docstrings. Especially such double back quotes don't help with readability (although copilot likes them).
We extend the copilot instructions to tell it to stop (in a separate PR).
Comment on lines
+178
to
+179
| # Stable feature identity persisted in project state for forward-compatible linking/migration. | ||
| self.id = str(uuid.uuid4()) |
Member
There was a problem hiding this comment.
It doesn't seem used. Maybe I don't really get the comment. Do you mean it'll only be used in the future in case of changing the format? If not, maybe drop this?
| """ | ||
| Convert list of features to JSON serializable list of dict | ||
| :param features: list of CryoFeature | ||
| :return: list of JSON serializable features |
Member
There was a problem hiding this comment.
Could you take the opportunity to adjust also this description of the return value
| :return: Stream indices to keep linked to the feature. | ||
| """ | ||
| record = self._stream_record_lookup().get(filename) | ||
| if record is not None: |
Member
There was a problem hiding this comment.
Please invert the logic, to handle early the special case of record is None.
Maybe also explain why when a filename is not in the record, you return that all the available counts are present. Is that some sort of optimism?
Member
There was a problem hiding this comment.
This file needs some test cases.
|
|
||
| You should have received a copy of the GNU General Public License along with | ||
| Odemis. If not, see http://www.gnu.org/licenses/. | ||
| """ |
Member
There was a problem hiding this comment.
Add a small explanation of what is saved in a "project" state. I think you should at least show the structure of the JSON file (the different entries). Also, clarify the relationship with the feature.json.
It would also help to explain what a "record" is.
Especially, it's not clear how the relationship between a project (state file) and a features (state file) work.
| return False | ||
|
|
||
|
|
||
| def set_stream_origin(stream: Stream, filename: str, stream_index: int) -> None: |
Member
There was a problem hiding this comment.
This whole "stream origin" is written as a hack on Odemis. We don't need a hack, we control the whole source code!
I think you want to store the filename and index on the stream -> add attributes (or properties) to the Stream base class.
If you want to add new metadata, add it to _metadata.py. If you want to store info about where a dataarray comes from, it'd make more sense to add it to the tiff and hdf5 exporters, instead of trying to sneak the info in after the DataArrays are returned.
| :param project_dir: directory to read the file from (typically project directory) | ||
| :return: list of deserialized features | ||
| """ | ||
| state = read_project_state(project_dir) |
Member
There was a problem hiding this comment.
It feels odd that reading features involves reading the project. It's like the project is a part of the features, while I'd expect the opposite: the features are part of the project. So I'd expect the GUI reads/writes the project ... and indirectly that implies reading/writing the features.
Comment on lines
+2396
to
+2398
| migrated = load_feature_streams_from_disk(feature, project_path) | ||
| if migrated: | ||
| save_features(project_path, self._tab_data_model.main.features.value) |
Member
There was a problem hiding this comment.
I think we should really try to make the migration transparent to the GUI. It can load the old version, but next time there is a write, it's automatically converted to the new version. No need to immediately save as a new version (and if we do so, it should nicely fallback in case writing fails).
K4rishma
reviewed
Mar 31, 2026
| self.assertFalse(os.path.exists(os.path.join(project_dir, "features.json"))) | ||
| r_features = read_features(project_dir) | ||
| self.assertEqual(len(features), len(r_features)) | ||
| self.assertEqual(features[0].name.value, r_features[0].name.value) |
Contributor
There was a problem hiding this comment.
Maybe this can be in the for loop?
| self.assertTrue(os.path.exists(filename)) | ||
|
|
||
| def test_load_feature_streams_does_not_fallback_with_project_state(self): | ||
| feature = CryoFeature("Feature-1", stage_position={"x": 0, "y": 0, "z": 0}, fm_focus_position={"z": 0}) |
Contributor
There was a problem hiding this comment.
docstring might be useful
|
|
||
| def add_stream_record(self, filename: str, stream_count: int) -> None: | ||
| """Register persisted stream links for one acquired file. | ||
|
|
Contributor
There was a problem hiding this comment.
Maybe the space can be removed, to be consistent with the docstring format?
| ) | ||
|
|
||
| def mark_stream_deleted(self, filename: str, stream_index: int) -> bool: | ||
| """Persistently mark one stream index as deleted. |
Contributor
There was a problem hiding this comment.
I feel docstring can be more descriptive for new functions. For e.g. here, it would be more clear if explanation is without the word persistently
| save_features(project_dir, features) | ||
| return features | ||
|
|
||
|
|
Contributor
There was a problem hiding this comment.
one line break convention between the two functions
| return records | ||
|
|
||
| def load_feature_streams_from_disk(feature: "CryoFeature", path: str) -> None: | ||
|
|
Contributor
There was a problem hiding this comment.
same here, the extra break line can be removed
| STREAM_INDEX_ATTR = "_project_stream_index" | ||
| STREAM_ORIGIN_FILENAME_MD = "Stream filename" | ||
| STREAM_ORIGIN_INDEX_MD = "Stream index" | ||
|
|
Contributor
There was a problem hiding this comment.
I think the function docstrings can be more descriptive by providing examples perhaps
| return normalized_record | ||
|
|
||
|
|
||
| def normalize_stream_records(records: List[Dict[str, Any]]) -> List[Dict[str, Any]]: |
Contributor
There was a problem hiding this comment.
the function call has the same name as in line 53, is this intentional?
| def get_state_filename(project_dir: str) -> str: | ||
| """Return the project-state filename for a project directory.""" | ||
| return os.path.join(project_dir, PROJECT_STATE_FILENAME) | ||
|
|
Contributor
There was a problem hiding this comment.
there should be one line between functions, please check the formatting
|
|
||
| def read_project_state(project_dir: str) -> Dict[str, Any]: | ||
| """Load global project state from disk. | ||
|
|
Contributor
There was a problem hiding this comment.
line break can be removed between the function description and the param description
tmoerkerken
changed the title
14 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Caution
Dot not merge. This PR was an attempt of a big refactor. After contemplation and discussion, I'm deciding to change the approach and to keep this as a reference. Replaced by: #3445
This PR replaces ad-hoc filename coupling with persistent project state for localization and fibsem tabs, and related stream handling. I left the correlation tab out of scope since that will be part of an upcoming related issue. I also discovered that there is a problem with user selected stream tint. I decided to leave it out of this PR (https://delmic.atlassian.net/browse/MSD-474)
Changes
project_state.jsonflow for stream/overview persistence.features.jsoninto the new state file.Demo
deletion_and_tint.mp4
NOTE: while recording this video, a colour persistence fix was in place. However, I found it to deviate too much from initial scope. So I removed that from this PR.
After the steps in the video, the feature was renamed from "Feature-1" to "Feature-renamed", and a single channel overview acquisition was obtained, after which the
project_state.jsonstate file for the example project looks like:After this a dual channel overview was obtained:
And one channel was deleted: