Hydra integration #1035
Hydra integration #1035
Conversation
henrykironde
left a comment
henrykironde
left a comment
There was a problem hiding this comment.
Thanks @jveitchmichaelis, the changes seem fine, I have made some suggestions where I feel there may be some issues. Let me know what you think.
| Args: | ||
| num_classes (int): number of classes in the model | ||
| config_file (str): path to deepforest config file | ||
| num_classes (int): number of classes in the model |
There was a problem hiding this comment.
Can we keep the order of the args the same as defined in the function
There was a problem hiding this comment.
Yeah I think for now that's a good idea.
Long term (2.0?), I think we could encourage users to go via the config route and provide fewer selected keyword arguments here. I'll have a look at how other projects have done this because there's probably a balance.
src/deepforest/main.py
Outdated
| if config is None: | ||
| config = utilities.load_config("config", overrides=config_args) | ||
| elif 'config_file' in config: | ||
| # This is from the hub |
There was a problem hiding this comment.
Do we need this comment here.
There was a problem hiding this comment.
Yeah mostly included for awareness - this condition can be deprecated at some point, when we revise the hub model. Possibly we want to update the checkpoint alongside this?
| self.config = config | ||
|
|
||
| # If num classes is specified, overwrite config | ||
| if not num_classes == 1: |
There was a problem hiding this comment.
I expected that if num_classes is set to 1 by default, that value would be used unless explicitly overridden. Since Hydra manages config composition and sanity checks, we shouldn't need to add extra if conditions—either the default is used or an override is applied automatically.
There was a problem hiding this comment.
The condition is from the existing code to declare it's deprecated, so I assumed we'd want to keep it (if we're also keeping the same constructor signature)?
tests/test_main.py
Outdated
| @pytest.mark.xfail | ||
| def test_custom_config_file_path(ROOT, tmpdir): | ||
| m = main.deepforest( | ||
| config_file='{}/deepforest_config.yml'.format(os.path.dirname(ROOT))) |
There was a problem hiding this comment.
I think we should be able to swap to the new config file.
| cfg = compose(config_name=config_name) | ||
| cfg.merge_with(overrides) | ||
| else: | ||
| if isinstance(overrides, str): |
There was a problem hiding this comment.
What happens to the list type if parsed to the function.
There was a problem hiding this comment.
The reference example for compose uses a list of key=value strings as if you've passed in things from the command line.
https://hydra.cc/docs/advanced/compose_api/
So depends how we expect users to override the config and what "user experiences" we imagine. Maybe we can discuss this in the issue?
|
Looks good, I'll fix up the tests this week and think about adding some new documentation with usage examples. Some of the issues you raised might need a bit more discussion. |
jveitchmichaelis
force-pushed
the
hydra-integration
branch
from
4659194 to
9dcf846
Compare
jveitchmichaelis
force-pushed
the
hydra-integration
branch
from
9dcf846 to
e8b57a9
Compare
|
@henrykironde once the tests pass, check the revisions out and if you're happy, this can all be squashed. |
henrykironde
left a comment
henrykironde
left a comment
There was a problem hiding this comment.
This looks good overall. We should add some more tests for load_config with the expected arguments types in a follow-up. For now, I’ll go ahead and merge this since the PR is already quite large.
2 participants
For review - there are some outstanding todos in the issue thread, mainly around documentation and whether we want to include some additional usage scenarios at this point (like CLI, SLURM tests, etc).
Suggest filtering by this commit first c664dbc
Addresses #847