[feature][MSD-506] Meteor store annoted data - #3451
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an annotated data-collection pipeline for cryo features, including user consent management, S3 upload/download utilities, and a per-project sampling flag that is propagated to newly created CryoFeature instances and persisted in features.json.
Changes:
- Introduces a
DataCollectorframework with background serialization/upload to S3 and persistent consent configuration. - Adds GUI consent dialog + Help menu toggle, and wires per-project feature sampling into feature creation/loading.
- Adds an S3 “fetch samples” CLI and extensive unit/integration tests for the new utilities.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/odemis/util/datacollector.py |
New data-collection framework (config, background worker, S3 backend, serialization). |
src/odemis/util/dc_fetch.py |
New S3 retrieval helpers + CLI entrypoint implementation. |
scripts/odemis-dc-fetch.py |
Script wrapper for the S3 sample fetch CLI. |
src/odemis/gui/win/consent.py |
New consent dialog UI. |
src/odemis/gui/main.py |
Shows consent prompt on startup and injects DataCollector into menu controller. |
src/odemis/gui/cont/menu.py |
Adds Help menu checkbox to toggle data sharing consent. |
src/odemis/acq/feature.py |
Adds CryoFeature.collect flag + collection routine and helpers. |
src/odemis/gui/model/tab_gui_data.py |
Propagates per-session features_collectable flag into newly created CryoFeatures. |
src/odemis/gui/cont/tabs/cryo_chamber_tab.py |
Makes a per-project sampling decision and clears collect on loaded features. |
src/odemis/gui/cont/features.py |
Adds triggers for collection on status change, posture transitions, and feature deletion. |
src/odemis/util/test/datacollector_test.py |
New tests for config, serialization, queue limit, retry, and S3 integration (skipped when creds missing). |
src/odemis/util/test/dc_fetch_test.py |
New tests for S3 listing/pagination and download filtering logic. |
src/odemis/acq/test/feature_test.py / src/odemis/acq/test/test-features.json |
Updates feature JSON format and adds tests for collect + collection helpers. |
debian/control |
Adds python3-boto3 dependency. |
| consent_val = self.consent | ||
| remind_val = self.remind_date | ||
|
|
||
| if consent_val is True: | ||
| consent_line = "consent = true" | ||
| elif consent_val is False: | ||
| consent_line = "consent = false" | ||
| else: | ||
| consent_line = "consent = none" | ||
|
|
There was a problem hiding this comment.
DataCollectorConfig._write() writes consent = none when consent is unset, but DataCollectorConfig.consent reads the value using ConfigParser.getboolean(), which cannot parse none and will raise ValueError on the next load. This makes cfg.consent unusable after a save/load cycle when consent is undecided (e.g., after clear_consent() / postpone_consent()). Either omit/comment out the consent option when unset (and keep it absent in the file), or update the getter to explicitly treat none/empty as None (catch ValueError).
| # Limit event_name length so the filename stays within filesystem limits. | ||
| safe_event = item.event_name[:64] if item.event_name else "event" | ||
| zip_name = f"{safe_event}-{timestamp_str}-{uuid8}.zip" | ||
|
|
||
| tmp_dir = Path(tempfile.mkdtemp(prefix="dc_")) | ||
| try: | ||
| payload_meta: dict = {} | ||
| extra_files: list = [] # list of (arcname, abs_path) | ||
|
|
||
| for key, value in item.payload.items(): | ||
| if value is None or isinstance(value, (str, int, float, bool)): | ||
| payload_meta[key] = value | ||
|
|
||
| elif isinstance(value, numpy.ndarray): | ||
| if item.image_format.upper() == "HDF5": | ||
| arc_name = f"{key}.h5" | ||
| abs_path = tmp_dir / arc_name | ||
| try: | ||
| da = value if isinstance(value, model.DataArray) else model.DataArray(value) | ||
| hdf5.export(str(abs_path), da) | ||
| except Exception: | ||
| logging.exception("Failed to export DataArray to HDF5 at %s", abs_path) | ||
| abs_path = None | ||
| else: | ||
| arc_name = f"{key}.ome.tiff" | ||
| abs_path = tmp_dir / arc_name | ||
| try: | ||
| da = value if isinstance(value, model.DataArray) else model.DataArray(value) | ||
| tiff.export(str(abs_path), da) | ||
| except Exception: | ||
| logging.exception("Failed to export DataArray to TIFF at %s", abs_path) | ||
| abs_path = None | ||
|
|
||
| if abs_path is not None and abs_path.exists(): | ||
| extra_files.append((arc_name, abs_path)) | ||
| payload_meta[key] = arc_name | ||
| else: | ||
| payload_meta[key] = None | ||
| payload_meta["export_error"] = True | ||
|
|
||
| elif isinstance(value, (dict, list)): | ||
| arc_name = f"extra_{key}.json" | ||
| abs_path = tmp_dir / arc_name | ||
| abs_path.write_text(json.dumps(value, default=str), encoding="utf-8") | ||
| extra_files.append((arc_name, abs_path)) | ||
| payload_meta[key] = arc_name |
There was a problem hiding this comment.
event_name and payload keys are used directly to construct filenames inside _serialize() (ZIP name, extra_<key>.json, <key>.ome.tiff/.h5). Because DataCollector.record() allows arbitrary strings for event_name and payload keys, this enables path traversal (e.g., event_name='../../x' or payload key containing path separators) and could write/replace files outside queue_dir/tmp_dir. Sanitize event_name and all derived filenames to a safe character set and ensure the final resolved path stays within the intended directory before writing/renaming.
| try: | ||
| _dc = DataCollector() | ||
| if not _dc.get_consent(): | ||
| return |
There was a problem hiding this comment.
collect_feature_data() instantiates a new DataCollector() on every call. When consent is granted this will create a new background worker thread per invocation (and a new config instance), which can leak threads and increase CPU/memory usage—especially since collection is triggered from multiple GUI events and also runs inside separate threads already. Prefer reusing a single shared DataCollector instance (e.g., inject it from the GUI/controller, or use a module-level singleton) rather than constructing a new one each time.
There was a problem hiding this comment.
@K4rishma currently, gui/main.py creates a _data_collector, and keeps it for the rest of the GUI lifetime. I'd suggest to move it to the MainGUIData(). Then, pretty much every controller will be able to access it. You can have a single DataCollector instantiated. You can pass from the caller to this function.
| """Create S3 client and return `(client, bucket)`.""" | ||
| backend = config.get_upload_backend() | ||
| if not isinstance(backend, S3UploadBackend): | ||
| raise RuntimeError("Only S3 backend is supported for retrieval.") | ||
| # Accessing protected members intentionally to reuse existing backend setup. | ||
| client = backend._get_client() # pylint: disable=protected-access | ||
| bucket = backend._bucket # pylint: disable=protected-access | ||
| return client, bucket | ||
|
|
||
|
|
There was a problem hiding this comment.
create_s3_client_from_config() appears unused (the fetch path uses build_s3_client_from_config() instead). Consider removing it or using it consistently to avoid duplicate ways of building an S3 client and reduce maintenance surface.
| """Create S3 client and return `(client, bucket)`.""" | |
| backend = config.get_upload_backend() | |
| if not isinstance(backend, S3UploadBackend): | |
| raise RuntimeError("Only S3 backend is supported for retrieval.") | |
| # Accessing protected members intentionally to reuse existing backend setup. | |
| client = backend._get_client() # pylint: disable=protected-access | |
| bucket = backend._bucket # pylint: disable=protected-access | |
| return client, bucket | |
| """Create an S3 client from configuration. | |
| This wrapper is kept for compatibility and delegates to | |
| build_s3_client_from_config so there is a single implementation | |
| for S3 client construction. | |
| """ | |
| return build_s3_client_from_config(config) |
| # Feature whose status VA we are subscribed to for data-collection triggering. | ||
| self._status_collect_feature: Optional[CryoFeature] = None | ||
|
|
There was a problem hiding this comment.
Optional is used in the type annotation for _status_collect_feature, but it is not imported. Without from __future__ import annotations, this will raise a NameError at import time and break the GUI controller. Import Optional from typing (or switch to typing.Optional[...]).
|
Warning Review limit reached
Next review available in: 41 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe change adds a Sequence Diagram(s)sequenceDiagram
participant FeatureController
participant CryoFeature
participant collect_feature_data
participant DataCollector
FeatureController->>CryoFeature: detect eligible status or posture event
FeatureController->>collect_feature_data: start background collection
collect_feature_data->>CryoFeature: read feature and overview streams
collect_feature_data->>DataCollector: record anonymized payload
DataCollector-->>collect_feature_data: enqueue event
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 10
🧹 Nitpick comments (2)
src/odemis/gui/cont/menu.py (1)
46-54: Add type hints to the new consent menu API.The changed constructor and new handlers should annotate all parameters and return types.
♻️ Proposed fix
+from typing import Any + @@ - def __init__(self, main_data, main_frame, data_collector: DataCollector): + def __init__(self, main_data: Any, main_frame: wx.Frame, data_collector: DataCollector) -> None: @@ - def _append_data_sharing_menu_item(self, main_frame): + def _append_data_sharing_menu_item(self, main_frame: wx.Frame) -> wx.MenuItem | None: @@ - def _on_toggle_data_sharing(self, evt): + def _on_toggle_data_sharing(self, evt: wx.CommandEvent) -> None:As per coding guidelines,
**/*.py: Always use type hints for function parameters and return types in Python code.Also applies to: 171-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/menu.py` around lines 46 - 54, Annotate the constructor and the new consent menu handlers with explicit type hints: update __init__ to declare parameter types (e.g., main_data: MainGUIData, main_frame: wx.Frame, data_collector: DataCollector) and the return type -> None; then locate the consent-related handler functions referenced around lines 171-185 and add full parameter and return type annotations (e.g., event: wx.Event or appropriate event type -> None, any other params typed to their domain types). Ensure you import or reference the types (MainGUIData, DataCollector, wx.Frame/Event) at the top of the module so the annotations are valid.src/odemis/gui/win/consent.py (1)
16-59: Add docstrings for the new dialog methods.The event handlers and initializer are new functions and should follow the project docstring rule.
♻️ Proposed fix
def __init__(self, parent: wx.Window, remind_days: int) -> None: + """ + Initialize the consent dialog. + + :param parent: Parent window for the dialog. + :param remind_days: Number of days before prompting again. + """ title = "Share data with Delmic" @@ def _on_opt_in(self, _evt: wx.CommandEvent) -> None: + """ + Handle the opt-in button. + """ self.EndModal(self.RESULT_OPT_IN) def _on_opt_out(self, _evt: wx.CommandEvent) -> None: + """ + Handle the opt-out button. + """ self.EndModal(self.RESULT_OPT_OUT) def _on_remind_later(self, _evt: wx.CommandEvent) -> None: + """ + Handle the remind-later button. + """ self.EndModal(self.RESULT_REMIND_LATER) def _on_close(self, _evt: wx.CloseEvent) -> None: + """ + Handle closing the dialog without an explicit choice. + """ self.EndModal(self.RESULT_REMIND_LATER)As per coding guidelines,
**/*.py: Include docstrings for all functions and classes, following the reStructuredText style guide, without type information and without using inline formatting markers or backticks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/win/consent.py` around lines 16 - 59, Add reStructuredText docstrings to the Consent dialog methods: document the __init__ constructor and each event handler method (_on_opt_in, _on_opt_out, _on_remind_later, _on_close). For each docstring include a one-sentence description of the method's purpose and, where helpful, describe important parameters (e.g., evt) and the effect (which modal result is returned) using plain text (no type info, no inline code/backticks), following the project's reST style conventions.
🤖 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 573-584: The code currently sets channel_key = None when MD_OUT_WL
is missing, causing all such streams to collide and only the first to be kept;
change to use a per-stream fallback like the stream's position so missing
MD_OUT_WL yields a unique key. Iterate with an index over the concatenated list
(fm_zstacks + fm_images) and set channel_key =
s.raw[0].metadata.get(model.MD_OUT_WL, f"fallback:{i}") (or similar unique
identifier from the stream) before checking seen_channels; update references to
selected_fm and seen_channels accordingly.
- Around line 539-638: The race is that multiple threads can pass the initial if
not feature.collect check and all run until feature.collect is set False at the
end; make the one-shot flip atomic by performing an immediate compare-and-set at
the start of the routine (e.g., in collect_feature_data): replace the plain
boolean check of feature.collect with an atomic operation that sets
feature.collect to False only if it was True (or acquire a per-feature lock
keyed by feature id, check feature.collect and set it False while holding the
lock), and if the CAS/lock indicates collect was already False return early;
reference feature.collect and the top-level routine (collect_feature_data in
this file) when adding the atomic CAS or lock.
In `@src/odemis/acq/test/feature_test.py`:
- Around line 349-365: The test test_payload_has_no_feature_name only checks
that f.name.value ("my_secret_feature_name") is not present among payload keys;
update the assertions to also ensure the feature name does not appear in payload
values by checking captured.values() and the stringified values (for
nested/serialized values) after collect_feature_data runs — e.g., add assertions
using self.assertNotIn("my_secret_feature_name", captured.values()) and
self.assertNotIn("my_secret_feature_name", str(list(captured.values()))) to the
test (keep existing fake_record/captured usage and collect_feature_data
invocation).
In `@src/odemis/gui/cont/features.py`:
- Line 31: The module imports Dict and List from typing but uses Optional in the
annotation for self._status_collect_feature (type Optional[CryoFeature]) which
is undefined; update the import statement that currently lists Dict and List to
also import Optional so Optional is available for the annotation (refer to the
import line and the attribute self._status_collect_feature / CryoFeature usage
to locate the change).
In `@src/odemis/gui/cont/tabs/cryo_chamber_tab.py`:
- Around line 388-389: The code sets
self.tab_data_model.main.features_collectable using random.random() and
FEATURE_COLLECT_PROBABILITY but those names are not defined; import the random
module at the top of this module and add a module-level constant
FEATURE_COLLECT_PROBABILITY (e.g., a float like 0.1) before it’s used so
_change_project_conf() and any code referencing features_collectable (and
class/attribute names like tab_data_model.main.features_collectable) do not
raise NameError.
In `@src/odemis/gui/main.py`:
- Around line 436-441: The consent dialog updates consent via
self._data_collector.set_consent(...) but the Help > Share data menu checkbox
isn't refreshed; add a helper method refresh_data_sharing_state(self) in the
menu controller (e.g. class in src/odemis/gui/cont/menu.py) that does if
self._consent_menu_item is not None:
self._consent_menu_item.Check(self._data_collector.get_consent() is True), then
call that helper after the dialog result handling in main.py (after the branches
that call set_consent(True/False) or postpone_consent()) so the menu checkbox
reflects the new persisted consent state immediately.
In `@src/odemis/util/datacollector.py`:
- Around line 588-604: The loop currently does "if
self._process_pending_zips(...): continue" which starves new in-memory records;
change _run so that when _process_pending_zips(...) returns True you still
attempt to drain or process queued items instead of skipping the queue.get()
step — e.g., replace the continue with a non-blocking attempt to fetch work (use
self._queue.get_nowait() in a try/except queue.Empty or
self._queue.get(timeout=0.0)) and call self._process_work_item(item) if you get
one; keep the existing exception handling (_schedule_retry, logging.exception)
and avoid a tight busy-loop by falling back to the original blocking
self._queue.get(timeout=1.0) when no items are available.
- Around line 328-373: The event_name and payload keys are used directly to
build filesystem names (safe_event, zip_name, arc_name, extra_files paths) which
allows path traversal or unsafe ZIP entries; add a sanitization step that
normalizes item.event_name and each payload key into safe filenames (strip or
replace path separators like "/" and "\" and sequences like "..", collapse to a
whitelist of allowed chars such as alphanumerics, hyphen, underscore, enforce a
max length like 64) before using them to construct zip_name, tmp_dir children,
or arc_name; apply this sanitizer to safe_event, every arc_name (e.g., when
creating extra_{key}.json or {key}.h5/.ome.tiff) and when writing to tmp_dir or
adding to extra_files so no user-supplied string can escape tmp_dir or create
dangerous ZIP entries (update references in the code around safe_event,
zip_name, arc_name, payload_meta assignments, and extra_files population).
In `@src/odemis/util/dc_fetch.py`:
- Around line 40-47: The ISO parse path must normalize a trailing 'Z' before
calling datetime.fromisoformat to maintain Python 3.10 compatibility: in the
code handling text (the branch that calls datetime.fromisoformat(text)), detect
and replace a trailing 'Z' (or '+00:00' equivalent if present) with '+00:00' or
otherwise remove it so fromisoformat won't raise ValueError, then proceed to set
tzinfo to timezone.utc when parsed.tzinfo is None and use
parsed.astimezone(timezone.utc) when it has tzinfo; update the logic around the
parsed = datetime.fromisoformat(text) call accordingly.
---
Nitpick comments:
In `@src/odemis/gui/cont/menu.py`:
- Around line 46-54: Annotate the constructor and the new consent menu handlers
with explicit type hints: update __init__ to declare parameter types (e.g.,
main_data: MainGUIData, main_frame: wx.Frame, data_collector: DataCollector) and
the return type -> None; then locate the consent-related handler functions
referenced around lines 171-185 and add full parameter and return type
annotations (e.g., event: wx.Event or appropriate event type -> None, any other
params typed to their domain types). Ensure you import or reference the types
(MainGUIData, DataCollector, wx.Frame/Event) at the top of the module so the
annotations are valid.
In `@src/odemis/gui/win/consent.py`:
- Around line 16-59: Add reStructuredText docstrings to the Consent dialog
methods: document the __init__ constructor and each event handler method
(_on_opt_in, _on_opt_out, _on_remind_later, _on_close). For each docstring
include a one-sentence description of the method's purpose and, where helpful,
describe important parameters (e.g., evt) and the effect (which modal result is
returned) using plain text (no type info, no inline code/backticks), following
the project's reST style conventions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2aab52b5-1622-44b1-9a6a-a474e984fe44
📒 Files selected for processing (15)
debian/controlscripts/odemis-dc-fetch.pysrc/odemis/acq/feature.pysrc/odemis/acq/test/feature_test.pysrc/odemis/acq/test/test-features.jsonsrc/odemis/gui/cont/features.pysrc/odemis/gui/cont/menu.pysrc/odemis/gui/cont/tabs/cryo_chamber_tab.pysrc/odemis/gui/main.pysrc/odemis/gui/model/tab_gui_data.pysrc/odemis/gui/win/consent.pysrc/odemis/util/datacollector.pysrc/odemis/util/dc_fetch.pysrc/odemis/util/test/datacollector_test.pysrc/odemis/util/test/dc_fetch_test.py
| if not feature.collect: | ||
| return | ||
|
|
||
| try: | ||
| _dc = DataCollector() | ||
| if not _dc.get_consent(): | ||
| return | ||
| except Exception: | ||
| logging.exception("collect_feature_data: failed to access DataCollector; skipping.") | ||
| return | ||
|
|
||
| try: | ||
|
|
||
| # Load feature streams from disk when not yet in memory. | ||
| if not feature.streams.value and project_dir: | ||
| try: | ||
| load_feature_streams_from_disk(feature, project_dir) | ||
| except Exception: | ||
| logging.exception( | ||
| "collect_feature_data: failed to load streams; skipping.") | ||
| return | ||
|
|
||
| feature_streams = list(feature.streams.value) | ||
|
|
||
| # Collect first z-stack per FM channel; fall back to first FM image per channel. | ||
| fm_zstacks: List = [] | ||
| fm_images: List = [] | ||
| for s in feature_streams: | ||
| if isinstance(s, StaticFluoStream): | ||
| if _is_zstack_stream(s): | ||
| fm_zstacks.append(s) | ||
| else: | ||
| fm_images.append(s) | ||
|
|
||
| # Per channel: prefer z-stack, then plain FM image. | ||
| # Channels are delineated by MD_OUT_WL; use index as fallback. | ||
| selected_fm: List = [] | ||
| seen_channels: set = set() | ||
| for s in fm_zstacks + fm_images: | ||
| try: | ||
| channel_key = s.raw[0].metadata.get(model.MD_OUT_WL) | ||
| except (IndexError, AttributeError): | ||
| channel_key = None | ||
| if channel_key not in seen_channels: | ||
| seen_channels.add(channel_key) | ||
| selected_fm.append(s) | ||
|
|
||
| # Collect spatially overlapping overview streams. | ||
| stage_pos = feature.stage_position.value | ||
| feat_x = stage_pos.get("x", 0.0) | ||
| feat_y = stage_pos.get("y", 0.0) | ||
|
|
||
| overview_fm: List = [] | ||
| overview_sem: List = [] | ||
| for s in (overview_streams or []): | ||
| if not _stream_overlaps_position(s, feat_x, feat_y): | ||
| continue | ||
| if isinstance(s, StaticFluoStream): | ||
| overview_fm.append(s) | ||
| elif isinstance(s, (StaticSEMStream, StaticFIBStream)): | ||
| overview_sem.append(s) | ||
|
|
||
| # Build privacy-preserving payload — generic keys, no names or filenames. | ||
| payload: dict = { | ||
| "status": feature.status.value, | ||
| "stage_position": dict(stage_pos), | ||
| "fm_focus_position": dict(feature.fm_focus_position.value), | ||
| } | ||
|
|
||
| def _get_raw(stream: "Stream") -> Optional["model.DataArray"]: | ||
| try: | ||
| return stream.raw[0] if stream.raw else None | ||
| except Exception: | ||
| return None | ||
|
|
||
| for idx, s in enumerate(selected_fm): | ||
| da = _get_raw(s) | ||
| if da is not None: | ||
| payload[f"channel_{idx}"] = da | ||
|
|
||
| for idx, s in enumerate(overview_fm): | ||
| da = _get_raw(s) | ||
| if da is not None: | ||
| payload[f"overview_fm_{idx}"] = da | ||
|
|
||
| for idx, s in enumerate(overview_sem): | ||
| da = _get_raw(s) | ||
| if da is not None: | ||
| payload[f"overview_sem_{idx}"] = da | ||
|
|
||
| image_keys = [k for k in payload if k.startswith(("channel_", "overview_fm_", "overview_sem_"))] | ||
| if not image_keys: | ||
| logging.debug( | ||
| "collect_feature_data: no images found for feature; skipping upload." | ||
| ) | ||
| return | ||
|
|
||
| _dc.record("feature_collected", "1.0", payload) | ||
|
|
||
| feature.collect = False |
There was a problem hiding this comment.
Make the one-shot collect transition atomic.
Concurrent status/posture/delete triggers can all pass Line 539 before Line 638 flips feature.collect, causing duplicate submissions for the same feature.
Proposed fix
FEATURE_COLLECT_PROBABILITY = 0.2
+_COLLECTION_STATE_LOCK = threading.Lock()- _dc.record("feature_collected", "1.0", payload)
-
- feature.collect = False
+ with _COLLECTION_STATE_LOCK:
+ if not feature.collect:
+ return
+ feature.collect = False
+
+ try:
+ _dc.record("feature_collected", "1.0", payload)
+ except Exception:
+ with _COLLECTION_STATE_LOCK:
+ feature.collect = True
+ raise
logging.debug("collect_feature_data: submitted feature data for collection.")🧰 Tools
🪛 Ruff (0.15.10)
[warning] 611-611: Do not catch blind exception: Exception
(BLE001)
🤖 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 539 - 638, The race is that multiple
threads can pass the initial if not feature.collect check and all run until
feature.collect is set False at the end; make the one-shot flip atomic by
performing an immediate compare-and-set at the start of the routine (e.g., in
collect_feature_data): replace the plain boolean check of feature.collect with
an atomic operation that sets feature.collect to False only if it was True (or
acquire a per-feature lock keyed by feature id, check feature.collect and set it
False while holding the lock), and if the CAS/lock indicates collect was already
False return early; reference feature.collect and the top-level routine
(collect_feature_data in this file) when adding the atomic CAS or lock.
| def test_payload_has_no_feature_name(self): | ||
| """Payload must not contain the feature name string as a key or value.""" | ||
| f = self._make_feature_with_stream(collect=True) | ||
| f.name.value = "my_secret_feature_name" | ||
| captured = {} | ||
|
|
||
| def fake_record(event_name, schema_version, payload, **kwargs): | ||
| captured.update(payload) | ||
|
|
||
| with patch("odemis.acq.feature.DataCollector") as MockDC: | ||
| MockDC.return_value.get_consent.return_value = True | ||
| MockDC.return_value.record.side_effect = fake_record | ||
| collect_feature_data(f) | ||
|
|
||
| self.assertNotIn("my_secret_feature_name", captured) | ||
| self.assertNotIn("my_secret_feature_name", str(captured.keys())) | ||
|
|
There was a problem hiding this comment.
Assert the feature name is absent from payload values too.
The docstring says “as a key or value,” but the assertions only inspect keys. A leaked feature name in "status", metadata, or another value would not fail this test.
Proposed fix
self.assertNotIn("my_secret_feature_name", captured)
self.assertNotIn("my_secret_feature_name", str(captured.keys()))
+ self.assertNotIn("my_secret_feature_name", str(captured.values()))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/acq/test/feature_test.py` around lines 349 - 365, The test
test_payload_has_no_feature_name only checks that f.name.value
("my_secret_feature_name") is not present among payload keys; update the
assertions to also ensure the feature name does not appear in payload values by
checking captured.values() and the stringified values (for nested/serialized
values) after collect_feature_data runs — e.g., add assertions using
self.assertNotIn("my_secret_feature_name", captured.values()) and
self.assertNotIn("my_secret_feature_name", str(list(captured.values()))) to the
test (keep existing fake_record/captured usage and collect_feature_data
invocation).
510e3ab to
8f0709c
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/odemis/util/dc_fetch.py (2)
179-264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
default_endpointcomputation.
default_endpoint = "" if S3_ENDPOINT_URL is None else str(S3_ENDPOINT_URL)is duplicated verbatim in_load_or_init_dc_fetch_config(line 190) andbuild_s3_client_from_config(line 242). Extracting a tiny helper (or module-level constant) would avoid the two copies diverging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/util/dc_fetch.py` around lines 179 - 264, Remove the duplicated default endpoint computation shared by _load_or_init_dc_fetch_config and build_s3_client_from_config. Extract it into a single module-level constant or small helper, then reuse that shared value in both functions while preserving the existing None-to-empty-string behavior.
58-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate key-parsing logic between the two parsers.
parse_key_timestamp_utcandparse_key_event_nameboth split the basename withstem.rsplit("-", 2)and re-validatelen(parts) != 3. Consider extracting a shared_parse_key_stem(key) -> Optional[Tuple[str, str, str]]helper to avoid drift between the two implementations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/util/dc_fetch.py` around lines 58 - 104, Extract the shared basename, ZIP-suffix, stem-splitting, and part-count validation from parse_key_timestamp_utc and parse_key_event_name into a _parse_key_stem helper returning the three parsed components or None. Update both parsers to reuse this helper while preserving their existing timestamp validation and event-name handling.src/odemis/util/test/dc_fetch_test.py (1)
77-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
skipped_existingandfailedbranches offetch_samples.Tests cover the happy path, host-prefix listing, and override forwarding, but not the "destination already exists" skip path or the download-exception/
.partcleanup path infetch_samples(src/odemis/util/dc_fetch.py lines 315-328). These are meaningful branches on a function flagged as high complexity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/util/test/dc_fetch_test.py` around lines 77 - 172, Add tests for the skipped_existing and failed branches in fetch_samples. Create a pre-existing destination file and assert it is skipped and counted without downloading; separately make client.download_file raise an exception, then assert failed is incremented and the temporary .part file is removed. Reuse the existing mocked S3 client and temporary-directory setup in the fetch_samples tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/odemis/acq/feature.py`:
- Around line 614-627: Inspect DataCollector.record() and the metadata produced
by _get_raw() to determine which identifying fields survive export. Before
assigning DataArrays into payload, sanitize their metadata by removing
stream/channel names, filenames, acquisition-path fields, and other identifying
keys, while preserving non-identifying metadata and avoiding mutation of the
original raw data. Apply this consistently to selected_fm, overview_fm, and
overview_sem.
In `@src/odemis/util/datacollector.py`:
- Around line 343-361: In the ndarray export branch, update the archive name
construction to concatenate exporter.EXTENSIONS[0] directly without adding
another dot, and replace the hardcoded tiff.export call with the selected
exporter’s export method. Keep the existing format selection and error handling
unchanged.
In `@src/odemis/util/dc_fetch.py`:
- Around line 163-177: Update _write_dc_fetch_config to create or overwrite the
credentials INI with owner-only permissions (0600), rather than relying on the
process umask and plain "w" open. Preserve directory creation and config.write
behavior, ensuring existing files are also chmodded to 0600.
In `@util/release-odemis`:
- Around line 35-40: Update the source tarball generation around the git archive
HEAD command so the locally verified datacollector.key is explicitly added at
install/linux/usr/share/odemis/datacollector.key before compression. Preserve
the expected archive path and ensure the injected key is present in the
.orig.tar.gz uploaded to Launchpad.
---
Nitpick comments:
In `@src/odemis/util/dc_fetch.py`:
- Around line 179-264: Remove the duplicated default endpoint computation shared
by _load_or_init_dc_fetch_config and build_s3_client_from_config. Extract it
into a single module-level constant or small helper, then reuse that shared
value in both functions while preserving the existing None-to-empty-string
behavior.
- Around line 58-104: Extract the shared basename, ZIP-suffix, stem-splitting,
and part-count validation from parse_key_timestamp_utc and parse_key_event_name
into a _parse_key_stem helper returning the three parsed components or None.
Update both parsers to reuse this helper while preserving their existing
timestamp validation and event-name handling.
In `@src/odemis/util/test/dc_fetch_test.py`:
- Around line 77-172: Add tests for the skipped_existing and failed branches in
fetch_samples. Create a pre-existing destination file and assert it is skipped
and counted without downloading; separately make client.download_file raise an
exception, then assert failed is incremented and the temporary .part file is
removed. Reuse the existing mocked S3 client and temporary-directory setup in
the fetch_samples tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aef70cfb-8c08-4999-a52e-fa80b6198dc4
📒 Files selected for processing (19)
.gitignoredebian/odemis.installdebian/rulesdoc/develop/data-framework-setup-guide.rstdoc/develop/index.rstsrc/odemis/acq/feature.pysrc/odemis/acq/test/feature_test.pysrc/odemis/acq/test/test-features.jsonsrc/odemis/gui/cont/features.pysrc/odemis/gui/cont/menu.pysrc/odemis/gui/cont/tabs/cryo_chamber_tab.pysrc/odemis/gui/main.pysrc/odemis/gui/model/tab_gui_data.pysrc/odemis/util/datacollector.pysrc/odemis/util/dc_fetch.pysrc/odemis/util/test/datacollector_test.pysrc/odemis/util/test/dc_fetch_test.pyutil/odemis-dc-fetch.pyutil/release-odemis
💤 Files with no reviewable changes (1)
- src/odemis/gui/main.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/odemis/acq/test/test-features.json
- src/odemis/gui/model/tab_gui_data.py
- src/odemis/gui/cont/menu.py
| elif isinstance(value, numpy.ndarray): | ||
| if item.image_format.upper() == "HDF5": | ||
| exporter = hdf5 | ||
| elif item.image_format.upper() == "TIFF": | ||
| exporter = tiff | ||
| else: | ||
| logging.warning("DataArray not in valid format", exc_info=True) | ||
| exporter = None | ||
|
|
||
| if exporter is not None: | ||
| ext = exporter.EXTENSIONS[0] | ||
| arc_name = f"{_sanitize_filename(key)}.{ext}" | ||
| abs_path = tmp_dir / arc_name | ||
| try: | ||
| da = value if isinstance(value, model.DataArray) else model.DataArray(value) | ||
| tiff.export(str(abs_path), da) | ||
| except Exception: | ||
| logging.warning("Failed to export DataArray to %s at %s", ext, abs_path, exc_info=True) | ||
| abs_path = None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f -e py '^(tiff|hdf5)\.py$' src/odemis/dataio
rg -nP '^\s*EXTENSIONS\s*=' src/odemis/dataio/tiff.py src/odemis/dataio/hdf5.py
rg -nP '^\s*def export\s*\(' src/odemis/dataio/tiff.py src/odemis/dataio/hdf5.pyRepository: delmic/odemis
Length of output: 465
🏁 Script executed:
#!/bin/bash
sed -n '330,370p' src/odemis/util/datacollector.py
sed -n '1,120p' src/odemis/dataio/hdf5.py
sed -n '60,100p' src/odemis/dataio/tiff.pyRepository: delmic/odemis
Length of output: 8490
Fix ndarray export to use the selected encoder and correct the archive name
tiff.export(...)is hardcoded even whenitem.image_format == "HDF5", so HDF5 arrays are written with the wrong exporter.exporter.EXTENSIONS[0]already includes the leading dot, sof"{_sanitize_filename(key)}.{ext}"produces names likekey..h5. Concatenateextdirectly.
Proposed fix
- arc_name = f"{_sanitize_filename(key)}.{ext}"
+ arc_name = f"{_sanitize_filename(key)}{ext}"
...
- tiff.export(str(abs_path), da)
+ exporter.export(str(abs_path), da)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| elif isinstance(value, numpy.ndarray): | |
| if item.image_format.upper() == "HDF5": | |
| exporter = hdf5 | |
| elif item.image_format.upper() == "TIFF": | |
| exporter = tiff | |
| else: | |
| logging.warning("DataArray not in valid format", exc_info=True) | |
| exporter = None | |
| if exporter is not None: | |
| ext = exporter.EXTENSIONS[0] | |
| arc_name = f"{_sanitize_filename(key)}.{ext}" | |
| abs_path = tmp_dir / arc_name | |
| try: | |
| da = value if isinstance(value, model.DataArray) else model.DataArray(value) | |
| tiff.export(str(abs_path), da) | |
| except Exception: | |
| logging.warning("Failed to export DataArray to %s at %s", ext, abs_path, exc_info=True) | |
| abs_path = None | |
| elif isinstance(value, numpy.ndarray): | |
| if item.image_format.upper() == "HDF5": | |
| exporter = hdf5 | |
| elif item.image_format.upper() == "TIFF": | |
| exporter = tiff | |
| else: | |
| logging.warning("DataArray not in valid format", exc_info=True) | |
| exporter = None | |
| if exporter is not None: | |
| ext = exporter.EXTENSIONS[0] | |
| arc_name = f"{_sanitize_filename(key)}{ext}" | |
| abs_path = tmp_dir / arc_name | |
| try: | |
| da = value if isinstance(value, model.DataArray) else model.DataArray(value) | |
| exporter.export(str(abs_path), da) | |
| except Exception: | |
| logging.warning("Failed to export DataArray to %s at %s", ext, abs_path, exc_info=True) | |
| abs_path = None |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 359-359: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/util/datacollector.py` around lines 343 - 361, In the ndarray
export branch, update the archive name construction to concatenate
exporter.EXTENSIONS[0] directly without adding another dot, and replace the
hardcoded tiff.export call with the selected exporter’s export method. Keep the
existing format selection and error handling unchanged.
| def _write_dc_fetch_config( | ||
| config: configparser.ConfigParser, | ||
| config_path: Path, | ||
| ) -> None: | ||
| """ | ||
| Write dc_fetch INI config to disk, creating parent directories if needed. | ||
|
|
||
| :param config: Parsed configuration object. | ||
| :param config_path: Destination INI file path. | ||
| :return: None. | ||
| """ | ||
| config_path.parent.mkdir(parents=True, exist_ok=True) | ||
| with config_path.open("w", encoding="utf-8") as fp: | ||
| config.write(fp) | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Restrict permissions on the credentials INI file.
_write_dc_fetch_config writes access_key/secret_key to dc_fetch.ini in plaintext via a plain "w" open, so the file inherits the process umask (commonly 0644, world-readable). Any other local user could read these S3 credentials. AWS's own CLI creates ~/.aws/credentials with 0600 permissions for this reason.
🔒 Proposed fix
+import os
+
def _write_dc_fetch_config(
config: configparser.ConfigParser,
config_path: Path,
) -> None:
config_path.parent.mkdir(parents=True, exist_ok=True)
with config_path.open("w", encoding="utf-8") as fp:
config.write(fp)
+ config_path.chmod(0o600)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _write_dc_fetch_config( | |
| config: configparser.ConfigParser, | |
| config_path: Path, | |
| ) -> None: | |
| """ | |
| Write dc_fetch INI config to disk, creating parent directories if needed. | |
| :param config: Parsed configuration object. | |
| :param config_path: Destination INI file path. | |
| :return: None. | |
| """ | |
| config_path.parent.mkdir(parents=True, exist_ok=True) | |
| with config_path.open("w", encoding="utf-8") as fp: | |
| config.write(fp) | |
| import os | |
| def _write_dc_fetch_config( | |
| config: configparser.ConfigParser, | |
| config_path: Path, | |
| ) -> None: | |
| """ | |
| Write dc_fetch INI config to disk, creating parent directories if needed. | |
| :param config: Parsed configuration object. | |
| :param config_path: Destination INI file path. | |
| :return: None. | |
| """ | |
| config_path.parent.mkdir(parents=True, exist_ok=True) | |
| with config_path.open("w", encoding="utf-8") as fp: | |
| config.write(fp) | |
| config_path.chmod(0o600) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/util/dc_fetch.py` around lines 163 - 177, Update
_write_dc_fetch_config to create or overwrite the credentials INI with
owner-only permissions (0600), rather than relying on the process umask and
plain "w" open. Preserve directory creation and config.write behavior, ensuring
existing files are also chmodded to 0600.
| # Ensure datacollector key is present in the build tree before any release action. | ||
| if [ ! -f ~/development/pkg-native/odemis/install/linux/usr/share/odemis/datacollector.key ]; then | ||
| echo "Missing required file: ~/development/pkg-native/odemis/install/linux/usr/share/odemis/datacollector.key" | ||
| exit 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
The locally verified key will not be included in the Launchpad source package.
While this check ensures the datacollector.key is present in the local build tree, the key will not be included in the source package uploaded to the PPA. At line 125, the script uses git archive HEAD to generate the .orig.tar.gz source tarball. Since git archive only includes committed files and datacollector.key is untracked (ignored in .gitignore), the key will be silently omitted from the archive.
When Launchpad attempts to build the package from source, dh_install will fail to find install/linux/usr/share/odemis/datacollector.key in the extracted tree (or silently omit it, depending on the debhelper compat level), resulting in a broken release artifact.
To fix this, you must explicitly inject the untracked key into the tarball before compressing it.
🐛 Proposed fix to inject the key into the source tarball
Update line 125 to inject the key into the tarball:
-git archive --prefix=odemis/ -o ../odemis_${RELVER}.orig.tar.gz HEAD || exit 1
+git archive --prefix=odemis/ -o ../odemis_${RELVER}.orig.tar HEAD || exit 1
+# Inject the untracked key into the tar archive before compressing
+tar -rf ../odemis_${RELVER}.orig.tar --transform 's,^,odemis/,' install/linux/usr/share/odemis/datacollector.key || exit 1
+gzip -f9 ../odemis_${RELVER}.orig.tar || exit 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@util/release-odemis` around lines 35 - 40, Update the source tarball
generation around the git archive HEAD command so the locally verified
datacollector.key is explicitly added at
install/linux/usr/share/odemis/datacollector.key before compression. Preserve
the expected archive path and ensure the injected key is present in the
.orig.tar.gz uploaded to Launchpad.
7900a29 to
abef264
Compare
abef264 to
e1749e0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
src/odemis/gui/cont/tabs/cryo_chamber_tab.py:531
feature_decoder()can return None; iteratingdecoded_featuresand unconditionally doingf.collect = Falsewill raise AttributeError when a None is present (and the subsequent assignment already filters None out).
decoded_features = [feature_decoder(f) for f in proj_data["features"]]
for f in decoded_features:
f.collect = False
self.tab_data_model.main.features.value = [df for df in decoded_features if df is not None]
src/odemis/gui/cont/tabs/cryo_chamber_tab.py:399
- This creates a new DataCollector instance just to read
probability. In the GUI, the app already owns a long-lived DataCollector instance (wx.GetApp()._data_collector), so this risks spinning up extra background workers and (becauseprobabilityis only updated inside record()) it will almost always read the default value. Also, the PR description states 20% sampling, but DataCollector’s default probability is currently 10%, so this decision won’t match the described behavior.
This issue also appears on line 528 of the same file.
# Decide once per project whether features created during this session are
# eligible for data collection. Stored as a dynamic attribute — not part
# of the formal model — and read by add_new_feature via getattr.
probability = DataCollector().probability
self.tab_data_model.main.features_collectable = (
src/odemis/acq/feature.py:313
feature_decoder()now reads thecollectfield from JSON, but project serialization does not currently writecollect(seeodemis/gui/cont/cryo_project.py:133-147). As a result,collectwill always default to False after a save/load cycle, contradicting the intended persistence behavior described in the PR.
collect = feature_raw.get('collect', False)
feature = CryoFeature(name=feature_raw['name'],
stage_position=stage_position,
fm_focus_position=fm_focus_position,
collect=collect
| # Track previous posture so we can detect FM → SEM/FIB transitions. | ||
| self._prev_posture = self.pm.getCurrentPostureLabel() if self.pm else None |
| if days_left is not None and days_left <= 1: | ||
| probability = _FULL_COLLECTION_PROBABILITY | ||
| self.probability = _FULL_COLLECTION_PROBABILITY | ||
| else: | ||
| probability = _DEFAULT_COLLECTION_PROBABILITY | ||
|
|
||
| if random.random() >= probability: | ||
| logging.debug( | ||
| "DataCollector: event '%s' not sampled (%.0f%% collection probability).", | ||
| event_name, probability * 100, | ||
| ) | ||
| return | ||
| self.probability = _DEFAULT_COLLECTION_PROBABILITY | ||
|
|
There was a problem hiding this comment.
@K4rishma I agree here that it doesn't make sense to re-update .probability every time .record() is called. It should be done at init only then, or when the consent date changes.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/odemis/acq/feature.py (1)
539-552: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Reachability path
● Entry src/odemis/gui/cont/tabs/cryo_chamber_tab.py:39 feature_decoder │ ▼ ● Sink src/odemis/acq/feature.pyRedact identifying
DataArraymetadata before export.
_export_data()writes the feature name intomodel.MD_DESCRIPTIONbefore persisting an acquisition. Lines 539-552 then pass the originalDataArrayobjects toDataCollector. Generic payload keys do not remove embedded metadata. If the image exporter retainsMD_DESCRIPTION, a collectable feature with identifying metadata can upload that identifier.Clone and sanitize each payload
DataArraywithout mutating feature data. Add an artifact-level regression test.Based on prior review context, this is a recurrence of the metadata-redaction concern.
#!/bin/bash set -euo pipefail ast-grep outline src/odemis/acq/feature.py --items all --type function --view expanded sed -n '432,565p' src/odemis/acq/feature.py sed -n '905,922p' src/odemis/acq/feature.py ast-grep outline src/odemis/util/datacollector.py --items all --type function --view expanded sed -n '314,405p' src/odemis/util/datacollector.py ast-grep outline src/odemis/dataio/tiff.py --items all --type function --view expanded rg -n -C 5 'def export|MD_DESCRIPTION|metadata' src/odemis/dataio/tiff.py🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/feature.py` around lines 539 - 552, Update the payload-building loops in _export_data to clone each non-null DataArray and remove identifying metadata, including model.MD_DESCRIPTION, before assigning it to payload. Preserve the original feature DataArrays unchanged and apply the sanitization consistently to selected_fm, overview_fm, and overview_sem; add an artifact-level regression test covering metadata redaction during export.
🧹 Nitpick comments (1)
src/odemis/acq/feature.py (1)
174-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete annotations and documentation for new Python callables.
The new callables have incomplete parameter or return annotations. Several local test callbacks also lack docstrings. The changed
CryoFeaturedocumentation includes type text despite the no-type-information requirement.
src/odemis/acq/feature.py#L174-L189: annotatecorrelation_data, add-> None, and remove type text from the changed docstring.src/odemis/acq/test/feature_test.py#L97-L292: add complete annotations and concise docstrings to added tests, helpers, and callbacks.src/odemis/gui/cont/tabs/cryo_chamber_tab.py#L385-L404: annotate_change_project_conf(new_dir: str) -> None.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.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/feature.py` around lines 174 - 189, Complete callable annotations and documentation across the three affected sites: in src/odemis/acq/feature.py lines 174-189, annotate CryoFeature.__init__ correlation_data with its appropriate type, add a None return annotation, and remove type text from the changed docstring; in src/odemis/acq/test/feature_test.py lines 97-292, add complete parameter and return annotations plus concise docstrings to all added tests, helpers, and callbacks; in src/odemis/gui/cont/tabs/cryo_chamber_tab.py lines 385-404, update _change_project_conf to accept new_dir: str and return None.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/odemis/acq/feature.py`:
- Around line 471-475: Update the load_feature_streams_from_disk call in the
feature stream-loading block to pass only the supported feature argument, while
preserving the existing exception handling and collection flow.
In `@src/odemis/gui/cont/tabs/cryo_chamber_tab.py`:
- Around line 396-399: Use a dedicated 20% sampling constant or initialized
query method in cryo_chamber_tab.py lines 396-399 for the per-project decision.
In datacollector.py lines 659-661, stop exposing the fixed default as a
ready-to-use computed rate; in datacollector.py lines 792-795, derive the rate
within the query method when consent-dependent sampling is required.
- Around line 528-531: Update the decoded-features handling around
feature_decoder so None results are filtered out before the loop mutates each
feature. Ensure f.collect is assigned only for valid decoded features, then
assign the filtered collection to main.features.value while preserving the
existing behavior for invalid entries.
---
Duplicate comments:
In `@src/odemis/acq/feature.py`:
- Around line 539-552: Update the payload-building loops in _export_data to
clone each non-null DataArray and remove identifying metadata, including
model.MD_DESCRIPTION, before assigning it to payload. Preserve the original
feature DataArrays unchanged and apply the sanitization consistently to
selected_fm, overview_fm, and overview_sem; add an artifact-level regression
test covering metadata redaction during export.
---
Nitpick comments:
In `@src/odemis/acq/feature.py`:
- Around line 174-189: Complete callable annotations and documentation across
the three affected sites: in src/odemis/acq/feature.py lines 174-189, annotate
CryoFeature.__init__ correlation_data with its appropriate type, add a None
return annotation, and remove type text from the changed docstring; in
src/odemis/acq/test/feature_test.py lines 97-292, add complete parameter and
return annotations plus concise docstrings to all added tests, helpers, and
callbacks; in src/odemis/gui/cont/tabs/cryo_chamber_tab.py lines 385-404, update
_change_project_conf to accept new_dir: str and return None.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 01e94947-f5b3-4c6f-927e-ebfe749b5f59
📒 Files selected for processing (7)
src/odemis/acq/feature.pysrc/odemis/acq/test/feature_test.pysrc/odemis/acq/test/test-features.jsonsrc/odemis/gui/cont/features.pysrc/odemis/gui/cont/tabs/cryo_chamber_tab.pysrc/odemis/gui/model/tab_gui_data.pysrc/odemis/util/datacollector.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/odemis/acq/test/test-features.json
- src/odemis/gui/model/tab_gui_data.py
- src/odemis/gui/cont/features.py
| if not feature.streams.value and project_dir: | ||
| try: | ||
| load_feature_streams_from_disk(feature, project_dir) | ||
| except Exception: | ||
| logging.warning("Failed to load streams from disk; skipping collection", exc_info=True) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pass only the supported loader arguments.
load_feature_streams_from_disk() accepts only feature. Line 473 passes project_dir too. This raises TypeError, which the nested handler converts into a skipped collection. Disk-backed feature streams can therefore never be collected.
Proposed fix
- load_feature_streams_from_disk(feature, project_dir)
+ load_feature_streams_from_disk(feature)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not feature.streams.value and project_dir: | |
| try: | |
| load_feature_streams_from_disk(feature, project_dir) | |
| except Exception: | |
| logging.warning("Failed to load streams from disk; skipping collection", exc_info=True) | |
| if not feature.streams.value and project_dir: | |
| try: | |
| load_feature_streams_from_disk(feature) | |
| except Exception: | |
| logging.warning("Failed to load streams from disk; skipping collection", exc_info=True) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/acq/feature.py` around lines 471 - 475, Update the
load_feature_streams_from_disk call in the feature stream-loading block to pass
only the supported feature argument, while preserving the existing exception
handling and collection flow.
| probability = DataCollector().probability | ||
| self.tab_data_model.main.features_collectable = ( | ||
| random.random() < probability | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Expose the project sampling rate before the chamber reads it.
A new DataCollector exposes 0.10 at construction. Its only recalculation occurs later in record(), on a different instance. Project sampling therefore uses 10%, not the required 20%, and cannot reflect consent-dependent state.
src/odemis/gui/cont/tabs/cryo_chamber_tab.py#L396-L399: use a dedicated 20% feature-sampling constant or an initialized query method.src/odemis/util/datacollector.py#L659-L661: do not expose a fixed default as a ready-to-use computed rate.src/odemis/util/datacollector.py#L792-L795: move rate derivation into the query method if consent-dependent sampling is required.
The PR objective specifies a 20% per-project sampling decision.
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 397-397: use secrets package over random package
Context: random.random()
Note: [CWE-330] Use of Insufficiently Random Values.
(avoid-random-python)
🪛 Ruff (0.16.0)
[error] 398-398: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
📍 Affects 2 files
src/odemis/gui/cont/tabs/cryo_chamber_tab.py#L396-L399(this comment)src/odemis/util/datacollector.py#L659-L661src/odemis/util/datacollector.py#L792-L795
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/gui/cont/tabs/cryo_chamber_tab.py` around lines 396 - 399, Use a
dedicated 20% sampling constant or initialized query method in
cryo_chamber_tab.py lines 396-399 for the per-project decision. In
datacollector.py lines 659-661, stop exposing the fixed default as a
ready-to-use computed rate; in datacollector.py lines 792-795, derive the rate
within the query method when consent-dependent sampling is required.
| decoded_features = [feature_decoder(f) for f in proj_data["features"]] | ||
| for f in decoded_features: | ||
| f.collect = False | ||
| self.tab_data_model.main.features.value = [df for df in decoded_features if df is not None] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Filter decoder failures before mutating decoded features.
feature_decoder() returns None when an entry lacks name or status. Line 530 accesses f.collect before Line 531 removes None. One invalid feature entry raises AttributeError and aborts project loading.
Proposed fix
- decoded_features = [feature_decoder(f) for f in proj_data["features"]]
- for f in decoded_features:
- f.collect = False
- self.tab_data_model.main.features.value = [df for df in decoded_features if df is not None]
+ decoded_features = []
+ for feature_raw in proj_data["features"]:
+ feature = feature_decoder(feature_raw)
+ if feature is None:
+ continue
+ feature.collect = False
+ decoded_features.append(feature)
+ self.tab_data_model.main.features.value = decoded_features📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| decoded_features = [feature_decoder(f) for f in proj_data["features"]] | |
| for f in decoded_features: | |
| f.collect = False | |
| self.tab_data_model.main.features.value = [df for df in decoded_features if df is not None] | |
| decoded_features = [] | |
| for feature_raw in proj_data["features"]: | |
| feature = feature_decoder(feature_raw) | |
| if feature is None: | |
| continue | |
| feature.collect = False | |
| decoded_features.append(feature) | |
| self.tab_data_model.main.features.value = decoded_features |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/gui/cont/tabs/cryo_chamber_tab.py` around lines 528 - 531, Update
the decoded-features handling around feature_decoder so None results are
filtered out before the loop mutates each feature. Ensure f.collect is assigned
only for valid decoded features, then assign the filtered collection to
main.features.value while preserving the existing behavior for invalid entries.
Provides a thread-safe, non-blocking ``DataCollector.record()`` call that any Odemis module can invoke to capture a labelled data sample. CLI interface is implemented to download the data from the cloud storage.
e1749e0 to
b5a8a4d
Compare
| try: | ||
| _dc = DataCollector() | ||
| if not _dc.get_consent(): | ||
| return |
There was a problem hiding this comment.
@K4rishma currently, gui/main.py creates a _data_collector, and keeps it for the rest of the GUI lifetime. I'd suggest to move it to the MainGUIData(). Then, pretty much every controller will be able to access it. You can have a single DataCollector instantiated. You can pass from the caller to this function.
| else: | ||
| fm_images.append(s) | ||
|
|
||
| def _get_raw(stream: Stream) -> Optional[model.DataArray]: |
There was a problem hiding this comment.
I don't think there is a case where there is a stream with an image, but without .raw. Do you have something in mind? If so, it would help to add it as a comment. Otherwise, I think you can simplify and return .raw, and None if it's empty.
| # Decide once per project whether features created during this session are | ||
| # eligible for data collection. Stored as a dynamic attribute — not part | ||
| # of the formal model — and read by add_new_feature via getattr. | ||
| probability = DataCollector().probability |
There was a problem hiding this comment.
Use the global data collector.
| md = self.main.focus.getMetadata() | ||
| fm_focus_position = md[model.MD_FAV_POS_ACTIVE] | ||
| feature = CryoFeature(f_name, stage_position, fm_focus_position) | ||
| features_collectable = getattr(self.main, "features_collectable", False) |
There was a problem hiding this comment.
It should be an attribute on CryoMainGUIData, and not require any protection.
Maybe adjust the name to make it more clear like "should_collect_features".
| self.tab_data_model.main.features_collectable = ( | ||
| random.random() < probability | ||
| ) |
| continue | ||
| other_pos = other.stage_position.value | ||
| ox, oy = other_pos.get("x", 0.0), other_pos.get("y", 0.0) | ||
| dist = math.sqrt((fx - ox) ** 2 + (fy - oy) ** 2) |
| # Collect percentage of the acquired data based on the selected probability | ||
| # in order to keep the growth of the collected data in control. |
There was a problem hiding this comment.
This comment is very hard to understand. Can you re-explain in your own words. I think the most important part is that it's a float between 0 -> 1 (right?). And a small value means less chance to collect.
| if days_left is not None and days_left <= 1: | ||
| probability = _FULL_COLLECTION_PROBABILITY | ||
| self.probability = _FULL_COLLECTION_PROBABILITY | ||
| else: | ||
| probability = _DEFAULT_COLLECTION_PROBABILITY | ||
|
|
||
| if random.random() >= probability: | ||
| logging.debug( | ||
| "DataCollector: event '%s' not sampled (%.0f%% collection probability).", | ||
| event_name, probability * 100, | ||
| ) | ||
| return | ||
| self.probability = _DEFAULT_COLLECTION_PROBABILITY | ||
|
|
There was a problem hiding this comment.
@K4rishma I agree here that it doesn't make sense to re-update .probability every time .record() is called. It should be done at init only then, or when the consent date changes.
| :returns: True when another feature is within the threshold distance. | ||
| """ | ||
| pos = feature.stage_position.value | ||
| fx, fy = pos.get("x", 0.0), pos.get("y", 0.0) |
There was a problem hiding this comment.
Do we need such protection? All features should have x&y in the positions. For "safety" just catch all exceptions and claim that the feature is not near any other feature.
|
|
||
| return {"z": zpos} | ||
|
|
||
| def _on_status_for_collection(self, _status: str) -> None: |
There was a problem hiding this comment.
I'm not fond of this name... but can't find something really better. Maybe just "_on_feature_status"? Any other idea?
| self.txt_projectpath.Value = os.path.basename(self.conf.pj_last_path) | ||
| self.tab_data_model.main.project_path.value = new_dir | ||
| # Decide once per project whether features created during this session are | ||
| # eligible for data collection. Stored as a dynamic attribute — not part |
There was a problem hiding this comment.
| # eligible for data collection. Stored as a dynamic attribute — not part | |
| # eligible for data collection. Stored as a dynamic attribute — not part |
| self.correlation_data = correlation_data | ||
|
|
||
| # Whether this feature is eligible for data collection | ||
| self.collect: bool = collect |
There was a problem hiding this comment.
I think this is not a great name. Collect is a verb and implies a method that does collection in my view. I'd rather go with something self-explanatory like eligible_for_collection, or a bit less self-explanatory, yet shorter is_collectible.
| # eligible for data collection. Stored as a dynamic attribute — not part | ||
| # of the formal model — and read by add_new_feature via getattr. | ||
| probability = DataCollector().probability | ||
| self.tab_data_model.main.features_collectable = ( |
There was a problem hiding this comment.
Is this now also executed when there is no consent? Especially the log can be confusing then: no consent, but stuff eligible for collection.
| continue | ||
| other_pos = other.stage_position.value | ||
| ox, oy = other_pos.get("x", 0.0), other_pos.get("y", 0.0) | ||
| dist = math.sqrt((fx - ox) ** 2 + (fy - oy) ** 2) |
There was a problem hiding this comment.
Is there a reason we ignore z? Also, would using the sample stage coordinate space make more sense?
Builds on PR of Building the data framework #3444
Overview
Introduces per-project sampling for cryo feature data collection. When a project is opened or created, a single random decision is propagates from the data collection framework and applies uniformly to all features created during that session. Features loaded from a previous session are immediately excluded from collection.
How it works
Single decision per project (
cryo_chamber_tab.py)_change_project_conf()is called every time a project is opened or created. It makes one random draw (random.random() \< PROBABILITY) and stores the result asmain.features_collectable— a lightweight dynamic attribute on the main GUI data model (not a formal VA, never persisted to disk).All new features inherit the decision (
tab_gui_data.py)CryoGUIData.add_new_feature()readsgetattr(self.main, "features_collectable", False)and passes it ascollect=when constructing eachCryoFeature. Every feature created in the same session therefore shares the same flag value — either all are eligible for collection or none are.Loaded features are excluded (
cryo_chamber_tab.py)In
_load_project_data(), after features are read fromfeatures.json, every loaded feature is immediately reset tocollect=Falsebefore being assigned to the model. Features from a prior session were either already collected or never selected; the new session's sampling decision applies only to features created going forward.CryoFeature.collectflag (feature.py)The
collectparameter onCryoFeature.__init__is a plainbool, defaulting toFalse. The flag is serialised intofeatures.jsonand survives a save/load cycle. If the key is absent in loaded JSON (older data), it defaults toFalse.