Add configs and model registry #13
Conversation
- New ABC base_model as template. - Easy access to defined set of models. - Modularly extendable with new implementations.
There was a problem hiding this comment.
Pull request overview
This PR introduces a structured configuration system (grouped dataclass configs) and replaces ad-hoc model loading-by-filepath with a model registry selected via --model_type, updating training/testing/demo entrypoints accordingly.
Changes:
- Added
FMPoseConfigand sub-config dataclasses with helpers (from_namespace,to_dict) plus unit tests. - Implemented a model registry (
register_model/get_model) and migrated main scripts to select models via--model_type. - Restructured the FMPose3D model into a subpackage and registered it under
"fmpose3d".
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_config.py |
Adds tests validating defaults and namespace/dict conversion for new config dataclasses. |
scripts/FMPose3D_train.sh |
Switches CLI usage from --model_path to --model_type. |
scripts/FMPose3D_test.sh |
Switches CLI usage from --model_path to --model_type. |
scripts/FMPose3D_main.py |
Uses registry get_model(args.model_type) instead of importing/loading model from a file path. |
fmpose3d/models/fmpose3d/model_GAMLP.py |
Registers the FMPose3D model and updates imports to new package structure. |
fmpose3d/models/fmpose3d/graph_frames.py |
Minor formatting-only change. |
fmpose3d/models/fmpose3d/__init__.py |
New subpackage initializer exporting Graph and Model. |
fmpose3d/models/base_model.py |
Adds model registry and BaseModel abstraction. |
fmpose3d/models/__init__.py |
Exposes registry APIs and imports model subpackages so registration executes. |
fmpose3d/common/config.py |
Adds configuration dataclasses and FMPoseConfig composition utilities. |
fmpose3d/common/arguments.py |
Replaces --model_path with --model_type argument. |
fmpose3d/common/__init__.py |
Re-exports new config objects from fmpose3d.common. |
fmpose3d/animals/common/arguments.py |
Replaces --model_path with --model_type argument for animals pipeline. |
demo/vis_in_the_wild.sh |
Switches demo script to --model_type (but currently hardcodes an absolute weights path). |
demo/vis_in_the_wild.py |
Uses registry get_model(args.model_type) instead of loading a model from a file path. |
animals/scripts/train_animal3d.sh |
Switches animals training script to pass --model_type. |
animals/scripts/test_animal3d.sh |
Switches animals test script to pass --model_type. |
animals/scripts/main_animal3d.py |
Uses registry get_model(args.model_type) for animals pipeline. |
animals/demo/vis_animals.sh |
Switches animals demo script to pass --model_type. |
animals/demo/vis_animals.py |
Uses registry get_model(args.model_type) for animals demo. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _MODEL_REGISTRY: dict[str, type["BaseModel"]] = {} | ||
|
|
There was a problem hiding this comment.
pyproject.toml declares requires-python = ">=3.8", but this module uses PEP 585 built-in generics (dict[str, ...]) which will raise a TypeError on Python 3.8 at import time. Use typing.Dict/typing.Type (or add from __future__ import annotations and bump the minimum supported Python to >=3.9) to keep compatibility.
There was a problem hiding this comment.
@xiu-cs can we bump the python version or are we somehow restricted to python 3.8?
There was a problem hiding this comment.
from 3.10+ is better (and more accurate!)
| def list_models() -> list[str]: | ||
| """Return a sorted list of all registered model names.""" | ||
| return sorted(_MODEL_REGISTRY) |
There was a problem hiding this comment.
list[str] is also a PEP 585 built-in generic and is not supported on Python 3.8 (the current minimum version in pyproject.toml). Please switch this return annotation to typing.List[str] (or bump the minimum supported Python version).
deruyter92
force-pushed
the
jaap/add_config_and_registry
branch
from
4583e94 to
b73d9ce
Compare
|
@deruyter92 Thanks a lot! I like the new structure. |
…FMPose3D into jaap/add_config_and_registry
3 participants
This PR proposes 2 changes to the repository:
1. Add configuration groups as dataclasses:
Instead of a long list of arguments that are introduced via argparse, I added configuration dataclasses that represent subgroups of configurations. There are many advantages of using dataclasses. For instance, since some modules require only a subset of arguments, it makes sense to separate these. Also dataclasses are much more easier to validate and configure with custom default parameters, they can be read / saved into yaml files, etc.
Note: for now the configs are implemented in parallel to the existing args. I would advise you to replace all occurrences in the code that now uses arg.namespace with the corresponding config dataclass: e.g.
args.myfield->config.myfieldwhere config is an instance of only the corresponding config that is required for that particular module e.g.ModelConfigfor the model.2. Add a model registry instead of loading from module files
Instead of loading from a file specified in
arg.model_path, it would be better to have a modularly extendable registry of models. You only have to choose which of the models from the registry you want to use.Note: in my last commit I made the replacement in all affected files. You can choose to drop this comment if you want to keep the registry only in parallel to your own implementation for now.EDIT: I only added an example registry usage for the human pose model now. Please check for animal model as well.
@xiu-cs, let me know what you think!