[dev] C7 - Configs refactor: finalize and simplify infrastructure - #3354
Conversation
|
@C-Achard, would be very helpful if you could give an intermediate review on the deeplabcut.core.config module and corresponding tests. (diff with main, there are some intermediate states that are irrelevant). Thanks a lot!! |
C-Achard
left a comment
There was a problem hiding this comment.
@deruyter92 Here are some first comments, mostly focusing on usability/possible refactor artifacts.
Note that I only reviewed the diff of C7 to the feature branch; happy to review the full version once this is finalized.
deruyter92
left a comment
There was a problem hiding this comment.
Thanks @C-Achard. Addressed all your comments. Let's discuss next week.
I have a few extra adjustments in mind (better hook-up in codebase), but after that it's ready for proper stress-testing.
deruyter92
left a comment
There was a problem hiding this comment.
@C-Achard, thanks for the very helpful review!
I addressed most comments and cherry-picked from #3366 what was needed.
Remaining tasks for me:
- address the remaining xfails and add more stress-tests if needed.
- decide on arbitrary types (dd5b469), I think allowing them might not be ideal and not needed per se.
- improve constructors in make_pose_config, modelzoo etc.
- re-visit core config utils and see what needs to operate on dict still, or can be changed to typed.
- revisit all todos to see if they are/can be addressed or we will do that in next version.
| for key, value in edits.items(): | ||
| cfg[key] = value |
There was a problem hiding this comment.
Ideally not. The favorable alternative is DLCBaseConfig.update() now, but we cannot deprecate it until all call sites are adjusted as well (including some TF branches that use configs that fall outside the current schemas).
I will revisit this after updating the main PoseConfig / TestConfig constructors. Probably most call sites can be fixed. I will also properly document that we are planning to remove this.
15aae98 to
6812f3f
Compare
|
@C-Achard pushed my latest commit for now, happy to take your review again. Let me know if anything is unclear. I have rebased on main for easier testing. Just so you know, you have reviewed everything up to 6812f3f before the rest is new. But any comments are welcome, also on prior code if you find anything. Most relevant commits are probably 2c3388c and ee524e5. Thanks again! |
C-Achard
left a comment
There was a problem hiding this comment.
Here are a few additional comments, nohting major really so feel free to skip some if they feel too tangential. Overall after some digging I could not really find fundamental design issues I think should be addressed, so I think we can roll this out soon!
Let me know if you would like further review as well, but it might mostly be minor details at this point.
| @property | ||
| def bodyparts_list(self) -> list[str]: | ||
| # Animal-count agnostic; Always return a list (never "MULTI!", None, etc.) | ||
| if self.multianimalproject: | ||
| return list(self.multianimalbodyparts) | ||
| if self.bodyparts == "MULTI!": | ||
| raise ValueError("bodyparts must be a list of strings when multianimalproject is False, got 'MULTI!'") | ||
| return list(self.bodyparts) |
There was a problem hiding this comment.
Should we add a few tests for this, since it is used in many places but not tested directly?
class TestProjectConfigBodypartsList:
def test_single_animal_bodyparts_list_returns_bodyparts(self):
cfg = ProjectConfig(multianimalproject=False, bodyparts=["nose", "tail"])
assert cfg.bodyparts_list == ["nose", "tail"]
def test_multi_animal_bodyparts_list_returns_multianimal_bodyparts(self):
cfg = ProjectConfig(
multianimalproject=True,
bodyparts="MULTI!",
multianimalbodyparts=["nose", "tail"],
)
assert cfg.bodyparts_list == ["nose", "tail"]
def test_single_animal_multi_sentinel_is_not_split_into_characters(self):
cfg = ProjectConfig(multianimalproject=False, bodyparts="MULTI!")
assert cfg.bodyparts_list == []There was a problem hiding this comment.
Agreed, good solution. Copied with one minor adjustment.
|
@C-Achard, thanks for the review, very helpful! I've addressed all your comments, a few might things might be worth noting the rest is minor:
|
Thanks again! |
…, field validation This commit introduces several refactors and simplifactions: 1. Settle with Pydantic BaseModel (no OmegaConf or Pydantic dataclasses, or Mixins) 2. Introduce a separate PoseMetadataConfig, rather than using a ProjectConfig as metadata field on the PoseConfig 3. Add a validation suite for field validators e.g. BodypartPair, UniqueStrList, etc 4. Clean up / update the schemas where necessary (e.g. default values.) 5. Better normalization / serializion for tuples, lists, ndArrays (WeightInitialization) 6. Simplify change tracking on the DLCBaseConfig 7. Simplify versioning migration: only `from_dict`, not on the full class 8. remove duplicate fields and add linting rule 9. add BaseConfig `set_nested` method 10. ProjectConfig: add convenience repair project path
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. - Prevent notes on missing fields. - Fix yaml comments for nested config fields. - fix validation for None-type read_config_as_dict. - VersionedConfigs use default factories for PrivateAttrs. - add missing snapshot_prefix to runner (SA inference requires it). - fix some faulty default values for modelzoo config templates
1. Refactor PoseConfig: separate modules and add canonical build method. - Split enums into separate file - Split PoseMetadata into separate file - Split PafParamters into separate file - Add canonical build methods for structured configs: e.g. build from project config 2. Refactor make_pose_config: - build entire default dict for net_type from the yaml file, before validation. - deprecate `make_pytorch_test_config`, `make_pytorch_pose_config` and `make_basic_project_config`. 3. Refactor modelzoo - superanimal configs construction via build method
1. Add tests for centralized config edge cases 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. add test for cross-field validation with bulk updates 2. fix invalid test imaug (faulty fixture) see DeepLabCut/UnitTestData#4 3. Update testscript_pytorch_multi_animal: ctd_conditions required for ctd shuffle. The documented primary workflow is: set conditions at shuffle / dataset creation, not defer until inference. This was not reflected in the test, and surfaced with the new structured configs. This commit makes the test in line with recommended workflow in API docstring; Config docs; BUCTD COLAB notebook; and GUI. -> ctd_conditions are passed at dataset creation time. making the shuffle a fully complete artefact, rather than somthing that requires patching in later API calls. 4. fix testscript multianimal transfer learning pass net_type when creating training dataset 5. add tests: resume training from existing snapshot add tests for pose config creation refactor test_pose_config_creation (test cases for migration to typed) update xfail for cross fields overrides remove comment tests fix test_apis_training make migrations fixtures to fix registry pollution use tmp instead of tempfile tests/core/config: catch expected DeprecationWarnings for aliases move TEST_DATA_DIR to fixture use centralized yaml loader instead of pyyaml update test_modify_train_test_cfg_sets_expected_values update documentation and tests for write_project_config update test_pose_config_creation copy dict instead of mutate update test_pose_config_creation, use centralized yaml loader add test for ProjectConfig bodypartslist update test_pose_config_creation separate testing of saving
expose get_yaml_loader get_yaml_dumper in core.config
…old API - export.py - deprecate update_config_by_dotpath and update_config - simplified schema DetectorDataConfig instead of DataConfig use centralized read_config in trainingsetmanipulation hook up trainsetmanipulation.py to typed ProjectConfig update callers of edit_config to use typed updates instead
Notable: revert default `box_score_thresh` in `DetectorModelConfig` currently a different box_score_thresh is used in inference (0.01) and evaluation (0.6), putting a default value in the config would change behavior. move deprecation.py from utils to core
bd60da2 to
a285f5e
Compare
This PR is part of the WIP (see #3198) for migrating from dictionary configs to typed & validated configurations (overview is tracked in #3193).
This PR finalizes, cleans and simplifies the infrastructure for structured configs before moving migrating to the structured config (schema V1).
Changes
DLCBaseConfig, DLCVersionedConfig
The previous versions of the structured configs used OmegaConf (because of dict-like behaviour and easy merging capabilities), or Pydantic dataclasses (lightweight validation, easy to tailor). However the result was still a lot of back-and-forth switching between types and potentially difficult to understand collection of mixins.
This PR settles for a simpler customized solution: a strict Pydantic BaseModel, subclassed by two DLC-custom classes.
All legacy references to OmegaConf are removed
Hook-up in main API
Normalization: the function signatures for all main API are changed to accept
str | Path | dict | ProjectConfig(or similar) which are all normalized to the typedProjectConfigat the beginning. This means that all downstream code now uses the validated schema.Construction: The construction of configs used to be a bit of a spaghetti of loading yamls, adjusting fields midway, and then updating them once more in the API calls, such as train_network. The structured configs refactor improved this a bit, but there was a mixed state with validation halfway construction, requiring the need for nullable fields and moving a lot back and forth between dicts and types. see discussion on this topic in #3369 (merged/included in the current PR)
This PR settles on a two-step approach:
Note: API calls can still update the PoseConfig with overrides (e.g. train_network), but these overrides are never required: the validated PoseConfig is always considered a fully initialized model that can be used downstream.
Testing
more tests will be added if needed [wip]