Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ConfigComponent #863

Merged
merged 40 commits into from
Apr 5, 2024
Merged

Add ConfigComponent #863

merged 40 commits into from
Apr 5, 2024

Conversation

savente93
Copy link
Contributor

@savente93 savente93 commented Mar 29, 2024

Issue addressed

Fixes #823
Fixes #799

Explanation

The implementation is almost identical to the original. The only thing that really changed was the name, which I put my reasoning for in #862

Checklist

  • Updated tests or added new tests
  • Branch is up to date with v1
  • Tests & pre-commit hooks pass
  • Updated documentation if needed
  • Updated changelog.rst if needed
  • For predefined catalogs: update the catalog version in the file itself, the references in data/predefined_catalogs.yml, and the changelog in data/changelog.rst

Additional Notes (optional)

Add any additional notes or information that may be helpful.

@savente93 savente93 marked this pull request as ready for review March 29, 2024 14:49
Copy link
Contributor

@hboisgon hboisgon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start @savente93 but we are not there yet.
First on the name of this component. I think ConfigComponent is good enough as hydromt config is not a component of model.

Then on the functionality:

  • one function was not transferred: Model.setup_config
  • the behavior of Model.config is not properly transferred

I elaborated a bit more in the comments but config was a particular object in Model that was always created on the fly from a template instead of the user specifically creating it. I think if we want to achieve full flexibility, we can simplify the behavior and harmonize with other components. My suggestion would be:

  • remove config_fn from Model.__init__
  • keep allowing to create a config using a template file. This can be achieved with either (1) add the config filename to ConfigComponent.__init__ and transfer the rest of the behavior as in main. Or (2) add a create function that allows to start a config from a template config file (more in line with other components but a bigger change for the user).

I think I would prefer option 2. @DirkEilander any thoughts on this?

@savente93
Copy link
Contributor Author

@hboisgon I see. I'm happy to update the functionality I'll get to work on that, but the name I feel a bit more strongly about. I don't really care what the affix will be (kernel/simulation/output/etc.) but I do feel quite strongly about it not being just ConfigComponent as this has caused me quite some confusion even while writing this PR. I'm happy to see if we can find a middle ground, but ConfigComponent is too generic in my opinion.

@savente93 savente93 requested a review from hboisgon April 2, 2024 12:33
@savente93 savente93 changed the title Add KernelConfigComponent Add ConfigComponent Apr 3, 2024
@hboisgon
Copy link
Contributor

hboisgon commented Apr 4, 2024

Hi @savente93 I added the changes to have config behave like other component and a method to create from template. I did not have time yet to create tests for it. Could you look at the implementation I made and finalize from there?

@savente93
Copy link
Contributor Author

@hboisgon I think it should all be good now. I added the note about our discussion during our last refinement in the migration guide instead of the plugin dev because that is still referencing the state on main so I didn't think there was a good place to put it there without being very out of place or rewriting the entire page, hope this is okay to you. If not please let me know where you think it should go and I'll put it there. Other than that, I think we're all in agreement now. If so, can you give the final approve?

Copy link
Contributor

@hboisgon hboisgon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating and adding tests @savente93 !
Because we have a create function now, read for config can work similar to other components ie we dont read in write only mode.
Also I don't think we should add ConfigComponent by default in Model as the ConfigComponent will not suit every plugins. Also the filename can be changed through the components arguments by the user so they dont need to be arguments of model init

@savente93 savente93 requested a review from hboisgon April 5, 2024 08:09
@savente93
Copy link
Contributor Author

@hboisgon should be all good now!

Copy link
Contributor

@hboisgon hboisgon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good now !

Two small comments before you merge:

  • before merging, seems that the tables stuff is still in model.py? Did you merge properly with v1?
  • while talking with @deltamarnix , we saw that in main, when reading yaml file, we would parse "None" string in yaml to the python None. This seems this hasn't been transferred here. I think it still good to have (both for hydromt yaml and for configcomponent) but you can also decide to add in another PR.

@savente93
Copy link
Contributor Author

oh wow good catch! thanks for that, you're right!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants