-
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
Improve zen pickle-compat and support for hydra_main(config_path)
#384
Conversation
Note to self: add pickle compat test |
hydra_main(config_path)
@Jasha10 I wonder if you have any thoughts about this solution -- is there anything hazardous with the solution I landed on? Any ideas about making this work for Hydra < 1.3.0? |
I.e. using a wrapper? It looks good to me!
Since the ...
if isinstance(config_path, str) and HYDRA_VERSION < Version(1, 3, 0): # pragma: no cover
target = self
target.__module__ = self.func.__module__
elif Version(1, 3, 0) <= HYDRA_VERSION and isinstance(config_path, str):
...
target = wrapper
else:
target = self
... To maintain some sense of decorum, you could even use a try:
return hydra.main(**kw)(target)()
finally:
if self_module_was_fudged:
self.__module__ = self_module_old Speaking of "hazardous solutions," hacking the |
if (config_path is _UNSPECIFIED_ and HYDRA_VERSION < Version(1, 2, 0)) or ( | ||
isinstance(config_path, str) and HYDRA_VERSION < Version(1, 3, 0) | ||
): # pragma: no cover | ||
warnings.warn( | ||
"Specifying config_path via hydra_zen.zen(...).hydra_main " | ||
"is only supported for Hydra 1.3.0+" | ||
) |
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.
The warning message would only seem to be relevant if config_path
has been specified. I'm confused about why it's necessary to check whether config_path is _UNSPECIFIED_
in this if
statement.
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 did this because if version_base
is Hydra 1.1 and config_path
is _UNSPECIFIED_
then Hydra defaults config_path
to be "."
But I realize that I need to have additional logic here to handle the case where the version_base
via zen
is specified
Great! 🚀
This is an interesting idea. I tried this and it seems that when the module becomes I haven't been able to find a complete backwards-compat w/ Hydra solution, so I think I will just punt and promise functionality for Hydra 1.3.0+. Fortunately the upgrade from 1.2 to 1.3 should be quite benign for most people. Thanks for the help @Jasha10 ! |
Zen
instances pickle-compatibleZen.hydra_main(config_path)
functionality for Hydra 1.3.0+ (Closesconfig_path
broken forhydra_main
#381)By default
hydra.main(config_path)
treatsconfig_path
as being relative to the directory that houses the file in which the task function is defined. Hydra has some relatively complicated logic for determining this directory.Because
zen(task).hydra_main(...)
sends a zen-wrapped task function tohydra.main
, the location oftask
cannot be determined. I did not appreciate what was going on under the hood here until #381 was opened. Fortunately, Hydra 1.3.0 added support for wrapped task functions (i.e. it will follow a__wrapped__
attribute to find the task function.Thus, in the case that
config_path
is a string and Hydra 1.3.0+ is installed,zen
will now produce a temporary wrapped function that points to the original task function. The reason whyzen
doesn't always do this is that the resulting wrapped function is not pickle-compatible, whereas the zen-instance is. It is possible in the future that pickle-compatible task functions could be useful for some launchers.Unfortunately, I was not able to find a way to make
config_path=<str>
work for Hydra 1.2.0 or earlier. Thus we emit a warning to users.Given:
config.yaml
Before:
After
(Hydra 1.3.0+ only)