-
-
Notifications
You must be signed in to change notification settings - Fork 639
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
Adding the argument target_wrapper
to hydra.utils.instantiate
to support recursive type checking
#2880
base: main
Are you sure you want to change the base?
Conversation
Added `target_wrapper` arguments.
@leycec you might find this interesting. |
Makes sense to me. But it seems to me (from a distance -- I'm not super familiar with the intantiation code) that it could be implemented as a |
@Jasha10 you might be interested in this. |
@JulesGM are you familiar with the hydra-zen project, and specifically with https://mit-ll-responsible-ai.github.io/hydra-zen/generated/hydra_zen.instantiate.html? The |
Luv it. Thanks so much for pinging me on, @JulesGM. I have now learned many things. I have learned about the appropriately named OmegaConf, which I now realize I have been waiting for my entire life. I have also learned that @patrick-kidger of Interestingly, # Import both the annotation and the `jaxtyped` decorator from `jaxtyping`
from jaxtyping import Array, Float, jaxtyped
# Use your favourite typechecker: usually one of the two lines below.
from typeguard import typechecked as typechecker
from beartype import beartype as typechecker
# Type-check a function
@jaxtyped(typechecker=typechecker)
def batch_outer_product(x: Float[Array, "b c1"],
y: Float[Array, "b c2"]
) -> Float[Array, "b c1 c2"]:
return x[:, :, None] * y[:, None, :] So for so good. You're in good company, @JulesGM. But I've been wondering... can we eventually simplify and streamline the process of selecting competing runtime type-checkers or is literally every Python package ever going to now define its own non-orthogonal proprietary API for selecting competing runtime type-checkers? It's the latter, isn't it? I'm sighing. You can almost feel the hot fetid breath I'm exhaling all over your keyboard. 😮💨 If we accept the current status quo and do nothing,← what will happen, guaranteed then user headaches explode combinatorially. Currently, users have to manually notify every Python package of their preferred runtime type-checker with an API unique to that package. Instead, users should be able to trivially, publicly, and globally notify every Python package of their preferred runtime type-checker all-at-once simultaneously with just a single Python statement. Instead, we're now facing the exact opposite scenario. Introducing...
|
Heh this is crazy timing! I just added this feature in hydra-zen literally four days ago. It is extremely powerful; e.g. adding a pydantic parsing layer to a hydra(-zen) app (and adding beartype support would be straightforward as well!)
I think it should live in With this feature, it would be very nice to be able to completely disable Hydra/omegaconf's type-checking features and just let this validation layer be responsible for everything. |
Btw @JulesGM you should check out hydra-zen sometime 😄 It is really nice for ML workflows and eliminates a lot of manual labor and boilerplate from using Hydra |
Just to be clear, my suggestion would provide the additional flexibility of specifying the wrapper in the config but this wouldn't be mandatory: you could still provide it as a kwarg when calling |
I understand, I just think that adding this to the config abstraction gives a degree of complexity that leads users down design paths that they ought not take. You also get weird things when you have nested configs that each have a target wrapper...People will start asking for non-recursive wrappers, etc |
We are planning to use the ConfigStore approach to do schema and most of the type checking, but it feels very annoying (and increase the maintenance work as the parameters change) to have to write down dataclasses for things that have a We feel like having a basic version of target_wrapper in hydra-core would be a pretty good place to start with to cover that, without having to go all the way with the inclusion of hydra-zen. The debate of whether this can be included in the configs can maybe be done further down the road as usage evolves? |
fwiw hydra-zen's only installation dependencies are hydra-core and typing-extensions, so it is very lightweight. And all you would need to do is use I'm not opposed at all to having this be added to hydra-core, just want to point out that this is also a trivially easy path forward. I just need to officially release v0.13.0, which I can probably do this week. |
@JulesGM would that work for you? |
hydra-zen v0.13.0 was released yesterday: https://mit-ll-responsible-ai.github.io/hydra-zen/changes.html#v0-13-0 |
Motivation
beartype and other libraries offer runtime typechecking with function and class decorators, like so:
In this PR, we suggest adding an argument to
hydra.utils.instantiate
that is calledtarget_wrapper
, that receives a callable as an argument. The idea is that callable is then called on_target_
before it itself is called. This allows to decorate all of a configuration's_target_
s all at once. (It's possible that the argument should be calledtarget_decorator
)One of the ways this can be used is with something like
beartype
, where runtime type-checking to the portion of the code being instantiated with_target_
where annotations are present, with a single line of non-invasive code, which would be really incredibly helpful for us.It would look like this:
See the
test
example lower in the post for a more full example.One could imagine other types of wrappers that people could want to do, for profiling for example, where folks could wrap every class type with a profiler before the instantiation happens.
One could also add recursive support for
OmegaConf.structured
with the following, which is very cool:and then
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Here is a standalone test with
beartype
:Related Issues and PRs
(Is this PR part of a group of changes? Link the other relevant PRs and Issues here. Use https://help.github.com/en/articles/closing-issues-using-keywords for help on GitHub syntax)