[dev] Configs refactor - Config refactors review: suggested tweaks and tests (Review only) #3366
Closed
C-Achard wants to merge 7 commits into
Closed
[dev] Configs refactor - Config refactors review: suggested tweaks and tests (Review only) #3366C-Achard wants to merge 7 commits into
C-Achard wants to merge 7 commits into
Conversation
Apply a set of small fixes and cleanup changes across multiple modules: fix Path/string handling in weight_init; remove many redundant `pass` stubs in abstract classes and placeholders; adjust loop/range usages (camera calibration, tracklets, DataFrame index); improve legacy-argument handling in Loader (DeprecationWarning with stacklevel, error messages) and infer model config path; reorder and add/remove imports where appropriate; avoid silently swallowing exceptions after printing (materialize, make_labeled_video); use tuple form for startswith check in auxiliary functions; tweak test random.sample range; and other minor formatting/consistency tweaks. These changes are intended to improve correctness, clarity, and maintainability without altering core behavior.
Context: it was trying to run on numpy arrays which pydantic considers an arbitrary type
Introduce tests/core/config/test_config_breakage.py covering pathological cases for the centralized config model. Tests exercise in-place nested mutation validation (xfail), dirty-state isolation between instances, change-note handling and alias-to-canonical mapping, nested YAML comments (xfail), and normalization/serialization of nested models containing Path and Enum values. Uses ProjectConfig, DLCBaseConfig and DLCVersionedConfig to assert expected dirty-tracking, logging, validation, and YAML output behaviors.
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
1 participant
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.
@deruyter92 Here are some small tweaks and improvements, including potentially interesting edge cases tests currently marked as xfail.
This should be considered part of the ongoing review of #3354 / #3198, so some changes, tests and comments may be complementary.
Caution
This is not meant to be merged as-is, as I had to rebase and run pre-commit on all to get proper comparisons with main. Please cherry-pick fc9a599 to a preferred dev branch of the config refactor instead.