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

Clean up C build dependencies (and bump Qiskit) #32

Merged
merged 10 commits into from
Apr 5, 2024

Conversation

wshanks
Copy link
Contributor

@wshanks wshanks commented Feb 1, 2024

  • Remove muparserx which is not needed as of version 0.13
  • Ignore run exports from spdlog. Only its headers are used at build time.
  • Make numpy a run dependency only. Qiskit Aer interfaces with numpy through the pybind11 headers and does not link directly to numpy.
  • Make the nlohmann_json bounds consistent across environemnts instead of special for osx arm64.
  • Increase qiskit minimum version

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

* Remove muparserx which is not needed as of version 0.13
* Ignore run exports from spdlog. Only its headers are used at build
  time.
* Make numpy a run dependency only. Qiskit Aer interfaces with numpy
  through the pybind11 headers and does not link directly to numpy.
* Make the nlohmann_json bounds consistent across environemnts instead
  of special for osx arm64.
* Increase qiskit minimum version
@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@wshanks wshanks marked this pull request as draft February 1, 2024 21:33
@wshanks
Copy link
Contributor Author

wshanks commented Feb 1, 2024

@leofang I am curious what you think about these changes. The warnings in the build log about DSO's and exports made me look loser at the linking of the C extension. I noticed that muparserx was dropped in the last release, so that was an easy change (as was realizing that I missed bumping the minimum qiskit version).

I also noticed that the extension does not link against the spdlog library. I think it only uses the headers at build time and doesn't need spdlog at run time.

I also noticed that the extension does not link against numpy and looking at the pybind11 documentation I read that pybind has its own numpy headers and does not need numpy at build time. I am a little concerned about numpy 2 based on this issue, but I think the pybind11 feedstock should set some kind of export on numpy if that is a problem rather than bounding numpy here. However, maybe just depending on numpy is more standard for getting the right run constraints any way for conda-forge?

I also bumped up the nlohmann_json version -- I just thought if we are using something different from upstream for one variant and it is working okay we might as well use it for all the variants.

@wshanks wshanks marked this pull request as ready for review February 1, 2024 22:24
@@ -23,6 +23,8 @@ source:
# This patch was accepted in https://github.com/Qiskit/qiskit-aer/pull/1940
# and can be removed from the receipe with the 0.14 release.
- 0001-windows-find-blas.patch
# muparserx usage was removed in https://github.com/Qiskit/qiskit-aer/pull/1884
- 0001-Remove-muparserx-from-cmake-configuration.patch
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The patch is in Qiskit/qiskit-aer#2045. I hadn't opened that when I pushed here or I would have put the link a comment. I don't want to churn through the whole CI for that, but I will put it in if I do another push.

@leofang
Copy link
Member

leofang commented Feb 2, 2024

Thanks, Will, a bit overwhelmed this week, will try to take a close look asap.

@wshanks
Copy link
Contributor Author

wshanks commented Feb 2, 2024

Thanks! No rush. The changes seem to pass the tests at least.

Copy link
Member

@leofang leofang left a comment

Choose a reason for hiding this comment

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

LGTM, sorry for delay

@leofang
Copy link
Member

leofang commented Feb 7, 2024

@wshanks feel free to resolve the conflicts and merge

@wshanks wshanks mentioned this pull request Apr 1, 2024
3 tasks
@wshanks
Copy link
Contributor Author

wshanks commented Apr 1, 2024

@conda-forge-admin, please rerender

@wshanks
Copy link
Contributor Author

wshanks commented Apr 1, 2024

When I rerendered locally, all the CUDA 12 builds got removed, so I am trying rerendering with the bot here.

@wshanks
Copy link
Contributor Author

wshanks commented Apr 1, 2024

@leofang It looks like it also removed the CUDA 12 build here and in #34. Any idea why or how to avoid that?

This was missed before and being pulled in by Qiskit (which recently
dropped it as a dependency).
The pybind11 issue (pybind/pybind11#4606)
requiring a deviation from the upstream migration in
conda-forge-pinning-feedstock should be fixed in the latest release
(2.12).
@wshanks wshanks merged commit 6c2838d into conda-forge:main Apr 5, 2024
30 of 32 checks passed
@wshanks wshanks deleted the c-cleanup branch April 5, 2024 18:10
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.

None yet

2 participants