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[next][dace]: cleanup dace backend module #1811

Merged
merged 265 commits into from
Jan 22, 2025

Conversation

edopao
Copy link
Contributor

@edopao edopao commented Jan 21, 2025

PR #1753 removed the dace_iterator backend, that before existed by side of dace_fieldview backend. Code common to these two backends was placed in dace_common module.

The current GTIR-DaCe backend supports both iterator view and field view flavors of the GTIR, so there is no need for a distinction at the top level of the backend module.

This PR applies the following cleanup tasks:

runners/dace_common/dace_backend -> runners/dace_fieldview/sdfg_callable
runners/dace_common/utility -> runners/dace_fieldview/utils
runners/dace_common/workflow, runners/dace_fieldview/workflow -> runners/dace_fieldview/workflow
runners/dace_fiedview/utility -> runners/dace_fieldview/gtir_sdfg_utils
runners/dace_fieldview -> runners/dace

The module runners/dace/workflow was also split into sub-modules. A doc string was added to runners/dace/workflow/__init__.py

edopao added 30 commits December 3, 2024 15:31
Copy link
Contributor

@philip-paul-mueller philip-paul-mueller left a comment

Choose a reason for hiding this comment

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

There are two things that must be taken care of:

  • I thing that debug_info() was removed, but it seems to be still needed.
  • There is now a duplicated file in the transformation sub-package.

However, I do not think that these two warrant an extra round.

Comment on lines -81 to -96
def debug_info(
node: gtir.Node, *, default: Optional[dace.dtypes.DebugInfo] = None
) -> Optional[dace.dtypes.DebugInfo]:
"""Include the GT4Py node location as debug information in the corresponding SDFG nodes."""
location = node.location
if location:
return dace.dtypes.DebugInfo(
start_line=location.line,
start_column=location.column if location.column else 0,
end_line=location.end_line if location.end_line else -1,
end_column=location.end_column if location.end_column else 0,
filename=location.filename,
)
return default


Copy link
Contributor

Choose a reason for hiding this comment

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

I can not find the new location of this function.
Also it is still referenced by other parts of the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I silently moved it to gtir_sdfg_utils so I could remove the dependency on gtir in the utils module.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay.
I just could not find it that was all.
But I noticed that for some reason Ctrl+F is a bit unreliable, but I do not understand why.
But I very much like your reason.
Sorry for the false alarm.

@@ -0,0 +1,227 @@
# GT4Py - GridTools Framework
Copy link
Contributor

Choose a reason for hiding this comment

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

The transformations subpackage has now tow files with the exact same content, once utils.py (which is new) and once util.py (which was there before).
I actually do not care which one survives and in fact I would say that utils.py should survive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. This must be an artifact of VSCode refactoring.

philip-paul-mueller and others added 3 commits January 22, 2025 09:16
This PR fixes a bug in `DistributedBufferRelocator` that was observed in
ICON4Py's `TestUpdateThetaAndExner` test.

In essence there was an `assert` that assumed that checked if this
temporary was a sink node, but, the code that finds all write backs was
never excluding such cases, i.e. the temporaries that were selected might
not be sink nodes in the state where they are defined.
The `assert` was not part of the original implementation and is not a
requirement of the transformation, instead it was introduced by
[PR#1799](GridTools#1799), that fixed some
issues in the analysis of read write dependencies.

There are two solutions for this, either removing the `assert` or prune
these kinds of temporaries. After some consideration, it was realized
that handling such cases will not lead to invalid SDFG, as long as the
other restrictions on the global data are respected. For that reason the
`assert` was removed.
However, we should thinking of doing something more intelligent in that
case.
@edopao edopao merged commit 89d3b0d into GridTools:main Jan 22, 2025
21 checks passed
@edopao edopao deleted the dace-gtir-refact-module branch January 22, 2025 09:00
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.

3 participants