Skip to content

minor follow-up adjustments for config GUI validation - #3399

Merged
deruyter92 merged 14 commits into
cy/config-gui-error-handlerfrom
jaap/config-gui-additions
Jul 20, 2026
Merged

minor follow-up adjustments for config GUI validation#3399
deruyter92 merged 14 commits into
cy/config-gui-error-handlerfrom
jaap/config-gui-additions

Conversation

@deruyter92

Copy link
Copy Markdown
Collaborator

Summary

Some add-on follow-up commits to merge/cherry-pick into #3397

  • Simplify error messages (83bf20d8) and deduplicate error handling (35eb0eb6).
  • Use a cached, typed ProjectConfig throughout the GUI (a37f5ef6, e4832ed1).
  • Reload after in-app edits and notify users about external changes (d649f6df, 2f82e030).
  • Handle invalid project and pose configs without crashing or hanging workers (83200b18, 12d07b2a).
  • Keep the config editor usable for repairing invalid files (1cb4d940).
  • Use the canonical typed config reader in compat.py (4f9cf4af).
  • Add basic pytest-qt coverage (991d9f77).

Tests

Some basic tests for the new behavior are added here:

python -m pytest tests/gui

@deruyter92 deruyter92 changed the title Jaap/config gui additions minor follow-up adjustments for config GUI validation Jul 15, 2026

@C-Achard C-Achard left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing follow-up, thank you! Great that you added GUI tests as well, will be very useful.

Comment thread deeplabcut/gui/config_file_monitor.py
Comment thread deeplabcut/gui/tabs/analyze_videos.py Outdated
config = self.root.config
editor = ConfigEditor(config, parent=self.root)
editor.accepted.connect(
lambda: QTimer.singleShot(

@C-Achard C-Achard Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kind of pattern is not very safe, I would recommend keeping a more explicit reference. This won't be an issue for now but once we add the tests to CI these timers can be very flaky in short lived CI instances and with mocks/fixtures

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok good point, I will think of a different implementation.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The _reload_timer is now stored as self._reload_timer in both AnalyzeVideos and ManageProject tabs, so the timer is an explicit named attribute. Does this work as you'd advice?

Comment thread deeplabcut/gui/tabs/manage_project.py Outdated
config = self.root.config
editor = ConfigEditor(config, parent=self.root)
editor.accepted.connect(
lambda: QTimer.singleShot(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above answer

Comment thread deeplabcut/gui/window.py Outdated
@C-Achard C-Achard added enhancement New feature or request GUI issues relating to GUI config Related to config.yaml, ruamel, YAML parsing, ... labels Jul 16, 2026
@C-Achard
C-Achard self-requested a review July 17, 2026 08:36
- Initialize self._config_monitor = None early in __init__
- Replace getattr(self, "_config_monitor", None) with direct attribute access in config_path setter
- Remove unintentional double @Property decorator on project_folder
- Catch BaseException in Worker.run() so SystemExit from CLI functions
  is marshalled to the UI instead of silently killing the thread
- Wrap the full evaluate_network method in a single try/except Exception
  so runtime errors (GPU OOM, corrupted models, etc.) are consistently
  reported through show_task_error instead of only catching config errors
- Add named _reload_timer members to AnalyzeVideos and ManageProject tabs
  so the timer is inspectable in tests and auto-cancels on tab destruction
- Wire editor.accepted to timer.start instead of a fire-and-forget lambda
a lightweight hook to force the next cfg access to re-read and validate from disk. Can be called from outside the GUI as well (e.g. after video analysis or training network, in case they manipulate te config)
…d timer

- Add test_task_error.py: verify generic vs config-error dialog rendering
  and that the 'Open configuration' button only appears for config errors
- Test invalidate_config_cache drops the cached config for the next access
  and is safe to call with no project loaded
- Test that the named _reload_timer (replacing QTimer.singleShot) reliably
  fires the reload callback

@C-Achard C-Achard left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are a few additional comments; thanks again!

Comment on lines +19 to +26
CONFIG_LOAD_ERRORS = (
ValidationError,
FileNotFoundError,
PermissionError,
OSError,
TypeError,
ValueError,
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a bit broad? How can we ensure it is indeed config related?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this handles pretty much anything that the GUI starts. Now, I do like having better exception wrappers, especially if they prevent making the GUI stuck, but still this may not be quite as granular as desired. Or am I missing a design piece?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. Basically this is just extracting what was already there and putting it in a separate constant. Not solving the underlying problem, sorry.

Instead what we should do is except on this broad list when the config is loaded, but not for the worker errors. In the latter case we can only be sure that it is a config error when it is a Pydantic ValidationError.

I've updated this now.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the show_task_error method (which receives errors from arbitrary worker threads) now only treats ValidationError as unambiguously config-related

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CONFIG_LOAD_ERRORS tuple is kept only for the config-loading recovery loop,

Comment thread deeplabcut/gui/displays/selected_shuffle_display.py Outdated
Comment thread deeplabcut/gui/tabs/train_network.py Outdated
Comment thread deeplabcut/gui/window.py
Comment thread deeplabcut/gui/utils.py
Comment thread deeplabcut/gui/config_file_monitor.py
Comment thread deeplabcut/gui/tabs/extract_frames.py
Comment thread deeplabcut/gui/tabs/evaluate_network.py Outdated
comparisonbodyparts=bodyparts_to_use,
)
except CONFIG_LOAD_ERRORS as error:
self.root.show_task_error(error, self.root.pose_cfg_path)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this always force the pose_cfg ? Since the config is read too by evaluate_network

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean here, but we could tackle it in a follow-up if you want

@deruyter92
deruyter92 force-pushed the jaap/config-gui-additions branch from 7b949d7 to ddce16f Compare July 17, 2026 15:40
Connect to the directoryChanged signal and add the parent directory
to the watched paths in set_path().  Some editors atomically replace
files (delete + rename), which silently drops a file-only watch in
QFileSystemWatcher.  Watching the parent catches those changes and
re-adds the file path in _check_for_change() as before.
The error signal sets it to True. _show_success_message checks it and skips the dialog entirely if there was an error.
…fig-related in show_task_error

CONFIG_LOAD_ERRORS includes FileNotFoundError, PermissionError,
OSError, TypeError, and ValueError — all of which can originate
from non-config sources (e.g. missing checkpoint, disk full,
wrong type from a GUI callback) when raised by worker threads.
Showing a config-themed dialog with an "Open configuration"
button for those errors was misleading.

Now show_task_error only trusts ValidationError (which is always
from config parsing) to decide on the config dialog.  The
config-loading recovery loop continues to use the full
CONFIG_LOAD_ERRORS tuple where the context is known.

Restore TypeError and ValueError to CONFIG_LOAD_ERRORS so the
recovery loop properly catches those as user-repairable config
errors.
@deruyter92
deruyter92 requested a review from C-Achard July 17, 2026 16:20
@deruyter92

Copy link
Copy Markdown
Collaborator Author

@C-Achard, I've addressed all your comments I think. See the unresolved ones for open questions.

I think it is in a good state, but will do another iteration on Monday before merging it in your PR.

@deruyter92
deruyter92 marked this pull request as ready for review July 17, 2026 16:24
@deruyter92
deruyter92 merged commit a1b5149 into cy/config-gui-error-handler Jul 20, 2026
1 of 3 checks passed
@deruyter92
deruyter92 deleted the jaap/config-gui-additions branch July 20, 2026 12:31
@deruyter92 deruyter92 mentioned this pull request Jul 20, 2026
9 tasks
deruyter92 added a commit that referenced this pull request Jul 21, 2026
* Add GUI formatter for config load errors

Introduce a new `config_errors` dialog helper that builds user-facing configuration error reports via `ConfigErrorReport`. It formats Pydantic validation issues into readable field paths, customizes messages for missing/unsupported settings, includes truncated invalid input values, and adds dedicated handling for missing-file and permission errors with technical details preserved for diagnostics.

* Handle GUI config load errors with recovery

Refactors project loading in the main GUI window to consistently validate config files and recover from failures. It adds a structured error dialog with Retry/Edit/Cancel actions, opens the config editor for repair, and reloads after edits. The flow now routes recent-project opens through the same validation path, clears partial tabs before retrying, and updates project state/load methods to return success status so invalid or unreadable configs no longer leave stale UI state.

* Use system app for config error recovery

Replaces the in-app config repair editor flow with an "Open configuration" action that launches the file in the OS default application, then guides users to save and reload. The config error dialog now supports repeated open/reload attempts in-place, and both project UI initialization and `load_config` were refactored to consistently handle validation and file-read errors with clearer logging and retry behavior.

* Improve shuffle config error label display

Refines `SelectedShuffleDisplay` error rendering when `pose_cfg.yaml` is missing by formatting the message with line breaks and full path visibility. The label is now configured for plain text, word wrapping, expandable sizing, and mouse/keyboard text selection, making long file paths easier to read and copy.

* Fix GUI window maximize behavior

Move window sizing constants to module scope and stop forcing the main window max size to screen dimensions. This allows the maximize button to work properly while preserving the initial resize factor and minimum window size defaults.

* Make DefaultTab content scrollable

Wrap `DefaultTab`’s main content in a `QScrollArea` with a dedicated content widget and zero outer margins so long tab content can be scrolled instead of being clipped. Also updates the `parent` type hint to `QtWidgets.QWidget | None` for consistency with modern typing style.

* Wrap selected videos label in GUI widget

Update `VideoSelectionWidget` so the selected-videos status label can wrap text and expand horizontally. This improves readability when many videos are selected and prevents the text from being clipped in the GUI layout.

* Normalize GUI config paths to absolute

Add a shared `absolute_path` helper in `gui.utils` and use it in `MainWindow` when selecting, opening recent, and reloading projects so config paths are consistently expanded and absolute. Also treat closing the config error dialog as a cancel action to avoid falling through without an explicit choice.

* minor follow-up adjustments for config GUI validation (#3399)

* squashed updates for GUI config validation error handling

* replace getattr guard with explicit attr

- Initialize self._config_monitor = None early in __init__
- Replace getattr(self, "_config_monitor", None) with direct attribute access in config_path setter
- Remove unintentional double @Property decorator on project_folder

* fix: widen worker and evaluate-network error handling

- Catch BaseException in Worker.run() so SystemExit from CLI functions
  is marshalled to the UI instead of silently killing the thread
- Wrap the full evaluate_network method in a single try/except Exception
  so runtime errors (GPU OOM, corrupted models, etc.) are consistently
  reported through show_task_error instead of only catching config errors

* refactor: replace QTimer.singleShot with explicit named reload timers
- Add named _reload_timer members to AnalyzeVideos and ManageProject tabs
  so the timer is inspectable in tests and auto-cancels on tab destruction
- Wire editor.accepted to timer.start instead of a fire-and-forget lambda

* Add `MainWindow.invalidate_config_cache()`

a lightweight hook to force the next cfg access to re-read and validate from disk. Can be called from outside the GUI as well (e.g. after video analysis or training network, in case they manipulate te config)

* test: add coverage for show_task_error, cache invalidation, and reload timer

- Add test_task_error.py: verify generic vs config-error dialog rendering
  and that the 'Open configuration' button only appears for config errors
- Test invalidate_config_cache drops the cached config for the next access
  and is safe to call with no project loaded
- Test that the named _reload_timer (replacing QTimer.singleShot) reliably
  fires the reload callback

* widen train_network error handling, similar to 4472fd3

* deliberately narrow down config errors. (exclude TypeError and ValueError)

* watch config file parent directory to detect atomic replacements

Connect to the directoryChanged signal and add the parent directory
to the watched paths in set_path().  Some editors atomically replace
files (delete + rename), which silently drops a file-only watch in
QFileSystemWatcher.  Watching the parent catches those changes and
re-adds the file path in _check_for_change() as before.

* guard pose_cfg.get("method") string-type

* Add a self._extract_error flag.

The error signal sets it to True. _show_success_message checks it and skips the dialog entirely if there was an error.

* update tests with narrow error types

* Revert narrowing of CONFIG_ERRORS - treat only ValidationError as config-related in show_task_error

CONFIG_LOAD_ERRORS includes FileNotFoundError, PermissionError,
OSError, TypeError, and ValueError — all of which can originate
from non-config sources (e.g. missing checkpoint, disk full,
wrong type from a GUI callback) when raised by worker threads.
Showing a config-themed dialog with an "Open configuration"
button for those errors was misleading.

Now show_task_error only trusts ValidationError (which is always
from config parsing) to decide on the config dialog.  The
config-loading recovery loop continues to use the full
CONFIG_LOAD_ERRORS tuple where the context is known.

Restore TypeError and ValueError to CONFIG_LOAD_ERRORS so the
recovery loop properly catches those as user-repairable config
errors.

* fix MainWindow init _config_monitor

* improve error handling

* update pyproject.toml and uv.lock

* copilot: hardening of PoseConfig-error instance check (Pydantic v2)

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

---------

Co-authored-by: Jaap de Ruyter van Steveninck <32810691+deruyter92@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

config Related to config.yaml, ruamel, YAML parsing, ... enhancement New feature or request GUI issues relating to GUI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants