feat: cleanup config classes by LuukBlom · Pull Request #1372 · Deltares/hydromt

LuukBlom · 2026-02-10T16:16:43Z

This PR is meant as a suggested improvement to #1365 to make reviewing these changes easier.

Issue addressed

Fixes #

Explanation

Explain how you addressed the bug/feature request, what choices you made and why.

General Checklist

Data/Catalog checklist

data/catalogs/predefined_catalogs.yml has not been modified.
None of the old data_catalog.yml files have been changed
data/changelog.rst has been updated
new file uses LF line endings (done automatically if you used update_versions.py)
New file has been tested locally
Tests have been added using the new file in the test suite

Additional Notes (optional)

Add any additional notes or information that may be helpful.

…romtModelStep into 1 class with a runtime function: 'bind_to_model'

Copilot

Pull request overview

This PR refactors configuration-related classes to improve clarity and maintainability by renaming classes and attributes to better reflect their purpose. The changes primarily involve renaming HydromtComponentConfig, HydromtGlobalConfig, HydromtModelSetup, HydromtModelStep, and RawStep to more descriptive names (ComponentSpec, ModelSpec, WorkflowSpec, and WorkflowStep), and renaming the args parameter to kwargs throughout the codebase.

Changes:

Renamed configuration classes to more intuitive names (e.g., HydromtModelSetup → WorkflowSpec)
Changed args parameter to kwargs to better reflect Python conventions
Refactored step execution logic by replacing to_model_step() with bind_to_model() and consolidating step binding and execution

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`hydromt/_validators/model_config.py`	Renamed classes (`ComponentSpec`, `ModelSpec`, `WorkflowSpec`, `WorkflowStep`) and refactored step binding logic
`hydromt/model/model.py`	Updated imports and simplified step execution by using list comprehensions
`hydromt/parsers.py`	Updated imports and class references to use new names
`hydromt/readers.py`	Updated import and return type annotation to `WorkflowSpec`
`hydromt/_validators/__init__.py`	Updated exports to use new class names
`tests/_validators/test_model_config.py`	Updated all test cases to use new class names and test the refactored API
`tests/test_readers.py`	Changed `args` to `kwargs` in test assertion

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-11T12:30:58Z

+            [step.bind_to_model(self) for step in raw_steps]
+            [step.execute() for step in raw_steps]


Replace list comprehensions with for loops. List comprehensions are intended for creating new lists, not for executing side effects. Use explicit for loops when calling methods solely for their side effects.

Suggested change

[step.bind_to_model(self) for step in raw_steps]

[step.execute() for step in raw_steps]

for step in raw_steps:

step.bind_to_model(self)

for step in raw_steps:

step.execute()

Copilot · 2026-02-11T12:30:59Z

+            [step.bind_to_model(self) for step in raw_steps]
+            [step.execute() for step in raw_steps]


Replace list comprehensions with for loops. List comprehensions are intended for creating new lists, not for executing side effects. Use explicit for loops when calling methods solely for their side effects.

Suggested change

[step.bind_to_model(self) for step in raw_steps]

[step.execute() for step in raw_steps]

for step in raw_steps:

step.bind_to_model(self)

for step in raw_steps:

step.execute()

hboisgon

Comments are mixed between this PR and the other one. I saw this one after I started reviewing. Still not fully sure what to think about this refactor. I guess better to validate when reading the yaml before calling build and update to get errors a little earlier but wonder if this does not make it harder to use for our users. Definitely requires docs.

Also would be nide to have a step back on what we are trying to do and steps to build/update a model with hydromt and terminology?

It was an important comment when we had an external audit on the code but the CLI build function is not linked to a model function ie CLI hydromt build does extra things including read_workflow_yaml which model.build does not do. Maybe moving validation around now highlights we should definitely work on this. Hence would be good to step back from the implementation and think again about what we are trying to do when building a model.

hboisgon · 2026-02-12T05:31:30Z

See my comment in the other PR. I think users should know the content of the these classes if this is what they get when calling the read_workflow_yaml function. This should move into hydromt.model and be renamed to model_workflow.py to avoid confusion with the ConfigComponent

hboisgon · 2026-02-12T05:45:19Z

+            raise ValueError(
+                f"Model instance of type {type(model_instance).__name__} does not have 'components' attribute"
+            )
+        elif not isinstance(model_instance.components, dict):


Is this case at all possible?

hboisgon · 2026-02-12T05:48:11Z


-class RawStep(BaseModel):
-    """Represents the YAML structure BEFORE runtime wiring."""
+class WorkflowStep(BaseModel):


I thought HydroMTModelStep was clearer also with the @hydromt_step decorator. Again steps can be defined and called outside of the workflow yaml so we maybe are too restrictive by using the name Workflow here?

hboisgon · 2026-02-12T05:50:14Z

-        kwargs = self.args or {}
-
+        """Execute this workflow step by calling the bound function with the provided arguments."""
+        if self._method is None:


Can't you first try and call bind_to_model and then if still None return an error? I'm a little fuzy about this works still when calling model.build and model.update sorry.

hboisgon · 2026-02-12T05:52:16Z

-        bound_args = {
-            param: p.default
-            for param, p in sig.parameters.items()
-            if p.default != inspect._empty
-        }
-        bound_args.update(kwargs)


Is this still happening somewhere? It's important for reproducibility if the complete function arguments used appera in the log and not just the one the user provided/changed.

hboisgon · 2026-02-12T06:07:30Z

-class HydromtGlobalConfig(BaseModel):
-    components: list[HydromtComponentConfig] = Field(default_factory=list)
-    data_libs: list[str | Path] = Field(default_factory=list)
+    components: list[ComponentSpec] = Field(default_factory=list)


I never looked into this before but why a list? In both model init and in the workflow yaml they are defined as dictionnaries so why change the type here?

hboisgon · 2026-02-12T06:13:30Z

+    The `modeltype` field specifies the model class to instantiate, which can be given as a string referring to a registered model plugin or as the class itself.
+    The `globals_` field contains the model configuration that will be passed as kwargs to the model constructor when instantiating the model.
+    The `steps` field is a list of workflow steps that will be executed in order as part of the model run.


I dont think the documentation is clear enough for users to be able to use whatever comes out of read_workflow_yaml.

modeltype is "wflow_sbm" or WflowSbmModel (second).

globals: will any argument of model.init be a property that you can call and keep the type model init expects? I see components is a list here and model init wants a dict? WflowSbmModel has extra a config_filename. How is this dealt with? If a model has extra arguments in init that also should be converted from relative to absolute paths how will this work? Do plugins need to extend this class? If so this should be documented in the plugins docs.

steps: steps won't really be a list of dict anymore right? already the step name is added with name property to the arguments dict? How can users modify steps arguments before calling model.build or model.update if they need to? Not that I mind working with python classes instead of dict but I do need documentation then.

hboisgon · 2026-02-12T06:16:07Z

-    modeltype: type
-    globals_: HydromtGlobalConfig = Field(alias="global")
-    steps: list[RawStep]
+class WorkflowSpec(BaseModel):


Same as above, I'm not sure workflow is the right name? Is ModelStepSpec or StepSpec clearer.

deltamarnix · 2026-02-12T08:12:47Z

I haven't done a full review myself yet, but I did have a conversation with Luuk before he started on this PR.

I understand Helene's concern that validation is now separate from the actual model building. One of the things that I discussed with Luuk was to make the core Model base class a pydantic.BaseModel. And then we could make it so that the yaml is directly read into a Model, or WflowModel, or whatever plugin model. Then we can get rid of all these workflow classes, because the components are pydantic serializable themselves. But that would be quite a change, because it might have an impact on the plugins too. Although I do think that would be a nice addition. It will simplify the whole reading process.

rename model config classes and attributes. Also join RawStep and Hyd…

bed6cd4

…romtModelStep into 1 class with a runtime function: 'bind_to_model'

LuukBlom requested a review from deltamarnix February 10, 2026 16:16

deltamarnix requested a review from Copilot February 11, 2026 12:30

Copilot AI reviewed Feb 11, 2026

View reviewed changes

hboisgon reviewed Feb 12, 2026

View reviewed changes

LuukBlom mentioned this pull request Feb 24, 2026

Replace raw dicts with documented Pydantic classes for api functions #1382

Open

deltamarnix marked this pull request as draft May 5, 2026 14:16

		[step.bind_to_model(self) for step in raw_steps]
		[step.execute() for step in raw_steps]

Uh oh!

Conversation

LuukBlom commented Feb 10, 2026

Issue addressed

Explanation

General Checklist

Data/Catalog checklist

Additional Notes (optional)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

hboisgon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deltamarnix commented Feb 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants