OmegaConf.to_yaml #313

pereman2 · 2020-07-20T16:03:09Z

Closes #263 .

Documentation
Jupyter notebook change

Pretty() instance method is changed to OmegaConf.to_yaml() static method.

omry · 2020-07-20T16:20:14Z

omegaconf/base.py

-    @abstractmethod
-    def pretty(self, resolve: bool = False, sort_keys: bool = False) -> str:
-        ...


You have just pissed off every single use of OmegaConf :)

deprecate it, have it issue a warning and call OmegaConf.to_yaml().
See other examples in the codebase. (look for "warn").

Oh shit true i'll undo that change asap

No rush :), this is not landed and released yet.

omegaconf/omegaconf.py

tests/examples/test_dataclass_example.py

pereman2 · 2020-07-20T18:56:59Z

Gotta leave, I'll add documentation and change and change the jupyter notebook.

omry · 2020-07-20T22:01:57Z

omegaconf/basecontainer.py

+            pretty() is deprecated, use OmegaConf.to_yaml() and resolve
+            now defaults to True (Since 2.0.1)


Suggested change

pretty() is deprecated, use OmegaConf.to_yaml() and resolve

now defaults to True (Since 2.0.1)

pretty() is deprecated and will be removed in a future version.

Use OmegaConf.to_yaml(). Please note that the default value for resolve has changed to True.

omry · 2020-07-20T22:04:59Z

omegaconf/basecontainer.py

+        """
+        returns a yaml dump of this config object.
+        :param resolve: if True, will return a string with the interpolations resolved, otherwise
+        interpolations are preserved
+        :param sort_keys: If True, will print dict keys in sorted order. default False.
+        :return: A string containing the yaml representation.
+        """


Suggested change

"""

returns a yaml dump of this config object.

:param resolve: if True, will return a string with the interpolations resolved, otherwise

interpolations are preserved

:param sort_keys: If True, will print dict keys in sorted order. default False.

:return: A string containing the yaml representation.

"""

omegaconf/base.py

omry · 2020-07-20T22:06:19Z

omegaconf/basecontainer.py

+        container = OmegaConf.to_container(self, resolve=resolve, enum_to_str=True)
+        return yaml.dump(  # type: ignore
+            container,
+            default_flow_style=False,
+            allow_unicode=True,
+            sort_keys=sort_keys,
+            Dumper=get_omega_conf_dumper(),
+        )


delegate to OmegaConf.to_yaml()

tests/test_basic_ops_dict.py

tests/test_basic_ops_list.py

omry · 2020-07-20T22:12:43Z

tests/test_nodes.py

+def test_pretty_deprecated() -> None:
+    c = OmegaConf.create({"foo": "bar"})
+    with pytest.warns(
+        expected_warning=UserWarning,
+        match=re.escape(
+            """
+            pretty() is deprecated, use OmegaConf.to_yaml() and resolve
+            now defaults to True (Since 2.0.1)
+            """
+        ),
+    ):
+        assert c.pretty() == "foo: bar\n"
+
+


not sure this is the vest place for it. why test_nodes?

Since the abstract method was in Container I thought it would be better but now looking at it it doesn't make that much sense. I'll move everything to test_omegaconf as you said since it's where to_yaml is.

omry · 2020-07-21T20:29:44Z

docs/notebook/Tutorial.ipynb

+    "Note below that when printed the interpolations are resolved.<br/>\n",
    "They get resolved once you access them."


Interpolations are evaluated lazily on field access. OmegaConf.to_yaml() is eagerly resolving the interpolations by default. You can change it by passing resolve=False.

Example without resolve=False.
Example with resolve=False

The other related doc update here is also similarly confusing.

omry

You should only include your own changes in this PR.

omry · 2020-07-22T00:35:40Z

docs/notebook/Tutorial.ipynb

@@ -521,15 +521,14 @@
    "\n",
    "OmegaConf support variable interpolation, Interpolations are evaluated lazily on access.\n",
    "\n",
-    "### Config node interpolation"
+    "## Config node interpolation"


Please provide a screen shot, I am not good at reading json data.

omry · 2020-07-22T00:36:26Z

docs/source/usage.rst

+You can add additional interpolation types using custom resolvers, which take strings as inputs.
+This example creates a resolver that adds 10 to the given value (note the need to cast it to `int`).


Is this not already committed in master? why is this here again?

omry · 2020-07-23T04:20:21Z

omegaconf/basecontainer.py

+            resolve has changed to True.
+            """,
+            category=UserWarning,
+            stacklevel=2,


why stackLevel 2?

I didn't give that much thought to it, I saw that select and update_node had the same parameters and all of them are in basecontainer so I did the same.

Don't pass stackLevel at all unless you do give it some thought.

tests/examples/test_dataclass_example.py

omry · 2020-07-23T04:23:19Z

tests/test_basic_ops_list.py

-        ),
-    ],
-)
-def test_iterate_list(input_: Any, expected: Any, list_key: str) -> None:


You are moving unrelated tests.
always review your own diffs before asking for someone else to review them.

omry · 2020-07-23T04:24:59Z

docs/notebook/Tutorial.ipynb

@@ -726,7 +733,7 @@
   ],
   "source": [
    "conf = OmegaConf.load('../source/env_interpolation.yaml')\n",
-    "print(conf.pretty(resolve=True))"


This example was making a point of showing the effect of resolve.
you removed resolve, but now it's not making any point because resolve is True by default.

Changed in the last commit, now the next cell shows wihout resolving.

omry · 2020-07-23T04:29:19Z

tests/test_omegaconf.py

+def test_to_yaml_string_boolean_list() -> None:
+    for t in _utils.YAML_BOOL_TYPES:
+        print(t)
+        c = OmegaConf.create([t, 1])
+        expected = "- '%s'\n- 1\n" % t
+        assert OmegaConf.to_yaml(c) == expected
+
+
+def test_to_yaml_string_int_list() -> None:
+    c = OmegaConf.create(["1", 1])
+    expected = "- '1'\n- 1\n"
+    assert OmegaConf.to_yaml(c) == expected


I didn't realize we now have a zillion tests for pretrty.

create a new dedicated test file for it : test_to_yaml.py

use parameterize to reduce the number of tests. for example the two tests here are almost identical and can be easily pameterized. (do this in two diffs so it's easier to review. first diff moves all the to_yaml tests to one file and the second parameterize.

I see a print here. remove it and other prints you may have added in tests.

omry · 2020-07-23T18:47:35Z

omegaconf/basecontainer.py

+            resolve has changed to True.
+            """,
+            category=UserWarning,
+            stacklevel=2,


Don't pass stackLevel at all unless you do give it some thought.

omry · 2020-07-23T18:51:23Z

tests/test_basic_ops_dict.py

+def test_instantiate_config_fails() -> None:
+    with pytest.raises(TypeError):
+        BaseContainer()  # type: ignore


This is not a real change, you are just moving it around in the file.
these kind of changes are making it really hard to review the diff.
Please fix your diff. at that point it might be easier to revert the changes to the logs and move just the correct functions to the new file.

omry

almost there

omry · 2020-07-23T22:50:49Z

tests/test_nodes.py

+def test_pretty_deprecated() -> None:
+    c = OmegaConf.create({"foo": "bar"})
+    with pytest.warns(
+        expected_warning=UserWarning,
+        match=re.escape(
+            """
+            pretty() is deprecated and will be removed in a future version.
+            Use OmegaConf.to_yaml. Please note that the default value for
+            resolve has changed to True.
+            """,
+        ),
+    ):
+        assert c.pretty() == "foo: bar\n"


this test makes no sense in test_nodes.
move to test_to_yaml.

omry · 2020-07-24T15:31:39Z

How will users know about this important change?

pereman2 · 2020-07-24T15:40:04Z

Update NEWS.md ?

omry · 2020-07-24T15:42:21Z

you never update NEWS directly, but close: create a news fragment.

omry · 2020-07-25T20:18:12Z

having second thoughts about changing the default for resolve to True:
if your config has interpolations that can't be resolved I think it will blow up.

pereman2 · 2020-07-25T21:42:41Z

Ok, if you want I can turn it back.
By interpolation that can't be resolved you mean a human error? if that's the case maybe it should raise and exception. Otherwise what type of interpolation cannot be resolved(if you have a snippet cool, if not i'll investigate it)?
I'm going to bed now, I'll look into it tomorrow. 😃

omry · 2020-07-25T23:09:02Z

so remember that this is used primarily for debugging, you would not be happy to get an exception from a str() method due to an error in your data, right?

pereman2 · 2020-07-26T12:51:56Z

True.

omry · 2020-07-26T15:20:28Z

Okay then, let's change the default of resolve back to False. remember to update the docs and news fragment.

omry requested changes Jul 20, 2020

View reviewed changes

omry reviewed Jul 21, 2020

View reviewed changes

pereman2 added 3 commits July 22, 2020 00:05

removed pretty, added OmegaConf.to_yaml

cf19e88

undo remove pretty, deprecated warning

1ef7c04

force to_yaml params and deprectation warning update

edb048f

omry requested changes Jul 22, 2020

View reviewed changes

pereman2 force-pushed the pretty branch from 57e8079 to 29574cc Compare July 22, 2020 17:48

omry requested changes Jul 23, 2020

View reviewed changes

pereman2 added 3 commits July 23, 2020 22:59

docs

a5e2dfc

deprecation warning pretty

1d03c19

tests

fc4ebe5

pereman2 force-pushed the pretty branch from 4d8a166 to fc4ebe5 Compare July 23, 2020 21:23

omry requested changes Jul 23, 2020

View reviewed changes

move deprecated test

c70ca0e

pereman2 and others added 2 commits July 24, 2020 18:04

removal news

46cd23b

Update 263.removal

650ce9a

omry merged commit 4d1d96a into omry:master Jul 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OmegaConf.to_yaml #313

OmegaConf.to_yaml #313

pereman2 commented Jul 20, 2020 •

edited

Loading

omry Jul 20, 2020

pereman2 Jul 20, 2020

omry Jul 20, 2020

pereman2 commented Jul 20, 2020

omry Jul 20, 2020

omry Jul 20, 2020

omry Jul 20, 2020

omry Jul 20, 2020

pereman2 Jul 21, 2020

omry Jul 21, 2020

omry Jul 21, 2020

omry left a comment

omry Jul 22, 2020

pereman2 Jul 22, 2020

omry Jul 22, 2020

omry Jul 23, 2020

pereman2 Jul 23, 2020

omry Jul 23, 2020

omry Jul 23, 2020

omry Jul 23, 2020

pereman2 Jul 23, 2020

omry Jul 23, 2020

omry Jul 23, 2020

omry Jul 23, 2020

omry left a comment

omry Jul 23, 2020

omry commented Jul 24, 2020

pereman2 commented Jul 24, 2020

omry commented Jul 24, 2020

omry commented Jul 25, 2020

pereman2 commented Jul 25, 2020

omry commented Jul 25, 2020

pereman2 commented Jul 26, 2020

omry commented Jul 26, 2020

		pretty() is deprecated, use OmegaConf.to_yaml() and resolve
		now defaults to True (Since 2.0.1)

		"Note below that when printed the interpolations are resolved.<br/>\n",
		"They get resolved once you access them."

		You can add additional interpolation types using custom resolvers, which take strings as inputs.
		This example creates a resolver that adds 10 to the given value (note the need to cast it to `int`).

OmegaConf.to_yaml #313

OmegaConf.to_yaml #313

Conversation

pereman2 commented Jul 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pereman2 commented Jul 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omry commented Jul 24, 2020

pereman2 commented Jul 24, 2020

omry commented Jul 24, 2020

omry commented Jul 25, 2020

pereman2 commented Jul 25, 2020

omry commented Jul 25, 2020

pereman2 commented Jul 26, 2020

omry commented Jul 26, 2020

pereman2 commented Jul 20, 2020 •

edited

Loading