minor follow-up adjustments for config GUI validation - #3399
Conversation
C-Achard
left a comment
There was a problem hiding this comment.
Amazing follow-up, thank you! Great that you added GUI tests as well, will be very useful.
| config = self.root.config | ||
| editor = ConfigEditor(config, parent=self.root) | ||
| editor.accepted.connect( | ||
| lambda: QTimer.singleShot( |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
ok good point, I will think of a different implementation.
There was a problem hiding this comment.
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?
| config = self.root.config | ||
| editor = ConfigEditor(config, parent=self.root) | ||
| editor.accepted.connect( | ||
| lambda: QTimer.singleShot( |
There was a problem hiding this comment.
see above answer
- 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
left a comment
There was a problem hiding this comment.
Here are a few additional comments; thanks again!
| CONFIG_LOAD_ERRORS = ( | ||
| ValidationError, | ||
| FileNotFoundError, | ||
| PermissionError, | ||
| OSError, | ||
| TypeError, | ||
| ValueError, | ||
| ) |
There was a problem hiding this comment.
Isn't this a bit broad? How can we ensure it is indeed config related?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
the show_task_error method (which receives errors from arbitrary worker threads) now only treats ValidationError as unambiguously config-related
There was a problem hiding this comment.
The CONFIG_LOAD_ERRORS tuple is kept only for the config-loading recovery loop,
| comparisonbodyparts=bodyparts_to_use, | ||
| ) | ||
| except CONFIG_LOAD_ERRORS as error: | ||
| self.root.show_task_error(error, self.root.pose_cfg_path) |
There was a problem hiding this comment.
Should this always force the pose_cfg ? Since the config is read too by evaluate_network
There was a problem hiding this comment.
Not sure what you mean here, but we could tackle it in a follow-up if you want
7b949d7 to
ddce16f
Compare
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.
|
@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. |
* 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>
Summary
Some add-on follow-up commits to merge/cherry-pick into #3397
83bf20d8) and deduplicate error handling (35eb0eb6).ProjectConfigthroughout the GUI (a37f5ef6,e4832ed1).d649f6df,2f82e030).83200b18,12d07b2a).1cb4d940).compat.py(4f9cf4af).991d9f77).Tests
Some basic tests for the new behavior are added here: