Optional and non-optional dependencies#154
Merged
FlorianDeconinck merged 8 commits intoMay 30, 2025
Merged
Conversation
romanc
force-pushed
the
romanc/non-optional-imports
branch
5 times, most recently
from
3ec64a3 to
4d16ef2
Compare
romanc
changed the title
romanc
force-pushed
the
romanc/non-optional-imports
branch
3 times, most recently
from
381e314 to
5b877f7
Compare
7 tasks
romanc
force-pushed
the
romanc/non-optional-imports
branch
from
5b877f7 to
816d28b
Compare
Closed
fmalatino
previously approved these changes
May 28, 2025
FlorianDeconinck
requested changes
May 28, 2025
Collaborator
FlorianDeconinck
left a comment
FlorianDeconinck
left a comment
There was a problem hiding this comment.
Couple of extra import clean up since we are there
romanc
force-pushed
the
romanc/non-optional-imports
branch
3 times, most recently
from
9f06d39 to
3d50671
Compare
romanc
commented
May 30, 2025
Collaborator
Author
romanc
left a comment
romanc
left a comment
There was a problem hiding this comment.
Some inline comments on the latest changes. New changes are only in the last commit. The rest just has new commit hashes because I rebased.
| import shutil | ||
|
|
||
| import pytest | ||
| from gt4py.cartesian import config as gt_config |
Collaborator
Author
There was a problem hiding this comment.
Both tests (and there are only two) were requiring the same imports, so I moved them up here
Comment on lines
+107
to
+112
| @function | ||
| def dim(a, b): | ||
| """ | ||
| Performs a check on the difference | ||
| between the values in arguments | ||
| a and b. The variable diff is set | ||
| to the difference between a and b | ||
| when the difference is positive, | ||
| otherwise it is set to zero. The | ||
| function returns the diff variable. | ||
| Calculates a - b, camped to 0, i.e. max(a - b, 0). | ||
| """ | ||
| diff = a - b if a - b > 0 else 0 | ||
| return diff | ||
| return max(a - b, 0) |
Collaborator
Author
There was a problem hiding this comment.
@oelbert I did sneak in some cleanups here. FYI. Also, given the simplicity of the expression, I wonder if we should rename the function or if we need it at all...
fmalatino
previously approved these changes
May 30, 2025
FlorianDeconinck
requested changes
May 30, 2025
Collaborator
FlorianDeconinck
left a comment
FlorianDeconinck
left a comment
There was a problem hiding this comment.
Remove iskip and we should be good to go
romanc
force-pushed
the
romanc/non-optional-imports
branch
from
ff38735 to
acfad0a
Compare
Collaborator
Author
|
Removed |
oelbert
approved these changes
May 30, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
It looks like in earlier versions, a couple packages were optional. In particular this includes the following packages
dace,gt4py,mpi4py, andxarraywhich are now all installed by default given the current
setup.py. We can thus simplify their imports (in case they were still imported conditionally). In addition, a couple code paths didn't importcupyfromndsl.optional_imports. This has been corrected too.Doing all this, I discovered a bug in
ndsl/checkpointer/snapshots.py, where a snapshot's dataset would only ever contain the firstsavepoint. This was unearthed by tests intests/checkpointer/test_snapshot.py, which suddenly started failing after cleaning up the imports.As a follow-up, PR #156 will look into re-enabling tests with
zarrinstalled. Looks like these tests will need some love/work before we can re-enable them.How Has This Been Tested?
Covered by existing test cases.
Checklist: