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

Deprecate subclassing dict #707

Merged
merged 14 commits into from
May 11, 2021
Merged

Conversation

Jasha10
Copy link
Collaborator

@Jasha10 Jasha10 commented May 6, 2021

Closes #663

Copy link
Owner

@omry omry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good.
Can you run this on Hydra to see if it would fail any tests?

docs/source/structured_config.rst Outdated Show resolved Hide resolved
tests/structured_conf/test_structured_config.py Outdated Show resolved Hide resolved
Comment on lines 240 to 244
warnings.warn(
"Subclassing of `Dict` by Structured Config classes is deprecated",
UserWarning,
stacklevel=2,
stacklevel=1,
)
Copy link
Collaborator Author

@Jasha10 Jasha10 May 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With stacklevel=2 we get a warning that looks like this:

$ python tmp.py
/home/jbss/omegaconf.git/omegaconf/_utils.py:373: UserWarning: Subclassing of `Dict` by Structured Config classes is deprecated
  dict_subclass_data = extract_dict_subclass_data(obj=obj, parent=dummy_parent)

With stacklevel=1 we get a warning that looks like this:

$ python tmp.py
/home/jbss/omegaconf.git/omegaconf/_utils.py:240: UserWarning: Subclassing of `Dict` by Structured Config classes is deprecated
  warnings.warn(

I changed 2 -> 1 because I thought the dict_subclass_data = extract_dict_subclass_data(...) was confusing...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exact interesting stack frame is probably not in a fixed depth.
Try to get it to issue the line calling OmegaConf.create() in the simple case.

Also, add the name of the type subclassing Dict to the warning.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 6eeb616 and 71ad2b6.

@Jasha10
Copy link
Collaborator Author

Jasha10 commented May 10, 2021

Can you run this on Hydra to see if it would fail any tests?

pytest is passing on Hydra.
Not too sure about nox -- seems like there are lots of linter errors when running against OmegaConf master branch.
I'm having trouble getting hydra installed with the latest version of OmegaConf. I can install OmegaConf using pip install -e ~/omegaconf.git (and then hydra pytest passes), but the nox tests aren't passing (because I haven't been able to set up hydra/setup.py to install from my local version of OmegaConf).

It seems that there are no conflicts 👍

omegaconf/_utils.py Outdated Show resolved Hide resolved
tests/test_errors.py Show resolved Hide resolved
@Jasha10 Jasha10 requested a review from omry May 10, 2021 23:50
Copy link
Owner

@omry omry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits.
Also, add a news fragment.

def test_dict_subclass_error() -> None:
src = Str2Int()
src["bar"] = "qux" # type: ignore
# expected.finalize(cfg)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 192c9db.

Comment on lines 1438 to 1439
def test_dict_subclass_error() -> None:
src = Str2Int()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment explaining what this function is testing and why it's a separate function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in b58bcd5.

@Jasha10 Jasha10 requested a review from omry May 11, 2021 22:50
Copy link
Owner

@omry omry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shipit

@Jasha10 Jasha10 merged commit 1349008 into omry:master May 11, 2021
@Jasha10 Jasha10 deleted the deprecate_subclassing_dict branch May 11, 2021 23:38
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

Successfully merging this pull request may close these issues.

Deprecate subclassing Dict
2 participants