-
Notifications
You must be signed in to change notification settings - Fork 15
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
Compatibility-breaking change in hydra 1.1.0dev7 #52
Conversation
config=task_cfg, | ||
task_function=task_function, | ||
job_dir_key="hydra.run.dir", | ||
job_subdir_key=None, | ||
configure_logging=False, | ||
) | ||
|
||
callbacks.on_run_end(config=task_cfg, config_name=config_name, job_return=job) |
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.
@jgbos it doesn't look like .on_run_end()
necessarily accepts job_return
; it looks like .on_job_end()
does (which we don't have to worry about).
Is there a specific reason why you specified this in our case?
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.
i just copied that from one of hydra's test cases
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.
Hmm.. Well I'm thinking we may want to remove it in case it creates any issues with callbacks that don't expect it
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.
oh whoops, i misread that, I was thinking it was in the test. The code came from Hydra's code:
https://github.com/facebookresearch/hydra/blob/master/hydra/_internal/hydra.py#L107
Probably best to just stick with the signature in the abstract method.
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.
Ah! Hmm that might be worth adding back in then just so that we mirror them. Good catch / sleuthing!
This is the culprit:
facebookresearch/hydra#1581
It looks like Hydra has support for callbacks now. I don't have very deep insight into how it works yet.
@jgbos I took a swing at fixing the compatibility-breaking change, but definitely take a look at their changes yourself. I am not confident that my "fix" is totally general / appropriate!
Note that
run_job
now also accepts an optionalhydra_context
argument. It looks like we need to pass it a context:It also looks like
hydra.initialize()
should be passed aconfig_path
in_load_config()
(although it looks like it is valid to / we may want to just passNone
)Some other things:
hydra.experimental
.