-
Notifications
You must be signed in to change notification settings - Fork 108
[Feature] Reload experiment without re-passing config #179
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
Conversation
benchmarl/evaluate.py
Outdated
| args = parser.parse_args() | ||
| checkpoint_file = str(Path(args.checkpoint_file).resolve()) | ||
| experiment = reload_experiment_from_file(checkpoint_file) | ||
| try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: for readability i'd suggest adapting the hydra_config util such that you can do:
if hydra_config.checkpoint_from_hydra_run(checkpoint_file)
experiment = hydra_config.reload_experiment_from_file()
else:
# Assume experiment can be loaded directly.
experiment = Experiment.reload_from_file(checkpoint_file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense in the medium term to maintain both hydra-based and non-hydra based saving/loading?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense in the medium term to maintain both hydra-based and non-hydra based saving/loading?
I would think so, the hydra one is more interpretable so it should be the first choice if you can
|
Thanks a lot for the review!! This should be ready now if you have any last comments |
benchmarl/hydra_config.py
Outdated
| try: | ||
| hydra_folder = _find_hydra_folder(restore_file) | ||
| except ValueError as e: | ||
| if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feel free to ignore style comments, but the best way to do this would be to raise a more specific error within _find_hydra_folder that you can catch here. I.e. MissingHydraMetadataError(FileNotFoundError). If someone changes the exception string in _find_hydra_folder, they may have no idea they are changing the functionality here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanksss for pushing me out of my lazyness ahahah
fixes #178
cc @itwasabhi