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]: Warn if GT4Py can't find DaCe #1692

Merged
merged 2 commits into from
Oct 18, 2024

Conversation

romanc
Copy link
Contributor

@romanc romanc commented Oct 17, 2024

Description

DaCe backends are optional backends in the cartesian version of GT4Py. Currently, we silently drop support for DaCe backends if DaCe can't be imported. This can can happen because DaCe isn't available or for a couple other reasons (e.g. a circular import in the import path). With this PR we thus add a warning message allowing developers to easily figure out that/why DaCe backends are disabled.

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.

DaCe adds optional backends to GT4Py. Currently, we silently drop
support for DaCe backends. This PR adds a warning message allowing
developers to easily figure out DaCe backends aren't available.
@romanc
Copy link
Contributor Author

romanc commented Oct 17, 2024

@egparedes would you have time for a review?

/cc @FlorianDeconinck as discussed

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 the PR. It looks much better now.

@romanc
Copy link
Contributor Author

romanc commented Oct 17, 2024

Thanks for the PR. It looks much better now.

Thanks for the quick review. Could you also kick-off the CSCS CI job? Thanks! 🙏

@egparedes
Copy link
Contributor

cscs-ci run

@romanc
Copy link
Contributor Author

romanc commented Oct 18, 2024

Thanks for running CSCS CI, Enrique.

Regarding the branch update: the branch is already outdated again. You might find merge queues an interesting option to avoid such situations. Merge queues provide the same level of security as requiring an up-to-date branch without authors (or maintainers) having to update branches manually.

@egparedes egparedes merged commit cb77ccb into GridTools:main Oct 18, 2024
31 checks passed
@egparedes
Copy link
Contributor

Regarding the branch update: the branch is already outdated again. You might find merge queues an interesting option to avoid such situations. Merge queues provide the same level of security as requiring an up-to-date branch without authors (or maintainers) having to update branches manually.

Thanks for the suggestion. We briefly considered to use merge queues in the past but given that the number of PRs merged per day is not really high, it's probably not worth it.

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