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

Adds literal node #865

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

Conversation

jordan-schneider
Copy link

Adds literal node, solving #422

I tried to implement this as a wrapper around EnumNode, but this turned out to be impossible. Literal can accept int, str, bool and Enum values, but enum keys must be strings. So for example

from dataclasses import dataclass

@dataclass
class Config:
  mode: Literal["5", 5]

there is no way to map both of these values to separate enum keys without also causing a collision somewhere else.

This PR validates a LiteralNode value by insisting on both = equality and type equality. Both are needed, otherwise python's promiscuous primitive equalities cause problems e.g. 1.0 is valid under Literal[True].

I tried to add tests wherever EnumNode had tests, but I'm unfamiliar with the codebase. Please let me know if additional tests are needed.

@lgtm-com
Copy link

lgtm-com bot commented Feb 26, 2022

This pull request introduces 1 alert when merging 9338386 into fd06224 - view on LGTM.com

new alerts:

  • 1 for `__eq__` not overridden when adding attributes

@jordan-schneider
Copy link
Author

Okay I clearly missed a bunch of tests, working on that. Not sure what's up with mypy not thinking Literal exists.

@jordan-schneider
Copy link
Author

The literal thing was because setup.cfg sets the mypy version to 3.6 by default. I had a legitimate problem where I need to import from typing_extensions before 3.8, which I now do. I also added a dependency to the pre-commit hooks to fix a bunch of mypy errors locally, although this didn't appear to show up in the last round of circleci results.

@lgtm-com
Copy link

lgtm-com bot commented Feb 26, 2022

This pull request introduces 1 alert when merging 6d302ab into fd06224 - view on LGTM.com

new alerts:

  • 1 for `__eq__` not overridden when adding attributes

@jordan-schneider
Copy link
Author

Looks like I've ironed out all the python version issues.

@lgtm-com
Copy link

lgtm-com bot commented Feb 26, 2022

This pull request introduces 1 alert when merging 3c115d7 into fd06224 - view on LGTM.com

new alerts:

  • 1 for `__eq__` not overridden when adding attributes

@lgtm-com
Copy link

lgtm-com bot commented Feb 26, 2022

This pull request introduces 1 alert when merging 1592df4 into fd06224 - view on LGTM.com

new alerts:

  • 1 for `__eq__` not overridden when adding attributes

@Jasha10 Jasha10 linked an issue Mar 7, 2022 that may be closed by this pull request
@Jasha10
Copy link
Collaborator

Jasha10 commented Mar 10, 2022

Thanks very much @jordan-schneider.
Unfortunately it may be some time before we are able to give this PR a serious review as the Hydra/OmegaConf team has its hands full at the moment.

Copy link
Contributor

@tmke8 tmke8 left a comment

Choose a reason for hiding this comment

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

Hi, I'm the one who originally opened #422. So I'm very happy you are doing this!

I'm obviously not an official member of OmegaConf, but I thought I would try to help you with a code review anyway.

Some questions:

  1. What happens if I annotate a node with just Literal without any type arguments for Literal?
  2. Don't we have to add something like typing_extensions;python_version<'3.8' to requirements/base.txt? Or is typing_extensions a dependency anyway?

omegaconf/_utils.py Outdated Show resolved Hide resolved
omegaconf/_utils.py Outdated Show resolved Hide resolved
Comment on lines 609 to 611
or (
"typing_extensions.Literal" in str(type_) and not isinstance(type_, str)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, why is this needed? Isn't Literal always from typing_extensions in this else branch?

Copy link
Author

Choose a reason for hiding this comment

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

So what's happening here is that:

python 3.8 and later has typing.Literal which has an __origin__ attribute
python 3.7 typing_extensions.Literal has an __origin__ attribute
python 3.6 and earlier typing_extensions.Literal does not have an __origin__ attribute or any equivalent attribute

AND

A = Literal[5]
A is Literal  # False
isinstance(A, Literal)  # TypeError: isinstance() arg 2 must be a type or tuple of types
issubclass(type(A), Literal)  # TypeError: issubclass() arg 2 must be a class or tuple of classes

so as far as I can tell, for 3.6 and earlier the only way to check for Literalness is using the string. This is gross but I don't know of a better way.

This logic is unnecessarily complicated though. How do you feel about

def is_literal_annotation(type_: Any) -> bool:
    origin = getattr(type_, "__origin__", None)
    return origin is Literal or ("typing_extensions.Literal" in str(type_) and not isinstance(type_, str))

Copy link
Contributor

Choose a reason for hiding this comment

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

There is an is_literal_type() function here which also work with Python 3.6. The solution to the problem seems very stupid, you have to import a hidden class _Literal:

Python 3.6.13 |Anaconda, Inc.| (default, Jun  4 2021, 14:25:59)
[GCC 7.5.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from typing_extensions import Literal, _Literal
>>> a = Literal[0]
>>> type(a) is Literal
False
>>> type(a) is _Literal
True

I have no idea why.

Copy link
Author

Choose a reason for hiding this comment

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

Before 3.6 typing_extensions creates the internal _Literal class using a typing._FinalTypingBase mixin https://github.com/python/typing/blob/master/typing_extensions/src/typing_extensions.py#L344 , and then Literal is an instantiation of that class. So type(a) returns the outer _Literal class instead of the instantiation or something. Anyway this seems less bad than the string hack, so I'll go with this.

Comment on lines 419 to 422
if hasattr(self.literal_type, "__args__"):
self.fields = list(self.literal_type.__args__) # pragma: no cover
elif hasattr(self.literal_type, "__values__"): # pragma: no cover
self.fields = list(self.literal_type.__values__) # pragma: no cover
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe make these branches explicitly correspond to python versions. So I think

Suggested change
if hasattr(self.literal_type, "__args__"):
self.fields = list(self.literal_type.__args__) # pragma: no cover
elif hasattr(self.literal_type, "__values__"): # pragma: no cover
self.fields = list(self.literal_type.__values__) # pragma: no cover
if sys.version_info >= (3, 7):
self.fields = list(self.literal_type.__args__) # pragma: no cover
else:
self.fields = list(self.literal_type.__values__) # pragma: no cover

but I'm not really sure about this.

Copy link
Author

Choose a reason for hiding this comment

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

Why do you feel this is better? Philosophically I tend to think that directly checking is the right way to do things, in case e.g. the attribute name every switches back or something equally cursed. This also has the benefit that mypy/pylance/other syntax parsers can correctly infer that the attribute exists, which is left implicit if you're going by version.

If this is about readability, I'd be happy to add comments saying why the check is necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I was just thinking that at some point in the future, omegaconf might want to drop support for Python 3.6 and then it would be useful that you can directly see that this if-else check has become unnecessary.

But, yes, a comment is likely already sufficient.

Jordan Schneider and others added 2 commits March 13, 2022 14:52
@jordan-schneider
Copy link
Author

jordan-schneider commented Mar 13, 2022

Hi, I'm the one who originally opened #422. So I'm very happy you are doing this!

I'm obviously not an official member of OmegaConf, but I thought I would try to help you with a code review anyway.

Some questions:

1. What happens if I annotate a node with just `Literal` without any type arguments for `Literal`?

2. Don't we have to add something like `typing_extensions;python_version<'3.8'` to `requirements/base.txt`? Or is `typing_extensions` a dependency anyway?
  1. I don't have time at the moment to directly check this but Literal.__values__/__args__ still exists but is empty, so the LiteralNode is constructed and whatever the value is it fails to validate. This seems like the correct behavior. I'll write a test for this when I have a moment.

  2. I think it's a dependency of one of the other dependencies but it should be added explicitly anyway, in case that changes. Again I'll handle this when I have a moment.

@lgtm-com
Copy link

lgtm-com bot commented Mar 14, 2022

This pull request introduces 1 alert when merging a0e2b35 into 1d622e7 - view on LGTM.com

new alerts:

  • 1 for `__eq__` not overridden when adding attributes

@jordan-schneider
Copy link
Author

All of the CI lints are failing due to

nox > [2022-03-14 15:46:08,598] mypy . --strict --install-types --non-interactive
nox > [2022-03-14 15:46:23,545] Command mypy . --strict --install-types --non-interactive failed with exit code 1:
Collecting types-dataclasses
  Downloading types_dataclasses-0.6.4-py3-none-any.whl (2.7 kB)
Collecting types-setuptools
  Downloading types_setuptools-57.4.10-py3-none-any.whl (27 kB)
Installing collected packages: types-setuptools, types-dataclasses
Successfully installed types-dataclasses-0.6.4 types-setuptools-57.4.10
tests/test_update.py:202: error: Unused "type: ignore" comment

Which I can't replicate locally. If I remove that type: ignore comment locally I do in fact get an error. Anyone know what's going on here?

@Jasha10
Copy link
Collaborator

Jasha10 commented Mar 14, 2022

If I remove that type: ignore comment locally I do in fact get an error. Anyone know what's going on here?

@jordan-schneider maybe upgrade the version of mypy that you have installed locally?

@Jasha10
Copy link
Collaborator

Jasha10 commented Mar 24, 2022

If I remove that type: ignore comment locally I do in fact get an error. Anyone know what's going on here?

The CI should pass if you rebase this branch against the master branch to incorporate changes from #878.

@lgtm-com
Copy link

lgtm-com bot commented Mar 26, 2022

This pull request introduces 1 alert when merging d321a4a into c0197e5 - view on LGTM.com

new alerts:

  • 1 for `__eq__` not overridden when adding attributes

@lgtm-com
Copy link

lgtm-com bot commented Mar 26, 2022

This pull request introduces 1 alert when merging b69fd38 into c0197e5 - view on LGTM.com

new alerts:

  • 1 for `__eq__` not overridden when adding attributes

@jordan-schneider
Copy link
Author

I think it was mostly outdated local versions of tools. The pre-commit version of black and the current version used by CircleCI also disagreed, so I updated the pre-commit version as well.

@codekansas
Copy link

Are there plans to merge this? It would be super useful!

@Jasha10
Copy link
Collaborator

Jasha10 commented Sep 22, 2022

Thanks for the nudge, @codekansas!

Are there plans to merge this?

Yes. We haven't been able to finalize this PR yet as the OmegaConf team's bandwidth is limited. It is on the roadmap.

@indigoviolet
Copy link

@Jasha10 nudge again to enquire about plans/status since some time has passed

@Queuecumber
Copy link

@omry @Jasha10 would appreciate some eyes on this, it's been in limbo for a year and a half and would be a nice way to produce clean structured configs

@daturkel
Copy link

+1 this feature would be great and feels more pythonic than Enums.

@spfrommer
Copy link

+1

1 similar comment
@gsq7474741
Copy link

+1

@jbohnslav
Copy link

+1, still relevant

@slambert-git
Copy link

+1

1 similar comment
@jkulhanek
Copy link

+1

@dkmiller
Copy link

Another gentle nudge, this would be very nice to have!

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.

Allow the use of typing.Literal as a pythonic alternative to Enums