Skip to content
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

Deprecation of register_resolver() to introduce new functionalities #426

Closed
odelalleau opened this issue Oct 28, 2020 · 7 comments
Closed
Assignees

Comments

@odelalleau
Copy link
Collaborator

odelalleau commented Oct 28, 2020

Migrating custom resolvers from OmegaConf 2.0 to 2.1

Summary

  1. Replace register_resolver with register_new_resolver
  2. Update resolvers to handle non-string arguments (if applicable)
  3. Only for resolvers whose output must be cached: ensure use_cache=True is passed to register_new_resolver

Detailed migration instructions

Here are the main changes between resolvers registered with register_new_resolver() vs the old register_resolver():

  • They may take as input data of types supported by the new grammar (e.g. int, float, bool, dict, list) and possibly computed from other interpolations, e.g.: ${func: [3.14, ${oc.env:VAR}], true}
  • Their cache is disabled by default (and can be enabled by passing use_cache=True to register_new_resolver())
  • They may access the parent and the root config nodes (by declaring _parent_ and _root_ to the callable respectively) (Support optional _parent_ and _root_ parameters in custom resolvers #599).

In most cases, migrating an existing resolver from OmegaConf 2.0 to 2.1 means replacing register_resolver() with register_new_resolver() and ensuring the resolver is correctly handling non-string inputs, if applicable.

As an example, imagine you had the following resolver in OmegaConf 2.0:

def add_ints(*args: str) -> int:
    return sum(int(x) for x in args)  # in 2.0 all arguments are strings and must be manually converted

OmegaConf.register_resolver("add_ints", add_ints)
cfg = OmegaConf.create("sum: ${add_ints:1,2,3}")
assert cfg.sum == 6

In this example arguments will now expected to be integers in OmegaConf 2.1:

def add_ints(*args: int) -> int:
    assert all(isinstance(x, int) for x in args)  # if we want to prevent adding non integer values
    return sum(args)

OmegaConf.register_new_resolver("add_ints", add_ints)
cfg = OmegaConf.create("sum: ${add_ints:1,2,3}")
assert cfg.sum == 6

Note that here we didn't register the resolver with use_cache=True: it is expected that most resolvers should not need to use the cache. If you are depending on the caching behavior by assuming that your resolver is always returning the same value even when the underlying function may return a different value (e.g. random(), time()), then register your resolver with use_cache=True.

The new resolver behavior is documented in detail in the documentation.

Additional remarks:

  • In 2.1 you can silence the deprecation warning by calling legacy_register_resolver() (without updating your resolver), but this is only a temporary workaround that will be deprecated in future versions.
  • There are a few more small differences in the behavior of register_new_resolver() (compared to register_resolver() in 2.0), see Improve the API of register_resolver() #370 for details

Deprecation plan for register_resolver()

The following deprecation plan is being applied to register_resolver() so as to enable new functionalities introduced in OmegaConf 2.1 (#445):

  1. In 2.1 register_resolver() is deprecated, and users calling this function will see a warning redirecting them to this issue. The old behavior of register_resolver() remains temporarily available through legacy_register_resolver(). Users are strongly encouraged to switch to the new function register_new_resolver() (see above for an example).

  2. In 2.2 register_new_resolver() will be renamed into register_resolver(), and users calling register_new_resolver() will be warned to call register_resolver() instead. Users calling the old legacy_register_resolver() will also be warned to switch to register_resolver(), updating their resolver accordingly.

  3. In 2.3 register_new_resolver() and legacy_register_resolver() will be removed

Update (2022-06-21)

The deprecation plan outlined above wasn't followed exactly for the OmegaConf 2.2 release. The schedule above will need to be modified.

@omry

This comment has been minimized.

@thoth291

This comment has been minimized.

@omry

This comment has been minimized.

@raphCode
Copy link

I am confused because I called register_resolver() with omegaconf version 2.2.2 and got the deprecation warning.
According to the deprecation plan outlined in the first post this is the behavior of omegaconf 2.1 .

If I understand the plan correctly, using register_resolver() in the current version is the future-proof method of registering resolvers and should not issue a warning.
Am I missing something or can I safely ignore the warning?

@odelalleau
Copy link
Collaborator Author

odelalleau commented Jun 21, 2022

I am confused because I called register_resolver() with omegaconf version 2.2.2 and got the deprecation warning. According to the deprecation plan outlined in the first post this is the behavior of omegaconf 2.1 .

If I understand the plan correctly, using register_resolver() in the current version is the future-proof method of registering resolvers and should not issue a warning. Am I missing something or can I safely ignore the warning?

Don't ignore the warning (and use register_new_resolver() instead), there's a good chance the deprecation plan outlined above wasn't followed exactly in 2.2.x and this issue should be updated to match what actually happened.

@Jasha10 thoughts?

@raphCode
Copy link

raphCode commented Jun 21, 2022

I am developing new code so I am flexible.
Ideally I want write code that I don't have to update anymore with future omegaconf versions.
It is not clear to me how I can achieve that: The plan states that both register_new_resolver() and legacy_register_resolver() will be eventually removed in a future version, which leaves only register_resolver() for a long-term solution?

In other words, skipping from version 2.0 directly to a future version where the register_resolver() has the new behavior would not surface any warnings but just silently use the updated function.

@Jasha10
Copy link
Collaborator

Jasha10 commented Jun 21, 2022

there's a good chance the deprecation plan outlined above wasn't followed exactly in 2.2.x

Yes, this is what happened.

In OmegaConf 2.2, register_new_resolver() is still the preferred method to use, and register_resolver still gives a deprecation warning.

I've updated the issue description above and will create a follow-up issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants