Skip to content

chore: add nanobind to build system#85

Open
SYMSCAE wants to merge 8 commits into
diffpy:migration-nanofrom
SYMSCAE:nanobind-deps
Open

chore: add nanobind to build system#85
SYMSCAE wants to merge 8 commits into
diffpy:migration-nanofrom
SYMSCAE:nanobind-deps

Conversation

@SYMSCAE

@SYMSCAE SYMSCAE commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

This PR updates cpp language version to c++23, replaces boost smart pointer usages with stl implementations and adds nanobind to the current build system.

@SYMSCAE

SYMSCAE commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

@sbillinge PR ready for review. Since nanobind doesn't provide a conda library we have to build it our own, the build_ext_with_nanobind function builds nanobind with its recommanded arg (this is a bit ugly but seems to be the only way until we introduce cmake).

@sbillinge

Copy link
Copy Markdown
Contributor

@SYMSCAE on a separate PR please could you do the upgrade of the pre-commit packages so that everything is at least passing pre-commit?

For the failing tests I don't really have a clear idea abot which ones we expect to be falling (didn't migrate those things) and which are concerning (something broken). Maybe it is best of you can add a few words in the comments about these issues and what you want me to do, for example, to merge an intermediate step, or not merge because we want to get all tests passing on the branch, for example..

@SYMSCAE

SYMSCAE commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

@SYMSCAE on a separate PR please could you do the upgrade of the pre-commit packages so that everything is at least passing pre-commit?

For the failing tests I don't really have a clear idea abot which ones we expect to be falling (didn't migrate those things) and which are concerning (something broken). Maybe it is best of you can add a few words in the comments about these issues and what you want me to do, for example, to merge an intermediate step, or not merge because we want to get all tests passing on the branch, for example..

Sure I'll make another pr to upgrade pre-commit to main and see what that happens. The tests are failing because ci is fetching libdiffpy from conda so it will continue to fail until we make a new conda release. It can be fixed if we change the workflow to fetch and build libdiffpy from source but I guess that's not preferred?

@sbillinge

Copy link
Copy Markdown
Contributor

@SYMSCAE on a separate PR please could you do the upgrade of the pre-commit packages so that everything is at least passing pre-commit?
For the failing tests I don't really have a clear idea abot which ones we expect to be falling (didn't migrate those things) and which are concerning (something broken). Maybe it is best of you can add a few words in the comments about these issues and what you want me to do, for example, to merge an intermediate step, or not merge because we want to get all tests passing on the branch, for example..

Sure I'll make another pr to upgrade pre-commit to main and see what that happens. The tests are failing because ci is fetching libdiffpy from conda so it will continue to fail until we make a new conda release. It can be fixed if we change the workflow to fetch and build libdiffpy from source but I guess that's not preferred?

I see. Is libdiffpy ready to release? Is it passing tests?

@SYMSCAE

SYMSCAE commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

@SYMSCAE on a separate PR please could you do the upgrade of the pre-commit packages so that everything is at least passing pre-commit?
For the failing tests I don't really have a clear idea abot which ones we expect to be falling (didn't migrate those things) and which are concerning (something broken). Maybe it is best of you can add a few words in the comments about these issues and what you want me to do, for example, to merge an intermediate step, or not merge because we want to get all tests passing on the branch, for example..

Sure I'll make another pr to upgrade pre-commit to main and see what that happens. The tests are failing because ci is fetching libdiffpy from conda so it will continue to fail until we make a new conda release. It can be fixed if we change the workflow to fetch and build libdiffpy from source but I guess that's not preferred?

I see. Is libdiffpy ready to release? Is it passing tests?

It is passing all tests on my end, but we might need to set version requirements for diffpy.srreal since this is breaking abi. Maybe we need a new version tag?

@sbillinge

Copy link
Copy Markdown
Contributor

@SYMSCAE on a separate PR please could you do the upgrade of the pre-commit packages so that everything is at least passing pre-commit?
For the failing tests I don't really have a clear idea abot which ones we expect to be falling (didn't migrate those things) and which are concerning (something broken). Maybe it is best of you can add a few words in the comments about these issues and what you want me to do, for example, to merge an intermediate step, or not merge because we want to get all tests passing on the branch, for example..

Sure I'll make another pr to upgrade pre-commit to main and see what that happens. The tests are failing because ci is fetching libdiffpy from conda so it will continue to fail until we make a new conda release. It can be fixed if we change the workflow to fetch and build libdiffpy from source but I guess that's not preferred?

I see. Is libdiffpy ready to release? Is it passing tests?

It is passing all tests on my end, but we might need to set version requirements for diffpy.srreal since this is breaking abi. Maybe we need a new version tag?

I think we just need a new release. @ycexiao or Rundong can help with that.

@ycexiao

ycexiao commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

@SYMSCAE on a separate PR please could you do the upgrade of the pre-commit packages so that everything is at least passing pre-commit?
For the failing tests I don't really have a clear idea abot which ones we expect to be falling (didn't migrate those things) and which are concerning (something broken). Maybe it is best of you can add a few words in the comments about these issues and what you want me to do, for example, to merge an intermediate step, or not merge because we want to get all tests passing on the branch, for example..

Sure I'll make another pr to upgrade pre-commit to main and see what that happens. The tests are failing because ci is fetching libdiffpy from conda so it will continue to fail until we make a new conda release. It can be fixed if we change the workflow to fetch and build libdiffpy from source but I guess that's not preferred?

I see. Is libdiffpy ready to release? Is it passing tests?

It is passing all tests on my end, but we might need to set version requirements for diffpy.srreal since this is breaking abi. Maybe we need a new version tag?

I think we just need a new release. @ycexiao or Rundong can help with that.

Hi @sbillinge @SYMSCAE, I am happy to help. Just to clarify, do you mean a new release of libdiffpy? Are there any other compatibility issues I need to pay attention to?

@sbillinge

Copy link
Copy Markdown
Contributor

Hi @sbillinge @SYMSCAE, I am happy to help. Just to clarify, do you mean a new release of libdiffpy? Are there any other compatibility issues I need to pay attention to?

yes. Libdiffpy has been changed to be compatible with nanobind. We need to think a bit about this because this may not be backwards compatible so we need to make sure that everything works. If someone installs a version of srreal that uses boost then it needs to load the old version of libdiffpy but if it loads a version of srreal that uses nanobind, it loads the new version. Please could we look into how to handle that elegantly?

@SYMSCAE

SYMSCAE commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

@SYMSCAE on a separate PR please could you do the upgrade of the pre-commit packages so that everything is at least passing pre-commit?
For the failing tests I don't really have a clear idea abot which ones we expect to be falling (didn't migrate those things) and which are concerning (something broken). Maybe it is best of you can add a few words in the comments about these issues and what you want me to do, for example, to merge an intermediate step, or not merge because we want to get all tests passing on the branch, for example..

Sure I'll make another pr to upgrade pre-commit to main and see what that happens. The tests are failing because ci is fetching libdiffpy from conda so it will continue to fail until we make a new conda release. It can be fixed if we change the workflow to fetch and build libdiffpy from source but I guess that's not preferred?

I see. Is libdiffpy ready to release? Is it passing tests?

It is passing all tests on my end, but we might need to set version requirements for diffpy.srreal since this is breaking abi. Maybe we need a new version tag?

I think we just need a new release. @ycexiao or Rundong can help with that.

Hi @sbillinge @SYMSCAE, I am happy to help. Just to clarify, do you mean a new release of libdiffpy? Are there any other compatibility issues I need to pay attention to?

Hi @ycexiao, yes it is a new release of libdiffpy. But before that, @sbillinge should we first merge changes to main for libdiffpy? Or you would like to have tests wired in the ci first?

@sbillinge

Copy link
Copy Markdown
Contributor

@SYMSCAE

Hi @ycexiao, yes it is a new release of libdiffpy. But before that, @sbillinge should we first merge changes to main for libdiffpy? Or you would like to have tests wired in the ci first?

indeed, this will be part of the release sequence.

@SYMSCAE

SYMSCAE commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

@sbillinge I've synced changes with main, we should get rest of the ci working once the new version of libdiffpy is available.

@ycexiao

ycexiao commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

yes. Libdiffpy has been changed to be compatible with nanobind. We need to think a bit about this because this may not be backwards compatible so we need to make sure that everything works. If someone installs a version of srreal that uses boost then it needs to load the old version of libdiffpy but if it loads a version of srreal that uses nanobind, it loads the new version. Please could we look into how to handle that elegantly?

@sbillinge there is a closely related FAQ in the Conda-forge documentation How to handle breaking of a package due to ABI incompatibility?

In summary:

  1. Add run_export in libdiffpy's meta.yaml in the next release
  2. Hot-fix the published version of diffpy.srreal to pin the version of libdiffpy by Repodata patching
  3. If we plan to add libdiffpy to conda-forge-pinning, create PR to Conda-forge-pinning-feedstock

Also I think we should update the setup.py in the diffpy.srreal.

Pease see if it is what we want.

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