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

fix[next]:Python>=3.11 #1388

Closed
wants to merge 2 commits into from
Closed

Conversation

kotsaloscv
Copy link
Contributor

Description

Fix for Python>=3.11

@kotsaloscv kotsaloscv requested a review from egparedes December 8, 2023 13:27
@kotsaloscv kotsaloscv self-assigned this Dec 8, 2023
Copy link
Contributor

@egparedes egparedes left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR! I think we still need more work to fully understand the cause of the error and find a cleaner the solution, but it's a good first try.

Comment on lines +529 to +536
if len(getattr(cls, "__parameters__", [])) == 0:
cls.__parameters__ = []
annotations = getattr(cls, "__annotations__", {})
type_ = annotations.get("type")
if annotations and type_:
for t in get_args(type_):
if isinstance(t, _typing.TypeVar):
cls.__parameters__.append(t)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really understand these lines, but they are adding a new __parameters__ member to any class passed as parameter, which is effectively monkey-patching the class for no reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @egparedes ,
FYI, I checked the python 3.10 and the __parameters__ member is missing as well. So it seems to be a recurring issue not related to 3.11 only. It does not crash because it never enters this part of the code. I tried to understand where they diverge (3.10 vs 3.11) but I could not find it.
Essentially what the above code does is to check if this field exists and if not to properly construct it. Another place that would make sense to put it, would be this part of the code: https://github.com/kotsaloscv/gt4py/blob/py311_fix/src/gt4py/eve/datamodels/core.py#L1071
What do you think?

@kotsaloscv
Copy link
Contributor Author

FYI,
cc @egparedes
We have used this branch with Will Sawyer to install icon4py and we got the following error:

_external_src/gt4py/src/gt4py/eve/type_validation.py:362: in _is_instance_of
    raise TypeError(
E   TypeError: 'Program.params' must be <class 'gt4py.next.ffront.program_ast.Symbol__datatypet__datatypet'> (got 'Symbol__datatypet(location=SourceLocation(filename='/Users/kotsaloc/repos/will/icon4py/model/atmosphere/diffusion/src/icon4py/model/atmosphere/diffusion/stencils/apply_nabla2_and_nabla4_global_to_vn.py', line=41, column=5, end_line=41, end_column=41), id=SymbolName('area_edge'), namespace=<Namespace.LOCAL: 'local'>, type=FieldType(dims=[Dimension(value='Edge', kind=<DimensionKind.HORIZONTAL: 'horizontal'>)], dtype=ScalarType(kind=<ScalarKind.FLOAT64: 1064>, shape=None)))' which is a <class 'gt4py.next.ffront.program_ast.Symbol__datatypet'>).

While the tests here are passing (even with this hack for Python 3.11), icon4py fails.
I am not sure if this is related to my fix/work-around or another issue with Python 3.11.

@kotsaloscv kotsaloscv closed this Jan 12, 2024
@egparedes
Copy link
Contributor

Closed in favor of #1407

@kotsaloscv kotsaloscv deleted the py311_fix branch January 24, 2024 07:22
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.

2 participants