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.

Sign up for GitHub

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

Jump to bottom

ClassVar parameters cannot include any type variables #565

Closed
jdahm opened this issue Nov 29, 2021 · 2 comments · Fixed by #979
Closed

ClassVar parameters cannot include any type variables #565

jdahm opened this issue Nov 29, 2021 · 2 comments · Fixed by #979
Projects
GridTools

Comments

@jdahm
Copy link
Contributor

jdahm commented Nov 29, 2021 •
edited

PEP-526 states that

Note that a ClassVar parameter cannot include any type variables, regardless of the level of nesting: ClassVar[T] and ClassVar[List[Set[T]]] are both invalid if T is a type variable.

Unfortunately the new contexts feature of the NodeVisitors use this implicitly because eve.concepts.TreeNode = Union[AnyNode, CollectionNode] and

AnyNode = TypeVar("AnyNode", bound="BaseNode")

Need to find a way around this or reimplement the feature.

cc: @egparedes
@gridtoolsjenkins gridtoolsjenkins added this to Triage in GridTools Nov 29, 2021
@egparedes
Copy link
Contributor

I was not really aware of the PEP-526 restriction on ClassVars and it looks like most of the static type checker tools weren't either until very recently ( microsoft/pyright#2567, python/mypy#11538, python/typeshed#6333 ).

I think the solution for this specific case is relatively straightforward: change the definition of TreeNode = Union[AnyNode, CollectionNode] to TreeNode = Union[BaseNode, CollectionNode]

@jdahm
Copy link
Contributor Author

jdahm commented Dec 1, 2021

Yeah, I caught it by putting gt4py through pyright the other day.

I think the solution for this specific case is relatively straightforward: change the definition of TreeNode = Union[AnyNode, CollectionNode] to TreeNode = Union[BaseNode, CollectionNode]

Sounds good.

@jdahm jdahm mentioned this issue Dec 4, 2021
Closed
@jdahm jdahm mentioned this issue Apr 22, 2022
Closed
@jdahm jdahm mentioned this issue Oct 24, 2022
Merged
jdahm added a commit that referenced this issue Oct 26, 2022
@jdahm
Remove src/__init__.py and fix many errors (#979)
adec0c3 
Major changes:

- Adds type checking on a few more files.
- Expr.dtype's type is no longer Optional[DataType], because that caused type havoc. Therefore, the default is changed from None to DataType.AUTO when not set explicitly. The gtir_dtype_resolver and other visitors are updated to check for DataType.AUTO instead of None.
- Moves most of the code that was in eve.iterators to eve.concepts to work around a circular dependency after solving ClassVar parameters cannot include any type variables #565 using @egparedes's suggestion. This is a temporary workaround until we replace Eve with the new version.
- gt4py.stencil_object.ArgsInfo is made non-private and used in gt4py.dace_stencil_object to fix a type issue.
- Ignores typing in eve visitor contexts -- need to fix that in a follow-up PR.
- npir.HorizontalMask is created as a tuple of tuples, to avoid the Optional[AxisBound] in the common.HorizontalInterval type.
- Replaces gt_backend references with gt4py.backend.

Follows #978.

Resolves #565.
Resolves #992.
Resolves #1007.
GridTools automation moved this from Triage to Done Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
GridTools
  
Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove src/__init__.py and fix many errors
Replace eve.concepts.AnyNode by eve.concepts.BaseNode
2 participants
@egparedes @jdahm