Skip to content

Conversation

@astrogewgaw
Copy link
Contributor

@astrogewgaw astrogewgaw commented Apr 12, 2025

Just as the title says, this PR shifts riptide to a src layout. To put it a bit more verbosely, the PR makes the following changes:

  1. Shifts to a src-based layout.
  2. Shifts tests outside the package.
  3. Formats all Python source code with black.

These changes involve absolutely no changes to riptide's functionality, since the source code has been left untouched (expect for being formatted).

@v-morello
Copy link
Owner

@astrogewgaw Actually, I'd rather merge the cosmetic/linting changes separately, so when the time comes to make significant changes the PR is easy to review. Otherwise, It'll be drowning in thousands of lines worth of diff.

Secondly, I am not particularly keen to move to nanobind, because pybind11 ain't broke and doesn't need fixing. Shorter compilation times are nice, but they are, let's face it, quite short already. Binary size is a non-issue, and runtime overheads of calling C code are non-existent in riptide. pybind11 also comes with nice utilities to trigger the build in setup.py (The Pybind11Extension class), while the only time I tried nanobind I had to struggle with CMake. I am concerned that the setup machinery will just become more complex for no visible gain.

I'm happy to merge the 3 changes you mentioned:

  1. Shifting to a src-based layout.
  2. Shifting tests outside the package.
  3. Formatting with all Python source code with black.

But for nanobind, you'll have to convince me 😄

@astrogewgaw
Copy link
Contributor Author

astrogewgaw commented Apr 12, 2025

@v-morello No worries, let us do it this way then: I will edit this PR, and make it just for shifting riptide to a src layout. You can then go ahead and review it, and merge it if nothing is amiss. I will then make another PR, which will implement the C++ bindings in nanobind rather than pybind11. The main thing that I like about nanobind is the support for arrays, which is much better than in pybind11, and I feel it will make a lot of the code easier to reason with (since obviously a lot of it deals with arrays). However, I think the PR itself might be better a way: it will either convince you to adopt the idea, or convince me to drop it 😁 . What do you think?

PS: To address the point about the setup machinery, nanobind does use CMake, but I don't think it makes the setup very complicated, since you never have to call CMake yourself. It would be called automatically by scikit-build-core when the package is built. But again, seeing is believing, and a PR might be more convincing I think.

@astrogewgaw astrogewgaw changed the title Shift from pybind11 to nanobind. Shift to a src layout. Apr 12, 2025
@astrogewgaw astrogewgaw marked this pull request as ready for review April 12, 2025 21:23
@v-morello
Copy link
Owner

Good plan with the PR split 👍

It would be called automatically by scikit-build-core

The problem with scikit-build-core is that it is very immature and there's no telling if it will still be there in 5 years. I know that pybind11 will still be around in 5 years, because a lot of major packages now depend on it (tensforflow is one of them).

a PR might be more convincing I think

I'm OK to look at the diff and decide there, but then no promises, and I'm still leaning towards no. Please don't spend too much time on it, because I don't want to waste it.

@astrogewgaw
Copy link
Contributor Author

@v-morello No worries, I think I will work on this quite slowly anyways. Let us take it up when I am done with that PR. As for this one, let me know if there any changes required, and we can go ahead accordingly.

@codecov
Copy link

codecov bot commented Apr 15, 2025

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@astrogewgaw astrogewgaw requested a review from v-morello April 18, 2025 13:17
Copy link
Owner

@v-morello v-morello left a comment

Choose a reason for hiding this comment

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

All good

@v-morello v-morello merged commit 50aaf4b into v-morello:master Apr 19, 2025
6 checks passed
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