Conversation
C-Achard
added
bug fix
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Pre-remap synchronization and permanently unbound config-first layers can still leak metadata across datasets.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds dataset identity checks to prevent surviving annotation layers from adopting unrelated image paths.
Changes:
- Adds persistent dataset-folder keys and mismatch detection.
- Warns users when layers cannot safely follow a new folder.
- Adds unit and end-to-end folder-switch coverage.
File summaries
| File | Description |
|---|---|
core/project_paths.py |
Adds dataset identity helpers. |
core/layer_lifecycle/manager.py |
Gates remapping and emits mismatch warnings. |
core/io.py |
Records dataset keys during reads. |
config/models.py |
Adds Points dataset metadata. |
_widgets.py |
Displays mismatch dialogs. |
_tests/e2e/utils.py |
Adds dataset test fixtures. |
_tests/e2e/test_folder_switch_integrity.py |
Tests folder-switch save integrity. |
_tests/core/test_remap.py |
Tests remap rejection behavior. |
_tests/core/test_project_paths.py |
Tests dataset identity helpers. |
_tests/core/layer_manager/test_manager.py |
Tests lifecycle mismatch handling. |
Review details
Suppressed comments (1)
src/napari_deeplabcut/core/layer_lifecycle/manager.py:948
- An unbound layer adopts this context without recording
_image_dataset_key. Config-first layers therefore remain permanently unbound; after labeling folder A, removing its image, and opening folder B with the same frame names,_belongs_to_current_dataset(None)allows the layer to remap and migrate to B—the leak this PR is intended to prevent. Bind a missing key on the first successful adoption.
try:
safe_image_meta = self._image_meta.model_dump(exclude_none=True)
safe_image_meta.pop("paths", None)
layer.metadata.update(safe_image_meta)
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
deruyter92
approved these changes
Sep 16, 2026
Extract shared test utilities to write DLC `config.yaml` files and labeled frames, then reuse them across existing fixture builders to remove duplicated setup code. Also add a new helper that creates a project with two `labeled-data` folders (including optional basename-collision frame names) for multi-folder e2e scenarios.
Add end-to-end coverage for reopening a different labeled folder while a Points layer survives. The new tests guard against partially rebinding layer metadata on unmappable switches, verify saves do not write foreign-indexed annotations into the new dataset, and include a positive control where matching frames correctly move root and paths together.
Only adopt the current image context for a layer when it has no existing paths or when a path remap is explicitly accepted. If remapping is rejected because the new folder does not match cleanly, show a deduplicated warning so users know the layer still saves back to its original dataset instead of silently following the newly opened folder.
Add coverage for remap cases that must leave layer metadata unchanged when frame matching fails or is ambiguous, and for successful remaps that must update `root` and `paths` together. The new tests also lock in suppression of duplicate dataset-mismatch warnings and cover remap results that reject path updates on ambiguous depth-1 or no-overlap matches.
Connect `LayerLifecycleManager.layer_dataset_mismatch` to `KeypointControls` so dataset-folder mismatch warnings are shown from the widget layer. Tests were updated to assert the signal-based behavior (including deduping once per target folder) instead of monkeypatching the old direct warning path. Route dataset mismatch warnings via signal Replace direct `show_warning` calls when a layer stays on its previous dataset with `_report_layer_left_on_previous_dataset`, which records mismatches per layer/new-root and reports only the first occurrence per folder switch. The message is now sent through `viewer.status` and a new `layer_dataset_mismatch` signal, while logging still captures every remap pass.
Prevented layer remapping from matching unrelated files when switching labeled-data folders. The lifecycle manager now checks whether a layer root and current image root refer to the same dataset folder, using full ordered-depth matching only in that case. For cross-dataset cases, remapping uses a new `DATASET_SCOPED` policy (depths 3 and 2 only) to avoid bare-filename collisions from DLC’s fixed frame names.
Add a dataset-scoped path matching policy that refuses ambiguous basename-only overlaps when frame names repeat across folders. This prevents layers from silently migrating annotations onto another dataset when the only commonality is matching filenames after a project or folder move. Updated remap and folder-switch tests to cover the safer behavior and guard against false-positive rebinds.
Add a regression test covering two DeepLabCut projects that share the same video-derived dataset path (`labeled-data/mouse1/...`). The new helper builds that fixture setup, and the test asserts an existing points layer keeps its original project/root metadata and does not save annotations into the second project when folders are switched.
Add an immutable `dataset_key` to image and points metadata based on the source folder. The lifecycle manager now uses that key to decide whether a layer should adopt the current image context, leaving layers from other datasets untouched and warning instead of remapping them. Since dataset identity is checked up front, path remapping can always use the ordered depth fallback safely, and the old dataset-scoped policy is removed.
Updates remap-related tests to reflect the new identity model: dataset ownership is determined by `dataset_key` (absolute dataset folder), not by basename/path-match policy alone. The layer manager tests now cover cross-dataset rejection, cross-project same-folder-name rejection, same-dataset remap adoption, and adoption of unbound placeholder layers. Related path/remap tests were simplified by removing `DATASET_SCOPED` policy assertions and adding checks that dataset keys round-trip and remain distinct across projects.
Use the image dataset key when tracking repeated dataset-mismatch warnings so duplicate notifications are suppressed consistently across folder changes. The commit also tightens the status message shown to users and clarifies the filename-matching comment around remapping frame paths.
Add `is_same_dataset` in `project_paths` to compare dataset folders robustly using normalized case and `os.path.samefile`, with fail-closed behavior when on-disk comparison is unavailable. Update layer lifecycle matching to use this helper instead of raw string equality, so equivalent paths (e.g., symlink/UNC aliases and Windows case variants) are treated as the same dataset. Expand tests to cover identical/distinct paths, missing folders, alternate routes to one folder, and Windows case-insensitive matching.
Treat config placeholders as dataset-bound when they adopt image context, so layers that start without a `dataset_key` keep the dataset they first take over instead of being remapped to a later folder with matching frame names.
The override rewrote `root` when the points root equalled the project root or was not a dataset root, so `sync_points_from_image` had two jobs: seeding missing image-derived fields, and second-guessing a root that was already set. Only the first was reachable.
Centralize dataset-follow checks for layer metadata seeding and bind layers to the dataset they actually adopt. This prevents points and image layers from inheriting paths or context from the wrong folder, especially when a layer is already tied to another dataset or starts unbound.
Clarify and tighten how points layers inherit image context from the currently open folder. Layers now consistently check whether they may follow the current dataset before inheriting `root` or `paths`, and the helper that records the adopted dataset key is renamed to better reflect that it only persists the folder identity after paths are inherited. This helps prevent layers from being accidentally associated with the wrong dataset when folders share frame names.
Cover points-layer syncing and wiring when metadata inherits paths and dataset keys from the open folder. The new tests ensure layers keep their original dataset root, adopt matching paths, and reject paths from a different dataset without spuriously warning when no image is open.
C-Achard
force-pushed
the
cy/fix-points-data-drift
branch
from
September 17, 2026 14:25
a53493b to
3796898
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
DLC videos lack dataset keys, mismatch notifications are not fully deduplicated, and same-named dataset warnings are ambiguous.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 3
- Review effort level: Balanced
When a video/image is opened, store its dataset key in metadata so layer lifecycle logic can compare the correct dataset identity against the active folder. This fixes dataset-mismatch detection for layers that have been reopened or moved across folders. The warning logic now tracks mismatches per layer more accurately and shows both the original save location and the active folder, making it clear where data will still be written.
This change fixes dataset identity checks when a video/image context lacks an explicit dataset key. The lifecycle manager now resolves the key from the containing labeled-data folder, avoids re-reporting the same mismatch, and includes both dataset folders in the mismatch message. read_video now stores the resolved dataset key in metadata so videos and annotations are treated as the same dataset when they belong to the same labeled-data folder. Added regression tests covering mismatch deduplication, folder names in warnings, and the video metadata key.
Update the layer manager test to use a real temporary folder path for the keyless image context. The test now reflects the actual resolved folder behavior and no longer stubs header validation.
When frame indices are remapped within the same opened dataset, the layer lifecycle manager no longer tells the user to clear the layer. This fixes the misleading guidance that appeared when re-extracting frames in place, where the annotations should stay with the existing folder and continue saving there. The fix distinguishes same-dataset remaps from true cross-folder mismatches and emits a clearer message in the same-folder case. A regression test covers the scenario where the folder name remains unchanged but the frame set is replaced.
C-Achard
requested review from
deruyter92
and
a balanced review from Copilot
September 18, 2026 09:34
Expand the `is_same_dataset` docstring to capture the Windows path aliasing bug from DeepLabCut#3348. This explains why the `samefile` fallback is required when the same project directory can appear under mapped-drive, UNC, or volume GUID paths, and warns against reducing the check to string equality.
deruyter92
reviewed
Sep 21, 2026
deruyter92
left a comment
deruyter92
left a comment
Collaborator
There was a problem hiding this comment.
Good fix, looks in good state.
one comment with a minor suggestion. Please judge for yourself if needed
deruyter92
approved these changes
Sep 21, 2026
Use the active dataset binding instead of a layer's stale root when deciding whether a points layer still belongs to the currently opened folder. This updates the mismatch message to reference the open folder and avoids telling unbound layers to clear annotations when they can still save to the current dataset. Tests cover unbound layers and the revised warning text.
Ensure point layers record their dataset key whenever they inherit image context from the currently open folder, whether that happens during layer setup or image metadata sync. This prevents adopted layers from gaining root/paths without being bound to the active dataset, and adds regression coverage for both entry points.
Add detailed comment explaining the historical context of the 'keypoints' fallback key. The 'keypoints' key was used in this package from May 2022 until April 2026, when the canonical 'df_with_missing' key was adopted. Projects labelled during that period may legitimately contain either key, so this fallback is necessary for backward compatibility with older labelling data.
Replace `dataset_key` with `dataset_folder` across layer metadata, lifecycle management, IO, and tests to make dataset identity terminology match the actual absolute folder being tracked. This also renames the folder resolver helper to `resolve_dataset_folder` and updates the related assertions and comments.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Two adoption paths can still pair existing paths with a new root before remapping validates the dataset.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Comment on lines
+694
to
+696
| inherits_context = (not ly.metadata.get("paths") and bool(self._image_meta.paths)) or ( | ||
| not ly.metadata.get("root") and bool(self._image_meta.root) | ||
| ) |
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.
Adds a warning avoiding edge cases related to leftover Points layer carrying saving information over to unrelated, newly loaded Image layers:
videoAlayer,videoBopened), no matter the frame namesproject-A/labeled-data/mouse1vsproject-B/…)Some cases were already covered but not clearly reported:
All these now clearly warn the user and give feedback.
Closes #245.
Main changes
dataset_keytoPointsMetadata, the resolved absolute folder a layer was read from, written once at read time on both image and annotation layers_remap_frame_indicescompares keys before attempting any frame matching, layers with differing keys are left untouchedroot,shapeandnameare adopted only alongside apathsupdate, or when the layer had nopathsat all, so the two can no longer name different datasetslayer_dataset_mismatchsignal, deduplicated per layer and target folder, and emitted out of the insert callback, opening a dialogrootandpaths, so a subsequent save writes back to its original dataset instead of the newly opened one.