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

refactor[cartesian]: Enable MaskInlining in gt4yp - DaCe bridge #1624

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

romanc
Copy link
Contributor

@romanc romanc commented Sep 2, 2024

Description

MaskInlining will be a key feature of the improved gt4py / DaCe bridge as envisioned in GEOS-ESM/NDSL#53, which will propagate more control flow elements from gt4py to DaCe. We thus want to make sure if MaskInlining itself is posing any problems. We tracked the git history of this explicit exclude back to PR #300, which not only added the npir backend, but also refactored how pipeline stages are defined. Previous to this PR optimization passes to be specified explicitly and the PR changed this to an exclude list. It looks like the MaskInlining pass was added after the list of optimization passes was formed and thus excluded (to keep the list constant). From this history we thus don't expect MaskInlining to break anything.

Requirements

  • All fixes and/or new features come with corresponding tests.
  • Important design decisions have been documented in the approriate ADR inside the docs/development/ADRs/ folder.

If this PR contains code authored by new contributors please make sure:

  • The PR contains an updated version of the AUTHORS.md file adding the names of all the new contributors.

@romanc romanc marked this pull request as ready for review September 2, 2024 07:33
@romanc
Copy link
Contributor Author

romanc commented Sep 2, 2024

@havogt I have a question about linting in gt4py. I ran pre-commit as suggested in the contribution guidelines. This ran fine. After that I was running tox (again as suggested) and this failed with flake8 in the linting stage in files that I didn't touch

image

Is that expected behavior?

@havogt
Copy link
Contributor

havogt commented Sep 2, 2024

We should not be using flake8 anymore. @egparedes there seem to be a few leftovers.

AUTHORS.md Outdated Show resolved Hide resolved
@romanc
Copy link
Contributor Author

romanc commented Sep 2, 2024

We should not be using flake8 anymore. @egparedes there seem to be a few leftovers.

I see, I can make a second PR to remove flake8 (in favor of ruff): see #1625.

@romanc romanc changed the title refactor [cartesian]: Enable MaskInlining in gt4yp - DaCe bridge refactor[cartesian]: Enable MaskInlining in gt4yp - DaCe bridge Sep 3, 2024
@romanc romanc force-pushed the romanc/dace-mask-inlining branch from 6c7a266 to 5f5e0d7 Compare September 3, 2024 15:55
@romanc
Copy link
Contributor Author

romanc commented Sep 3, 2024

With #1625 merged this PR rebased to main, we should be good for review now. Note that the AUTHORS.md file was changed in PR #1625 so nothing to do anymore in this PR regarding attribution.

/cc @FlorianDeconinck adding MaskInlining (to avoid confusion as to why the pass was skipped) as discucssed.

@romanc romanc requested a review from havogt September 3, 2024 16:00
Copy link
Contributor

@FlorianDeconinck FlorianDeconinck left a comment

Choose a reason for hiding this comment

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

LGTM!

Yay for clean up \o/

@romanc
Copy link
Contributor Author

romanc commented Sep 3, 2024

cscs-ci run default

@romanc
Copy link
Contributor Author

romanc commented Sep 3, 2024

cscs-ci run default

Looks like I don't have permission to kick off the cscs/default CI. Can someone with proper rights do this for me? Thanks!

@egparedes
Copy link
Contributor

cscs-ci run default

@romanc
Copy link
Contributor Author

romanc commented Sep 4, 2024

@egparedes thanks for running cscs tests. Please go ahead and merge (as I don't have write access to this repository).

@egparedes egparedes merged commit 07f37fb into GridTools:main Sep 4, 2024
31 checks passed
@romanc romanc deleted the romanc/dace-mask-inlining branch September 4, 2024 17:16
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.

4 participants