Fix SkeletonBuilder initialization from GUI - #3380
Merged
Merged
Conversation
Change SkeletonBuilder to use BaseSkeletonBuilder as the first base class and make its __init__ accept config_path as a keyword-only argument. Initialize the Qt dialog with an explicit parent (parent=parent) and keep BaseSkeletonBuilder.__init__ called with config_path. Update the call site in create_videos to pass config_path=... and parent=self.root. These changes clarify MRO, enforce explicit config_path usage, and ensure the dialog gets the correct parent widget.
Update call in label_frames.py to use explicit keyword arguments: config_path=self.root.config and parent=self.root. This ensures the SkeletonBuilder receives the parent GUI context and makes the parameter usage clearer.
Update LabelFrames.build_skeleton to instantiate SkeletonBuilder without passing parent=self.root. This aligns the call with the updated SkeletonBuilder constructor/signature and avoids supplying the GUI root object.
Assign SkeletonBuilder instances to self.skeleton_builder and call show() in both create_videos and label_frames tabs. Also update the import in label_frames to use deeplabcut.gui.widgets.SkeletonBuilder. This ensures the builder window is displayed.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a crash when launching the Skeleton Builder from the GUI (notably the Create Videos tab) by adjusting SkeletonBuilder’s Qt-related initialization and updating GUI call sites to keep the dialog alive and shown.
Changes:
- Reordered
SkeletonBuildermultiple inheritance to avoidQDialog.__init__()triggering an MRO path that calls the base builder__init__withoutconfig_path. - Updated GUI call sites to pass
config_path/parentexplicitly, retain a reference onself, and call.show(). - Switched the Label Frames tab to use the GUI
SkeletonBuilderimplementation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
deeplabcut/gui/widgets.py |
Adjusts SkeletonBuilder inheritance and constructor behavior to prevent Qt initialization crash. |
deeplabcut/gui/tabs/label_frames.py |
Uses GUI SkeletonBuilder and ensures the dialog is kept alive and shown. |
deeplabcut/gui/tabs/create_videos.py |
Ensures Skeleton Builder is constructed with explicit kwargs, retained, and shown. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Change SkeletonBuilder.__init__ signature in deeplabcut/gui/widgets.py from `def __init__(self, *, config_path, parent=None)` to `def __init__(self, config_path, *, parent=None)` so callers can pass config_path positionally while keeping parent keyword-only. No other behavior changes.
C-Achard
marked this pull request as ready for review
June 24, 2026 13:15
Collaborator
Author
|
This patch uses the GUI/Qt version of the widget, which I think looks cleaner overall. Note we could also use the base matplotlib version (see #3379) if preferred. |
deruyter92
approved these changes
Jun 24, 2026
deruyter92
left a comment
Collaborator
There was a problem hiding this comment.
Good fix! Also good call to use the Qt dialog instead of matplotlib.
LGTM
Add skeleton_builder = None to CreateVideos and LabelFrames to ensure the attribute exists and avoid potential attribute errors. In SkeletonBuilder, instantiate FigureCanvas earlier (remove duplicate instantiation) and add an export() override that calls the base export and logs a success message. Also includes a minor whitespace/layout cleanup.
Ensure only one SkeletonBuilder dialog is created per tab and properly cleaned up. CreateVideos and LabelFrames now check if self.skeleton_builder is None before instantiating, connect the widget's destroyed signal to _on_skeleton_builder_destroyed to clear the reference, and avoid reopening if already present. In widgets.SkeletonBuilder set WA_DeleteOnClose so the dialog is deleted on close and show a brief statusbar message on export. Also import Qt into widgets where needed. These changes prevent multiple dialog instances, fix cleanup, and provide user feedback on export.
Use self.cfg.get("skeleton", []) and a local skeleton variable before iterating, replacing direct indexing of self.cfg["skeleton"]. This avoids a KeyError when the config lacks a skeleton entry while preserving existing behavior when skeleton is present.
Introduce logging and make SkeletonBuilder UI parameters configurable. Added import logging and module logger; initialize self._ax and self.df in constructor. Replace local ax and ampl with instance attributes (self._ax, self.ampl) and expose UI parameters (lasso_select_size, clear/export button axes and labels) to avoid hard-coded values. Update build_ui to use the new attributes and wire up LassoSelector and Buttons using configurable axes/text. These changes improve configurability and prepare for further config refactoring.
Add UI feedback on export and harden export logic: wrap export in try/except, log when saving an empty or partially connected skeleton, write config and show a temporary green "Saved N" button label (with timer reference to avoid GC). Make lasso selection resolution/zoom independent by transforming keypoints and lasso verts into display coordinates, querying a KDTree built in display space using self.lasso_select_size, and ensure segments are passed as a list to set_segments before redrawing.
Refactor SkeletonBuilder plotting code to use instance attributes instead of hard-coded values: rename ax to _ax for encapsulation, use self.clear_button_axes and self.export_button_axes for button placements, use self.clear_button_text and self.export_button_text for labels, and replace the inline ampl constant with self.ampl. Also attach the LassoSelector and all drawing operations to _ax and keep canvas draw_idle(). These changes allow external configuration of button layout, labels, and zoom/amplification behavior.
Collaborator
Author
|
@deruyter92 Made some further improvements, let me know what you think! Waiting for more info from #3379 otherwise as I'm not sure I can reproduce exactly what they describe. |
Move UI/usage parameters (lasso_select_size, clear/export button axes/text, ampl) from __init__ to class-level attributes in deeplabcut/utils/skeleton.py so they become shared defaults rather than instance-only values. Update tests in tests/utils/test_skeleton.py: attach_fake_canvas now creates an _ax on the Figure and sets its x/y limits to ensure plotting code has an axes to operate on; test_export_sorts_pairs_and_warns_for_unconnected now uses the caplog fixture (caplog.at_level and an assertion on caplog.text) instead of pytest.warns to verify the informational log about unconnected bodyparts. These changes improve default handling and make tests more robust about logging and axis availability.
deruyter92
approved these changes
Jun 26, 2026
deruyter92
left a comment
Collaborator
There was a problem hiding this comment.
Great additions. Few very minor questions/comments about the lasso_select_size and the logging.
Make skeleton export report real success/failure to the Qt UI by returning a boolean from `SkeletonBuilder.export()` and conditionally showing success or failure messages in `gui/widgets.py`. The export path now logs incomplete/empty skeleton states as warnings and returns `False` on exceptions. This changes `lasso_select_size` from 5 to 10 to improve selection usability in the skeleton builder.
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.
Crash description
This PR fixes a crash that occurs when using Build Skeleton in the Create Videos tab.
Reason
The crash might be caused by an interaction between SkeletonBuilder’s multiple inheritance order and Qt dialog initialization. SkeletonBuilder inherited from QtWidgets.QDialog before BaseSkeletonBuilder, and QDialog.init() could trigger an initialization path that expected config_path but did not receive it, resulting in a TypeError. This could be because QtWidgets.QDialog is a Qt/C++-backed class, so its initialization might behave differently across Qt bindings or versions.
Attempted fix
Closes #3379