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

update from antlr-4.8 to antlr-4.9 #2192

Merged
merged 3 commits into from
May 7, 2022
Merged

Conversation

pixelb
Copy link
Contributor

@pixelb pixelb commented May 5, 2022

Use the latest 4.9 release (4.9.3)

Addresses issue #2155

Needs to follow omegaconf/pull/912 (master branch)

Use the latest 4.9 release (4.9.3)

Addresses issue facebookresearch#2155
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 5, 2022
Copy link
Contributor Author

@pixelb pixelb left a comment

Choose a reason for hiding this comment

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

@Jasha10 I don't know how valid my change to test_instantiate is TBH.
Note without that, omegaconf is throwing from a clause adjusted in omry/omegaconf#911
however I can't see how that specific change is at issue here, as it only seems to be a naming change

@Jasha10
Copy link
Collaborator

Jasha10 commented May 6, 2022

It looks like the CI failure happened when OmegaConf's AnyNode class refused to wrap a SimpleDataClass:

.nox/test_core-3-10/lib/python3.10/site-packages/omegaconf/nodes.py:139: in _validate_and_convert_impl
    raise UnsupportedValueType(
E   omegaconf.errors.UnsupportedValueType: Value 'DictConfig' is not a supported primitive type
E       full_key: a
E       object_type=SimpleDataClass

I don't think setting allow_objects=True is the best approach here. Setting allow_objects=True makes AnyNode more flexible (allowing any object type instead of just primitive object types)... but there is still the question of why SimpleDataClass is being wrapped in AnyNode instead of being treated as a structured config.

@Jasha10
Copy link
Collaborator

Jasha10 commented May 6, 2022

I'm confused because the PR omry/omegaconf#911 was supposed to be a pure refactoring without any behavior change.

@pixelb
Copy link
Contributor Author

pixelb commented May 6, 2022

I'm confused because the PR omry/omegaconf#911 was supposed to be a pure refactoring without any behavior change.

Yes this PR is just coincidental it seems. Reverting just that change, we're still left with the issue

@Jasha10
Copy link
Collaborator

Jasha10 commented May 6, 2022

It seems that the CI failed not just for the OmegaConf dev release -- it failed for the OmegaConf 2.1 install as well.
This makes me think that the issue is on the Hydra side...

Nevermind, I missed that you had bumped the pin on OmegaConf.

@pixelb
Copy link
Contributor Author

pixelb commented May 6, 2022

It seems that the CI failed not just for the OmegaConf dev release -- it failed for the OmegaConf 2.1 install as well. This makes me think that the issue is on the Hydra side... Nevermind, I missed that you had bumped the pin on OmegaConf.

Right. I just now locally changed the pin to 2.2.0.dev2 (from April 13), and the following is fine:

$ pip install -r requirements/dev.txt 
$ pytest tests/instantiate/test_instantiate.py

So that narrows it down a lot

@Jasha10
Copy link
Collaborator

Jasha10 commented May 6, 2022

I just did a git bisect to find that PR omry/omegaconf#909 is the culprit.

@Jasha10
Copy link
Collaborator

Jasha10 commented May 6, 2022

I'll send in a PR to revert omry/omegaconf#909.

pixelb added 2 commits May 6, 2022 21:20
This version of omegaconf now has a consistent requirement on antlr-4.9
Adjust as per the renamed interfaces in:
omry/omegaconf#911
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants