Add a method for removing/unregistering/clearing a single registered resolver

[sugatoray]

The Problem

At present, you can only remove/clear/unregister all the user-defined registered resolvers using OmegaConf.clear_resolvers(). But there is no method dedicated to removing a single user-defined registered resolver.

Describe the solution you'd like
Add a method OmegaConf.clear_resolver(name) with the following API footprint:

def clear_resolver(cls, name: str, ignore_default_resolver: bool=True) -> None:
    # Logic
    #------------------------------------------------------
    # check if:
    # - name is exists in existing resolvers 
    # - name is not among default-resolver-names
    #   - if name is among default-resolvers, 
    #      - skip raising error if ignore_default_resolver== True
    #      - raise Error if ignore_default_resolver== False.
    # if all of the above are true, clear the resolver.

This would also require a way to track the default resolver names. I propose to add a variable DEFAULT_RESOLVER_NAMES in omegaconf.py as follows:

# Dynamically evaluate default resolver names
DEFAULT_RESOLVER_NAMES = list(BaseContainer._resolvers.keys())

# Alternatively this can be hard-coded as:
#     In this case, perhaps it's better to keep this 
#     maintained within: `resolvers/__init__.py`
DEFAULT_RESOLVER_NAMES  = [
    "oc.create", "oc.decode", "oc.deprecated", "oc.env", 
    "oc.select", "oc.dict.keys", "oc.dict.values", "env"
]

💡 Question to maintainers: Do you think it will make sense to add this DEFAULT_RESOLVER_NAMES as an attribute to OmegaConf?

Describe alternatives you've considered
I have already implemented the code and will send a PR. So, please assign it to me. Thank you.

[omry]

Hi, why do you need ignore_default_resolver (and the corresponding list of intenal resolvers?)

[sugatoray]

@omry I kept ignore_default_resolver to let the user intentionally skip throwing error, if need be. Say, you have a list of resolvers and you are removing them dynamically, if this list happens to contain a default resolver's name, then the logic would skip processing it and also not throw error (given, ignore_default_resolver=True).

The functionality is like certain methods in pandas where you can pass an ignore=True parameter to avoid some sort of output.

This is just an idea and I would like reviewers' opinion on whether that will make sense or not.

At the time of proposing this, I did not see any method that yields a list of default resolvers or checks if a resolver is default or custom.

💡 In hind sight, I think perhaps it's better to remove the parameter ignore_default_resolver and leave the responsibility of checking whether a given resolver is a default resolver or not with the user. This keeps scope of clear_resolver clean (single responsibility). But, in that case, I would like to add another method OmegaConf.is_default_resolver(name) to let the user determine if a given resolver name is default or not.

# code for OmegaConf.is_default_resolver
@classmethod
def is_default_resolver(cls, name: str) -> bool:
    """Check if a resolver is default or not."""
    return name in DEFAULT_RESOLVER_NAMES

What do you think?

[omry]

I think we should keep minimal and not do things because other libraries are doing them.

A simple API for clear resolver can be:

def clear_resolver(name: str) -> bool:
  ...

This will remove a resolver and return False if it's not found, allowing the user to handle this case.
It will allow a user to remove all resolvers, including built in ones. I think this is okay.

[sugatoray]

It will allow a user to remove all resolvers, including built in ones. I think this is okay.

@omry I like your suggestion. But, since this lets the user remove any resolver, shouldn't there be a method to check if a given resolver is default or not?

What do you think about adding a separate method to check if a resolver is default or not? Please let me know your thoughts.

@classmethod
def is_default_resolver(cls, name: str) -> bool:
    """Check if a resolver is default or not."""
    return name in BaseContainer._resolvers.keys()

In this case, the user could do something like this:

# If the user wants to ensure removing only non-default 
# resolvers, the following snippet should suffice.
resolver_name = "os.env"
if not OmegaConf.is_default_resolver(resolver_name):
    OmegaConf.clear_resolver(resolver_name)

# If the user wants to remove resolver without any check
OmegaConf.clear_resolver(resolver_name)

A question

OmegaConf.register_new_resolver() or OmegaConf.clear_resolvers() does not return anything. Then, shouldn't OmegaConf.clear_resolver() also show similar behavior?

[Jasha10]

But, since this lets the user remove any resolver, shouldn't there be a method to check if a given resolver is default or not?

Let's add OmegaConf.clear_resolver first.
Implementing OmegaConf.is_default_resolver would require adding some logic and global state to keep track of which resolvers are default. This would make OmegaConf more complex and hard to maintain. I think that adding is_default_resolver is currently not worth it.

For now:

• users will be able to call OmegaConf.register_new_resolver(..., replace=False). With replace=False, you can guard against accidentally overwriting a default (or previously-registered) resolver.
• If you want to know which resolvers are the defaults, you can look at list(BaseContainer._resolvers) before registering any new resolvers. Alternatively, you can keep track of the resolvers that you registered yourself, and make sure that you only call OmegaConf.clear_resolver on the resolvers you've registered.

A question

OmegaConf.register_new_resolver() or OmegaConf.clear_resolvers() does not return anything. Then, shouldn't OmegaConf.clear_resolver() also show similar behavior?

Note that OmegaConf.has_resolver returns a bool :)

Edit: Exposing a public API like OmegaConf.list_resolvers or OmegaConf.get_resolvers (equivalent to list(BaseContainer._resolvers.keys())) would probably be easier than maintaining a list of which resolvers are default and which ones are not.

[omry]

I don't think we should really care about internal resolvers.
If someone wants to remove them, it's their problem/responsibility. It's not like it will happen accidentally.

[sugatoray]

@omry and @Jasha10 Thank you for responding to my concerns. Well, now that it's all sorted, I will go ahead and remove unnecessary code (from the existing PR), implement OmegaConf.clear_resolver as @omry suggested and update the tests.