Added clear_resolver method to OmegaConf #770

sugatoray · 2021-07-19T23:06:51Z

This closes #769 .

Added OmegaConf.clear_resolver() method to omegaconf.py. 🟢
- This allows the user to remove a single resolver.
Added tests to tests/test_omegaconf.py::test_clear_resolver. 🟢
Added documentation for OmegaConf.clear_resolvers and OmegaConf.clear_resolver. 🟢

- added DefaultResolverWriteProtectionError to errors.py

- rolled back custom error in error.py - added test_clear_resolver in test_omegaconf.py - minor change in CONTRIBUTING.md - small chanes in omegaconf.py

- Added test case for ValueError: test_omegaconf.py::test_clear_resolver - imorted DEFAULT_RESOLVER_NAMES in omegaconf/__init__.py

sugatoray · 2021-07-20T05:47:27Z

Hi @omry,

I hope this functionality will give people the ability to selectively remove certain custom resolvers, after adding them. Please let me know your thoughts on it. Thank you.

Jasha10 · 2021-07-21T00:21:57Z

Thanks for submitting this @sugatoray :)
I'll take a look at it shortly.

sugatoray · 2021-07-21T14:06:08Z

@Jasha10 If you see my current implementation, you will find that the constant list, DEFAULT_RESOLVER_NAMES is being manually created. This means in future, if you were to add/remove a default resolver (like removing deprecated resolver env), you will have to manually update this list.

❓ What if I move it inside the method that defines the default resolvers and dynamically create DEFAULT_RESOLVER_NAMES as:

DEFAULT_RESOLVER_NAMES = list(BaseContainer._resolvers.keys())

This will ensure that this list is always populated with the default resolver names and no manual maintenance of it will be necessary either.

Jasha10 · 2021-07-21T19:38:29Z

question What if I move it inside the method that defines the default resolvers...

Yes, that could work. That being said, please see Omry's suggestion in the linked issue; we might be able to skip the DEFAULT_RESOLVER_NAMES variable all together.

- updated test: test_omegaconf::test_clear_resolver - updated omegaconf/__init__.py - updated clear_resolver method: returns a bool now.

sugatoray · 2021-07-25T19:55:03Z

@Jasha10 I just pushed the changes to the PR. Please review it and let me know if any changes should be made. Thank you.

Jasha10

Thanks @sugatoray. A few comments:

tests/test_omegaconf.py

- slim down test-cases - make use of fixture "restore_resolvers" - remove redundant parameter: expected["post_removal"] - add comments: notes for others on the use of fixture, "restore_resolvers" - use pytest.param instead of tuple for parametric testing

tests/test_omegaconf.py

- simplified input test parameters

- Also, updated test_omegaconf.py::test_clear_resolver - OmegaConf.clear_resolver now returns - False if resolver not found. - True if resolver [found + removed].

sugatoray · 2021-07-31T07:35:14Z

@Jasha10 I also added a section for clearing resolvers in the docs. Please take a look at it and let me know if you think changes should be made.

Note: I am not sure if OmegaConf.clear_resolvers() has any documentation available or not. So, I added it here as any user trying to add a custom resolver, would probably find it handy here to know how to clear/remove resolver(s).

For quick review, I am attaching a screenshot of the generated docs. I added the following section in docs/source/custom_resolvers.rst. This screenshot corresponds to this commit: 76179cd.

- changes made in docs/source/custom_resolvers.rst

Jasha10

Note: _I am not sure if OmegaConf.clear_resolvers() has any documentation available or not.

You're right, there was no documentation before.

So, I added it here as any user trying to add a custom resolver, would probably find it handy here to know how to clear/remove resolver(s)._

Great, thanks!

A few more comments below on the tests:

omegaconf/omegaconf.py

tests/test_omegaconf.py

- removed `pragma: nocover` (was unnecessary)

- removed comments of using pytest fixture: restore_resolvers - removed key: expected["pre_addition"] - added key: expected["result"] - swaped logic dependent test with hardcoded scenarios

sugatoray · 2021-08-03T03:28:24Z

@Jasha10 Thank you, for the detailed review. I have made the changes you suggested. Instead of adding expected_result as another parameter, I added that as a key to the dict-parameter, expected:

expected["result"]

Jasha10

Looking good! Just a few more comments.

tests/test_omegaconf.py

docs/source/custom_resolvers.rst

Co-authored-by: Jasha10 <8935917+Jasha10@users.noreply.github.com>

- minor update to docs/source/custom_resolvers.rst - some changes to test_omegaconf.py::test_clear_resolver

sugatoray · 2021-08-04T04:45:20Z

@Jasha10 I just pushed the changes to the PR. Thank you, for the reviews. Please let me know if there is anything else that needs change.

omry

Thanks for working on it. Overall looks good. I have a few comments.

docs/source/custom_resolvers.rst

omry · 2021-08-04T04:56:32Z

docs/source/custom_resolvers.rst

+.. code-block:: python
+
+    def clear_resolvers() -> None
+
+In the following example, first we register a new custom resolver ``str.lower``, and then clear all
+custom resolvers.
+
+.. doctest::
+
+    >>> # register a new resolver: str.lower
+    >>> OmegaConf.register_new_resolver(
+    ...     name='str.lower',
+    ...     resolver=lambda x: str(x).lower(),
+    ... )
+    >>> # check if resolver exists (after adding, before removal)
+    >>> OmegaConf.has_resolver("str.lower")
+    True
+    >>> # clear all custom-resolvers
+    >>> OmegaConf.clear_resolvers()
+    >>> # check if resolver exists (after removal)
+    >>> OmegaConf.has_resolver("str.lower")
+    False
+    >>> # default resolvers are not affected
+    >>> OmegaConf.has_resolver("oc.env")
+    True


The contract of this function is very simple, I am not convinced that it needs an example at all.
I vote to remove the example.

Granted it's down right simple 😉 . But, I believe, examples are kind of a major useful feature of the documentation of this library. So, I thought if it is made clear through an example, it will be helpful. I wanted to show that custom resolvers are removed, while default ones are not, when clear_resolvers() is used.

However, if you insist on it's (the example: starting from line-412) removal, that's okay as well. 😄

docs/source/custom_resolvers.rst

omegaconf/omegaconf.py

- update docs for clear_resolvers() Co-authored-by: Omry Yadan <omry@fb.com>

- update docs for clear_resolver() Co-authored-by: Omry Yadan <omry@fb.com>

- simplified code for clear_resolver() Co-authored-by: Omry Yadan <omry@fb.com>

- updated docs for clear_resolver() Co-authored-by: Omry Yadan <omry@fb.com>

- simplify code in clear_resolver() Co-authored-by: Omry Yadan <omry@fb.com>

sugatoray · 2021-08-04T22:44:06Z

@omry Thank you, for your reviews. I have made the changes you suggested (except the docs example on clear_resolvers). I hope, now it is very close to ready-to-merge.

omry

lgtm, thanks!
@Jasha10, feel free to merge.

sugatoray · 2021-08-05T20:48:19Z

@omry and @Jasha10 It was a very pleasant experience working with you, on my first PR to this library. I hope to contribute more in future.

💡 Question: is there any CHANGELOG that you would like me to update?

Jasha10 · 2021-08-05T21:50:55Z

Thanks @sugatoray, it was good working with you!

Yes, good idea about the changelog (I forgot about that step).
I've created a follow-up PR #774 which adds a new file news/769.feature.

FYI: OmegaConf uses a tool called towncrier to generate its NEWS.md changelog. This news file gets updated during OmegaConf's release cycle; the contents of this news/769.feature will be added to NEWS.md automatically. The name of the news file news/769.feature refers to the original issue #769 that you created.

sugatoray added 3 commits July 19, 2021 17:53

added clear_resolver method to OmegaConf

7ea9ae3

- added DefaultResolverWriteProtectionError to errors.py

added tests and simplified some code

46c1021

- rolled back custom error in error.py - added test_clear_resolver in test_omegaconf.py - minor change in CONTRIBUTING.md - small chanes in omegaconf.py

Added test scenario for Error

9ed558d

- Added test case for ValueError: test_omegaconf.py::test_clear_resolver - imorted DEFAULT_RESOLVER_NAMES in omegaconf/__init__.py

updated OmegaConf.clear_resolver method

9812ce9

- updated test: test_omegaconf::test_clear_resolver - updated omegaconf/__init__.py - updated clear_resolver method: returns a bool now.

Jasha10 self-requested a review July 27, 2021 03:22

Jasha10 requested changes Jul 27, 2021

View reviewed changes

Jasha10 reviewed Jul 28, 2021

View reviewed changes

tests/test_omegaconf.py Outdated Show resolved Hide resolved

sugatoray added 3 commits July 29, 2021 19:02

updated test_omegaconf.py::test_clear_resolver

ce7a05c

- simplified input test parameters

Updated logic for OmegaConf.clear_resolver

f62b567

- Also, updated test_omegaconf.py::test_clear_resolver - OmegaConf.clear_resolver now returns - False if resolver not found. - True if resolver [found + removed].

Added section for clearing resolver(s) in docs

64ca79d

sugatoray and others added 2 commits July 31, 2021 02:42

updated docs examples for clearing resolver(s)

76179cd

- changes made in docs/source/custom_resolvers.rst

Merge branch 'omry:master' into feature/769_clear_single_resolver

5ec405a

sugatoray requested a review from Jasha10 August 1, 2021 01:16

Jasha10 requested changes Aug 2, 2021

View reviewed changes

sugatoray added 3 commits August 2, 2021 22:02

cleaning up inside OmegaConf.clear_resolver

5074cf4

- removed `pragma: nocover` (was unnecessary)

cleaned up comments from test_omegaconf.py::test_clear_resolver

cc974b6

made changes to test_omegaconf.py::test_clear_resolver

324d0e1

- removed comments of using pytest fixture: restore_resolvers - removed key: expected["pre_addition"] - added key: expected["result"] - swaped logic dependent test with hardcoded scenarios

Jasha10 requested changes Aug 3, 2021

View reviewed changes

tests/test_omegaconf.py Outdated Show resolved Hide resolved

docs/source/custom_resolvers.rst Show resolved Hide resolved

sugatoray and others added 3 commits August 3, 2021 23:23

Update docs/source/custom_resolvers.rst

8da4cbc

Co-authored-by: Jasha10 <8935917+Jasha10@users.noreply.github.com>

updated docs for OmegaConf.clear_resolvers

2cb44f8

changes to tests and docs

350011b

- minor update to docs/source/custom_resolvers.rst - some changes to test_omegaconf.py::test_clear_resolver

omry reviewed Aug 4, 2021

View reviewed changes

sugatoray and others added 5 commits August 4, 2021 17:10

Update docs/source/custom_resolvers.rst

8e51670

- update docs for clear_resolvers() Co-authored-by: Omry Yadan <omry@fb.com>

Update docs/source/custom_resolvers.rst

1b687a4

- update docs for clear_resolver() Co-authored-by: Omry Yadan <omry@fb.com>

Update omegaconf/omegaconf.py

8d6c609

- simplified code for clear_resolver() Co-authored-by: Omry Yadan <omry@fb.com>

Update docs/source/custom_resolvers.rst

1d209ca

- updated docs for clear_resolver() Co-authored-by: Omry Yadan <omry@fb.com>

Update omegaconf/omegaconf.py

a42bc44

- simplify code in clear_resolver() Co-authored-by: Omry Yadan <omry@fb.com>

Jasha10 approved these changes Aug 5, 2021

View reviewed changes

omry approved these changes Aug 5, 2021

View reviewed changes

Jasha10 merged commit c861d00 into omry:master Aug 5, 2021

Jasha10 mentioned this pull request Aug 5, 2021

news fragment for clear_resolver #774

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added clear_resolver method to OmegaConf #770

Added clear_resolver method to OmegaConf #770

sugatoray commented Jul 19, 2021 •

edited

Loading

sugatoray commented Jul 20, 2021 •

edited

Loading

Jasha10 commented Jul 21, 2021

sugatoray commented Jul 21, 2021 •

edited

Loading

Jasha10 commented Jul 21, 2021

sugatoray commented Jul 25, 2021

Jasha10 left a comment •

edited

Loading

sugatoray commented Jul 31, 2021 •

edited

Loading

Jasha10 left a comment

sugatoray commented Aug 3, 2021 •

edited

Loading

Jasha10 left a comment

sugatoray commented Aug 4, 2021 •

edited

Loading

omry left a comment

omry Aug 4, 2021

sugatoray Aug 4, 2021 •

edited

Loading

sugatoray commented Aug 4, 2021

omry left a comment

sugatoray commented Aug 5, 2021

Jasha10 commented Aug 5, 2021

Added clear_resolver method to OmegaConf #770

Added clear_resolver method to OmegaConf #770

Conversation

sugatoray commented Jul 19, 2021 • edited Loading

sugatoray commented Jul 20, 2021 • edited Loading

Jasha10 commented Jul 21, 2021

sugatoray commented Jul 21, 2021 • edited Loading

Jasha10 commented Jul 21, 2021

sugatoray commented Jul 25, 2021

Jasha10 left a comment • edited Loading

Choose a reason for hiding this comment

sugatoray commented Jul 31, 2021 • edited Loading

Jasha10 left a comment

Choose a reason for hiding this comment

sugatoray commented Aug 3, 2021 • edited Loading

Jasha10 left a comment

Choose a reason for hiding this comment

sugatoray commented Aug 4, 2021 • edited Loading

omry left a comment

Choose a reason for hiding this comment

omry Aug 4, 2021

Choose a reason for hiding this comment

sugatoray Aug 4, 2021 • edited Loading

Choose a reason for hiding this comment

sugatoray commented Aug 4, 2021

omry left a comment

Choose a reason for hiding this comment

sugatoray commented Aug 5, 2021

Jasha10 commented Aug 5, 2021

sugatoray commented Jul 19, 2021 •

edited

Loading

sugatoray commented Jul 20, 2021 •

edited

Loading

sugatoray commented Jul 21, 2021 •

edited

Loading

Jasha10 left a comment •

edited

Loading

sugatoray commented Jul 31, 2021 •

edited

Loading

sugatoray commented Aug 3, 2021 •

edited

Loading

sugatoray commented Aug 4, 2021 •

edited

Loading

sugatoray Aug 4, 2021 •

edited

Loading