[dev] C7 - Configs refactor: finalize and simplify infrastructure #3354
[dev] C7 - Configs refactor: finalize and simplify infrastructure #3354deruyter92 wants to merge 4 commits into
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.
| def print( | ||
| self, | ||
| indent: int = 0, | ||
| print_fn: Callable[[str], None] | None = None, | ||
| ) -> None: | ||
| pretty_print(config=self.to_dict(), indent=indent, print_fn=print_fn) |
There was a problem hiding this comment.
Would a __repr__ be useful ?
| ) | ||
| return self | ||
|
|
||
| def to_dict(self) -> dict: |
There was a problem hiding this comment.
This needs the normalize keyword, right? Since super().to_yaml calls dict_data = self.to_dict(normalize=True)
| @@ -35,8 +33,7 @@ class DataLoaderConfig(ConfigMixin): | |||
| type: str | |||
There was a problem hiding this comment.
Is this needed? I don't think this is used anywhere now
There was a problem hiding this comment.
_post_yaml_load_updates calls the parent class function, but the parent just passes. Is this intended?
There was a problem hiding this comment.
Is it some sort of future proofing? What would be potential use cases if yes?
There was a problem hiding this comment.
if project_path.resolve() != self.project_path.resolve():resolve() or absolute() here?
| return resolved | ||
|
|
||
|
|
||
| def normalize_for_serialization(obj: Any) -> Any: |
There was a problem hiding this comment.
Does this need to handle list/tuples/set recursively?
| }, | ||
| ) | ||
|
|
||
| # Annotation data set configuration (and individual video cropping parameters) |
There was a problem hiding this comment.
| # Annotation data set configuration (and individual video cropping parameters) | |
| # Annotation dataset configuration, individual video cropping parameters |
| multianimalbodyparts: list[str] = field(default_factory=list) # multi-animal project key | ||
| unique_bodyparts: list[str] = field(default_factory=list) # metadata key; same as uniquebodyparts | ||
| individuals: list[str] = Field(default_factory=lambda: ["individual_1"]) | ||
| uniquebodyparts: list[str] = Field(default_factory=list) # multi-animal project key |
There was a problem hiding this comment.
This appears several times, I assume the one with aliases defs should stay?
| @@ -126,9 +127,9 @@ class ProjectConfig(ChangeTrackingMixin, MigrationMixin, ConfigMixin): | |||
| colormap: str = "rainbow" | |||
There was a problem hiding this comment.
What would be the plan for validation/documenting valid values? This will come at a later time, right? (Apologies if we discussed that point already)
| json_schema_extra={"comment": "Alias for 'uniquebodyparts'. Kept for backwards compatibility."}, | ||
| ) | ||
|
|
||
| # TODO @deruyter92 2026-02-06: These parameters are no longer used in the new pipeline. |
There was a problem hiding this comment.
Should we comment them out then? If it's really dead code
There was a problem hiding this comment.
Most model config is in the pytorch backend, but this is "core" config. Is this desirable, or would it be good to move?
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: