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

Python bindings: Nanobind SID adapter #1762

Merged
merged 15 commits into from
Aug 9, 2023
Merged

Python bindings: Nanobind SID adapter #1762

merged 15 commits into from
Aug 9, 2023

Conversation

petiaccja
Copy link
Contributor

No description provided.

@petiaccja petiaccja requested a review from havogt July 27, 2023 15:32
@petiaccja
Copy link
Contributor Author

Depends on #1761

@petiaccja
Copy link
Contributor Author

launch jenkins

@petiaccja
Copy link
Contributor Author

CI failures:

  • HIP node has Python 3.6.8, 3.8+ required
  • tsa doesn't seem to have Python 3 development headers at all
  • cray c++20: std libraries don't seem to have std::lerp which was added in c++20

@havogt
Copy link
Contributor

havogt commented Jul 31, 2023

CI failures:

* HIP node has Python 3.6.8, 3.8+ required

I'll look into updating the Python env on ault

* `tsa` doesn't seem to have Python 3 development headers at all

The mechanism for enabling Python tests is currently here https://github.com/GridTools/gridtools/blob/master/tests/regression/py_bindings/CMakeLists.txt as it's the only place were Python is used, 2 options:

  • promote the Python enabling to the tests/CMakeLists.txt, or
  • write a test in regression/py_bindings for nanobind
  • (probably the clean solution is both: Python check at the top-level and an integration test in py_bindings)
* cray c++20: std libraries don't seem to have `std::lerp` which was added in c++20

I can try to play with the compiler versions and check if we find something that works.

@havogt
Copy link
Contributor

havogt commented Aug 2, 2023

Update on CI failures:

@petiaccja
Copy link
Contributor Author

launch jenkins

@petiaccja
Copy link
Contributor Author

launch jenkins

@petiaccja
Copy link
Contributor Author

launch jenkins

@petiaccja
Copy link
Contributor Author

launch jenkins

@petiaccja
Copy link
Contributor Author

launch jenkins

Copy link
Contributor

@havogt havogt left a comment

Choose a reason for hiding this comment

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

let's quickly talk

include/gridtools/storage/adapter/nanobind_adapter.hpp Outdated Show resolved Hide resolved
.github/workflows/cmake-configure.yml Outdated Show resolved Hide resolved
include/gridtools/storage/adapter/nanobind_adapter.hpp Outdated Show resolved Hide resolved
include/gridtools/storage/adapter/nanobind_adapter.hpp Outdated Show resolved Hide resolved
@petiaccja
Copy link
Contributor Author

launch jenkins

@petiaccja
Copy link
Contributor Author

launch jenkins

@petiaccja
Copy link
Contributor Author

launch jenkins

This reverts commit f767022.
@petiaccja
Copy link
Contributor Author

launch jenkins

Copy link
Contributor

@havogt havogt left a comment

Choose a reason for hiding this comment

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

I believe it's not needed to wrap each test in the check for GT_TESTS_ENABLE_PYTHON_TESTS as we only enable Python if was true. But there is an (unlikely?) case where it would still be needed if gridtools is added to another project with fetch_content/add_subdirectory and Python was found by this super project). So I guess we can keep it.

@havogt havogt changed the title Nanobind SID adapter Python bindings: Nanobind SID adapter Aug 9, 2023
@havogt havogt merged commit 4fb793a into GridTools:master Aug 9, 2023
50 checks passed
havogt pushed a commit that referenced this pull request Aug 16, 2023
Add a SID adapter for dlpack via nanobind.
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