Skip to content

Conversation

@jhaux
Copy link
Collaborator

@jhaux jhaux commented Jan 16, 2020

This solves #216 partly. The stacking behavior is as described in the issue: Configs are merged using edflow.util.merge which in turn uses edflow.util.update in the background. Both of these functions are now properly documented, but there is no proper high level documentation.

Copy link
Owner

@pesser pesser left a comment

Choose a reason for hiding this comment

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

Great stuff! A couple of suggestions:
Let's try to ensure we use the same behavior everywhere. For example

config.update(other_config)

config.update(yaml.full_load(f))

any other places? I think it would be best to move load_config (edflow/edflow l. 24) to utils and rely on that function in all other places.
I would also really like either an option or a dedicated command that just prints a final config resulting from a combination of -b <configs..> and --key/path value command line arguments --- one can then easily check that the config looks as intended and pipe the resulting config to a file to put it under version control and reuse it.

@jhaux
Copy link
Collaborator Author

jhaux commented Feb 27, 2020

Great stuff! A couple of suggestions:
Let's try to ensure we use the same behavior everywhere. For example

config.update(other_config)

config.update(yaml.full_load(f))

any other places? I think it would be best to move load_config (edflow/edflow l. 24) to utils and rely on that function in all other places.
I would also really like either an option or a dedicated command that just prints a final config resulting from a combination of -b <configs..> and --key/path value command line arguments --- one can then easily check that the config looks as intended and pipe the resulting config to a file to put it under version control and reuse it.

I will put up a seperate pull request for unified behavior. This should not be too hard but needs proper documentation, such that we can make it easy to add new entry points, which follow the same principles.

The fully updated and final config is displayed richt before iteration starts. I think this should be the point where it is printed, as afterwards, we want it to be fixed.

Copy link
Owner

@pesser pesser left a comment

Choose a reason for hiding this comment

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

#254 also changed config parsing a bit and we really should aim for #255. I think it is best if we fix #255 in one PR. We can keep this one open and update it until that is done, but we should not merge partial solutions since changes in config parsing behavior are quite critical.

@jhaux
Copy link
Collaborator Author

jhaux commented Apr 29, 2020

Agreed! Let's implement the unified entry point handling. Nevertheless I think, we should enhance this PR to solve #255 and not start a new PR, as I am certain, that we would simply reinvent some features that this PR introduces (especially update and merge). What do you think?

@jhaux jhaux mentioned this pull request Apr 29, 2020
@theRealSuperMario
Copy link
Collaborator

ping. Can we get this done soon so that we can make a new release version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants