Skip to content

Fix KeypointStore not initializing when layers load before widget#169

Draft
milesAraya wants to merge 2 commits intoDeepLabCut:mainfrom
milesAraya:fix/retroactive-keypoint-store-init
Draft

Fix KeypointStore not initializing when layers load before widget#169
milesAraya wants to merge 2 commits intoDeepLabCut:mainfrom
milesAraya:fix/retroactive-keypoint-store-init

Conversation

@milesAraya
Copy link
Copy Markdown

Pull Request: Fix KeypointStore not initializing when layers load before widget

Current Branch: fix/retroactive-keypoint-store-init
Target Branch: main


Content

Summary

  • When napari-deeplabcut is activated via the DeepLabCut GUI, on_insert may not fire for Points layers that were added before the KeypointControls widget was fully initialized
  • This leaves _stores empty, which disables labeling mode controls (Sequential/Quick/Loop), keypoint cycling (Up/Down/M keys), and dropdown menus
  • Added _retroactive_store_init() to KeypointControls.__init__ which retries every second (up to 10 attempts) to detect and initialize any unprocessed Points layers
  • Also guards on_insert against Points layers without DLC metadata to prevent KeyError crashes on missing header

Design Decisions

  • QTimer retry loop over a single delayed call: The plugin activation via action.trigger() in DeepLabCut's launch_napari() is asynchronous, and the delay before the widget is ready varies (observed ~3-7 seconds). A retry loop with a cap is more robust than guessing a fixed delay
  • 10 attempts at 1-second intervals: Conservative upper bound; in testing the fix typically succeeds within 4-7 attempts. After 10 seconds without a Points layer, it's safe to assume none will appear from the initial load
  • Guard in on_insert rather than try/except: Explicitly checking for header metadata is clearer than catching KeyError, and avoids masking unrelated errors in KeypointStore.__init__
  • Duplicating on_insert logic in _retroactive_store_init: Considered calling on_insert directly with a synthetic event, but on_insert uses event.source[-1] which assumes specific event structure. Duplicating the store initialization logic is safer and more readable

Evidence

  • Tested on macOS (Apple M3) with DeepLabCut 3.0.0rc6, napari 0.6.6, PySide6 6.7.3
  • Without fix: _stores is empty, all mode radio buttons disabled (isEnabled() == False)
  • With fix: _stores populated after ~4-7 seconds, mode buttons enabled, Loop/Quick/Sequential modes functional, Up/Down keypoint cycling works

References

Files changed

  • src/napari_deeplabcut/_widgets.py -- Add _retroactive_store_init() method with QTimer retry loop to KeypointControls.__init__; add early return guard in on_insert for Points layers missing header metadata

Manual Testcases

  • Launch DeepLabCut GUI: python -m deeplabcut
  • Load a project and click "Label Frames"
  • Select a labeled-data subfolder (e.g. labeled-data/video_name/)
  • Verify CollectedData_<scorer> layer appears in napari
  • Verify labeling mode radio buttons (Sequential/Quick/Loop) become enabled within ~5 seconds
  • Select "Loop" mode and place keypoints — verify it auto-cycles through bodyparts
  • Press M key — verify it cycles between Sequential/Quick/Loop
  • Press Up/Down — verify it switches between bodyparts
  • Add a manual Points layer via napari menu — verify no crash occurs

Unit, Integration, Contract Test Coverage

  • No existing tests cover this initialization path; the fix is in GUI widget startup timing

Others

Difficulties

  • The root cause is a race condition between action.trigger() (async plugin activation) and viewer.open() (sync layer loading) in DeepLabCut's launch_napari(). The widget's on_insert listener connects during __init__, but __init__ hasn't completed when layers are already being added
  • on_insert uses event.source[-1] rather than event.value, so creating synthetic events for retroactive processing is fragile

Risk Assessment

Area Risk Notes
Retroactive store init Low Only runs when _stores is empty; no-ops if on_insert already handled layers normally
on_insert guard Low Early return for layers without header metadata; no impact on normal DLC workflow
QTimer retry Low Capped at 10 attempts; each attempt is lightweight (iterates layers list)

@milesAraya milesAraya marked this pull request as ready for review March 6, 2026 08:29
@C-Achard
Copy link
Copy Markdown
Collaborator

C-Achard commented Mar 6, 2026

Hello @milesAraya,

Thanks for the PR ! This quirk is already being addressed in #163, #164 and related refactor work, which also tackle quite a few other issues with the plugin.

While this minimal fix is very nice and concise, I would kindly suggest a few potential points for improvement :

  • Timer-based retries may not be the most specific way of addressing the problem:
    • It is less targeted than wiring into the layer insertion events
    • Event-driven init is typically more deterministic than time-based init and avoids retry logic when the widget opens
  • QTimers introduce timing sensitivity, which can cause issues in tests and CI, as they can introduce non-determinism and therefore require dedicated handling, otherwise they may cause teardown errors.
  • Duplicating logic for keybinds handling is best avoided, ideally a centralized helper is to be used as in the refactor.
  • When introducing timer-based behavior, it’s usually best to add/adjust tests to make the timing deterministic, and document any test/CI considerations, especially regarding the timer handling. I understand this may not seem to be very well-enforced in the codebase so far, but the refactor aims to address this.

For now I’ll mark this PR as draft to avoid merging overlapping fixes while the refactor is in progress.

If you'd like to get involved in the refactor, you're very welcome to review relevant sections, and share feedback on the implementation suggested here and here, as well as the signal wiring throughout.

Any contributions there are welcome, feel free to open a PR targeting #168 for example.
I'm happy to hear your thoughts!

Best,
Cyril

@C-Achard C-Achard marked this pull request as draft March 6, 2026 09:20
@C-Achard C-Achard self-assigned this Mar 6, 2026
@C-Achard C-Achard added enhancement New feature or request bug fix Fixes an issue or a bug labels Mar 6, 2026
@C-Achard C-Achard added this to the Core plugin overhaul (2026) milestone Mar 6, 2026
@milesAraya milesAraya force-pushed the fix/retroactive-keypoint-store-init branch from 3b3940d to 4cdb6b9 Compare March 6, 2026 09:35
@milesAraya
Copy link
Copy Markdown
Author

Thanks Cyril! Good to know this is covered in the refactor. The timer approach was pragmatic, we needed a working fix for our lab's labeling workflow. Happy to see _adopt_existing_layers and the root fallback in _wire_points_layer handle this properly.

@C-Achard
Copy link
Copy Markdown
Collaborator

C-Achard commented Mar 6, 2026

@milesAraya Understood, thanks for sharing the fix !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug fix Fixes an issue or a bug enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants