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

Remove src/__init__.py and fix many errors #979

Merged
merged 23 commits into from
Oct 26, 2022
Merged

Remove src/__init__.py and fix many errors #979

merged 23 commits into from
Oct 26, 2022

Conversation

jdahm
Copy link
Contributor

@jdahm jdahm commented Oct 15, 2022

Description

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.

@gronerl
Copy link
Contributor

gronerl commented Oct 17, 2022

Looks like there's a circular import problem now, how should we proceed with this?

@jdahm
Copy link
Contributor Author

jdahm commented Oct 17, 2022

Looks like there's a circular import problem now

I committed the code and pushed after nearly a whole day of working out these issues, only to find out about it 😆 .

how should we proceed with this?

The circular dependency should be solved, or rather avoided, by moving to the next version of eve. But in this case a bit of analysis and moving around code definitions within this version of eve could solve it.

src/gt4py/stencil_object.py Outdated Show resolved Hide resolved
src/gt4py/stencil_builder.py Outdated Show resolved Hide resolved
@jdahm
Copy link
Contributor Author

jdahm commented Oct 24, 2022

bors try

bors bot added a commit that referenced this pull request Oct 24, 2022
@bors
Copy link

bors bot commented Oct 25, 2022

try

Build failed:

src/gt4py/stencil_object.py Outdated Show resolved Hide resolved
@jdahm
Copy link
Contributor Author

jdahm commented Oct 25, 2022

bors try

bors bot added a commit that referenced this pull request Oct 25, 2022
@bors
Copy link

bors bot commented Oct 25, 2022

try

Build failed:

@jdahm
Copy link
Contributor Author

jdahm commented Oct 25, 2022

bors try

bors bot added a commit that referenced this pull request Oct 25, 2022
@bors
Copy link

bors bot commented Oct 26, 2022

try

Timed out.

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.

It's impressive the amount of work you've done in this PR to fix all typings! It looks good to me, just a couple of very minor questions and suggestions, but I don't see anything blocking the merge.

src/gt4py/cli.py Outdated Show resolved Hide resolved
src/gt4py/stencil_object.py Show resolved Hide resolved
src/gtc/dace/expansion/daceir_builder.py Show resolved Hide resolved
src/gtc/passes/oir_access_kinds.py Show resolved Hide resolved
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.

It looks good to go.

@jdahm
Copy link
Contributor Author

jdahm commented Oct 26, 2022

bors try

bors bot added a commit that referenced this pull request Oct 26, 2022
@bors
Copy link

bors bot commented Oct 26, 2022

try

Build succeeded:

@jdahm jdahm merged commit adec0c3 into GridTools:master Oct 26, 2022
@jdahm jdahm deleted the remove-init branch October 26, 2022 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants