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

Add qiskit-aer (and muparserx) #21404

Merged
merged 11 commits into from
Feb 14, 2023
Merged

Add qiskit-aer (and muparserx) #21404

merged 11 commits into from
Feb 14, 2023

Conversation

wshanks
Copy link
Contributor

@wshanks wshanks commented Dec 2, 2022

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged.
  • Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

@conda-forge-linter
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/muparserx, recipes/qiskit-aer) and found some lint.

Here's what I've got...

For recipes/muparserx:

  • The recipe license should not include the word "License".

For recipes/muparserx:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.

For recipes/qiskit-aer:

  • Non noarch packages should have python requirement without any version constraints.
  • Non noarch packages should have python requirement without any version constraints.

@conda-forge-linter
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/muparserx, recipes/qiskit-aer) and found some lint.

Here's what I've got...

For recipes/qiskit-aer:

  • Non noarch packages should have python requirement without any version constraints.
  • Non noarch packages should have python requirement without any version constraints.

@conda-forge-linter
Copy link

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 (recipes/muparserx, recipes/qiskit-aer) and found it was in an excellent condition.

@wshanks
Copy link
Contributor Author

wshanks commented Dec 5, 2022

Current status:

  • The Linux build is passing now. I added a couple patches to work around quirks of scikit-build. There could be better ways to avoid the issues (scikit-build trying to pip install cmake and scikit-build copying everything from MANIFEST.in into the package).

  • I am a little uncertain about libblas, liblapack, and libgomp/llvm-openmp. Should those be run dependencies as well or are they just needed for compilation? muparserx is dynamically linked, so the package does not work without it, and I think it should be similar for the other libraries.

  • A Windows build is not possible right now because the qiskit-terra dependency is not currently built for Windows.

  • The osx build is failing with undeclared identifier '_SC_PHYS_PAGES':

    FAILED: qiskit_aer/backends/wrappers/CMakeFiles/controller_wrappers.dir/bindings.cc.o
    /Users/runner/mambaforge/conda-bld/qiskit-aer_1669948044976/_build_env/bin/x86_64-apple-darwin13.4.0-clang++ -Dcontroller_wrappers_EXPORTS -I/Users/runner/mambaforge/conda-bld/qiskit-aer_1669948044976/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehol/include/python3.10 -I/Users/runner/mambaforge/conda-bld/qiskit-aer_1669948044976/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehol/lib/python3.10/site-packages/pybind11/include -I/Users/runner/mambaforge/conda-bld/qiskit-aer_1669948044976/work/src -march=core2 -mtune=haswell -mssse3 -ftree-vectorize -fPIC -fPIE -fstack-protector-strong -O2 -pipe -stdlib=libc++ -fvisibility-inlines-hidden -fmessage-length=0 -isystem /Users/runner/mambaforge/conda-bld/qiskit-aer_1669948044976/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehol/include -fdebug-prefix-map=/Users/runner/mambaforge/conda-bld/qiskit-aer_1669948044976/work=/usr/local/src/conda/qiskit-aer-0.11.1 -fdebug-prefix-map=/Users/runner/mambaforge/conda-bld/qiskit-aer_1669948044976/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehol=/usr/local/src/conda-prefix -pedantic -Wall -Wfloat-equal -Wundef -Wcast-align -Wwrite-strings -Wmissing-declarations -Wredundant-decls -Wshadow -Woverloaded-virtual -fopenmp=libomp -mpopcnt -O3 -DNDEBUG -arch x86_64 -isysroot /Applications/Xcode_13.2.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk -mmacosx-version-min=10.9 -fPIC -fvisibility=hidden   -fopenmp=libomp -std=gnu++14 -MD -MT qiskit_aer/backends/wrappers/CMakeFiles/controller_wrappers.dir/bindings.cc.o -MF qiskit_aer/backends/wrappers/CMakeFiles/controller_wrappers.dir/bindings.cc.o.d -o qiskit_aer/backends/wrappers/CMakeFiles/controller_wrappers.dir/bindings.cc.o -c /Users/runner/mambaforge/conda-bld/qiskit-aer_1669948044976/work/qiskit_aer/backends/wrappers/bindings.cc
    In file included from /Users/runner/mambaforge/conda-bld/qiskit-aer_1669948044976/work/qiskit_aer/backends/wrappers/bindings.cc:19:
    In file included from /Users/runner/mambaforge/conda-bld/qiskit-aer_1669948044976/work/src/framework/results/pybind_result.hpp:20:
    In file included from /Users/runner/mambaforge/conda-bld/qiskit-aer_1669948044976/work/src/framework/results/legacy/pybind_data.hpp:19:
    In file included from /Users/runner/mambaforge/conda-bld/qiskit-aer_1669948044976/work/src/framework/results/legacy/snapshot_data.hpp:22:
    /Users/runner/mambaforge/conda-bld/qiskit-aer_1669948044976/work/src/framework/utils.hpp:1280:34: error: use of undeclared identifier '_SC_PHYS_PAGES'
    size_t pages = (size_t)sysconf(_SC_PHYS_PAGES);
    

    From what I see searching for _SC_PHYS_PAGES, there seems to be uneven support for this symbol on macOS. I wonder why the upstream CI works though. It is using GitHub Actions with no special set up that I see (see here). Could an extra dependency help?

  • qiskit-aer also provides a standalone binary. I will leave it for the future to package that as a separate package. For now just the Python project is built and packaged.

  • qiskit-aer has a GPU backend that requires NVIDIA hardware. It packages the GPU enabled build as a separate qiskit-aer-gpu package here. I am not sure if conda-forge would do that as well or would try to make the gpu backend a build variant. I will leave a GPU build as future work unless someone thinks it would be easy to add on in this recipe.

  • qiskit-aer has MPI support. This is similar to the GPU case, but I think it might be possible to turn this on without messing up someone not running on a cluster. For now, I will leave it disabled unless someone suggests it is easy to turn on without disrupting the single node use case.

@wshanks
Copy link
Contributor Author

wshanks commented Dec 5, 2022

@conda-forge/help-c-cpp Do you have any suggestions for _SC_PHYS_PAGES issue described in my previous comment (the fourth point)?

Also, do you have any feedback regarding run dependencies on the libraries mentioned in second point in my previous comment?

@carterbox
Copy link
Member

A Windows build is not possible right now because the qiskit-terra dependency is not currently built for Windows.

That happens sometimes. Write an explainer comment in the recipe next to the skip so we know why.

@carterbox
Copy link
Member

I am a little uncertain about libblas, liblapack, and libgomp/llvm-openmp. Should those be run dependencies as well or are they just needed for compilation? muparserx is dynamically linked, so the package does not work without it, and I think it should be similar for the other libraries.

There are sections in the conda-forge docs that explain how to use both openmp and the BLAS in a recipe. Generally shared libraries are listed in host and not in run because their recipes will have run_exports.

@carterbox
Copy link
Member

qiskit-aer has a GPU backend that requires NVIDIA hardware. It packages the GPU enabled build as a separate qiskit-aer-gpu package here. I am not sure if conda-forge would do that as well or would try to make the gpu backend a build variant. I will leave a GPU build as future work unless someone thinks it would be easy to add on in this recipe.

We support CUDA build variants. If the CUDA package is orthogonal to these packages, then it should get its own recipe. If it is a build variant, then it shares a recipe.

@carterbox
Copy link
Member

From what I see searching for _SC_PHYS_PAGES, there seems to be uneven support for this symbol on macOS. I wonder why the upstream CI works though. It is using GitHub Actions with no special set up that I see (see here). Could an extra dependency help?

This is a portability question for Upstream.

Copy link
Member

@carterbox carterbox left a comment

Choose a reason for hiding this comment

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

Thank you for already upstreaming your patches.

recipes/qiskit-aer/meta.yaml Outdated Show resolved Hide resolved
recipes/qiskit-aer/meta.yaml Outdated Show resolved Hide resolved
recipes/qiskit-aer/meta.yaml Outdated Show resolved Hide resolved
recipes/qiskit-aer/meta.yaml Outdated Show resolved Hide resolved
Comment on lines 29 to 31
commands:
- test -f ${PREFIX}/include/muparserx/mpParser.h # [unix]
- test -f ${PREFIX}/lib/libmuparserx${SHLIB_EXT} # [unix]
Copy link
Member

Choose a reason for hiding this comment

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

Please existence tests for a CMake and package config file to indicate that they are available.

@wshanks
Copy link
Contributor Author

wshanks commented Dec 18, 2022

Thanks, @carterbox, for the review! I am reading up on pinning and run_exports to try to update this PR (I'll bump the version to the one with my patch as well). By the way, did you have any idea about the macOS build failure? -- I missed the comment about it being a question for upstream.

recipes/qiskit-aer/meta.yaml Outdated Show resolved Hide resolved
@leofang
Copy link
Member

leofang commented Dec 21, 2022

Hi @wshanks, I am from NVIDIA cuQuantum team, also a long time CUDA package maintainer here.

I am happy to help with CUDA builds either in this PR, or in subsequent PRs after a feedstock is created. But, due to a Qiskit Aer bug that we just reported (Qiskit/qiskit-aer#1686), currently your import qiskit_aer test would fail for the CUDA build. We can skip it, but it's better to have it fixed (in parallel, maybe). Happy to discuss steps to move forward.

cc: @tlubowe for vis (once the CUDA build is setup, the cuQuantum::cuStateVec support can be built by listing custatevec as run_constrained in this conda recipe).

@leofang
Copy link
Member

leofang commented Dec 21, 2022

  • qiskit-aer has MPI support. This is similar to the GPU case, but I think it might be possible to turn this on without messing up someone not running on a cluster. For now, I will leave it disabled unless someone suggests it is easy to turn on without disrupting the single node use case.

Indeed. The MPI build for Qiskit Aer would not work for single-node users, as MPI_Init_thread is called unconditionally once AER_MPI is set at build time:
https://github.com/Qiskit/qiskit-aer/blob/7bfdbb4e49c82c982f9c7f22de7b04ff19196d2e/contrib/standalone/qasm_simulator.cpp#L100

It would require you to build another flavor, following the conda-forge documentation. The cuQuantum feedstock could be one good example. The only extra thing we do is to ensure the nompi flavor is picked when the user environment does not have MPI installed, and when MPI is installed the solver would pick the MPI flavor accordingly.

Now, the real challenge here is there could be a number of flavors to build if you aim to support them all, due to Aer's design:

  • CPU-only, nompi (done in this PR)
  • CPU-only, with MPI (<--- we are discussing this)
  • CPU+GPU, nompi (see my previous comment)
  • CPU+GPU, with MPI (???)

so I'd suggest to not worry about MPI until after this feedstock is created.

@wshanks
Copy link
Contributor Author

wshanks commented Dec 22, 2022

Thanks, @leofang! I agree it seems best to defer further builds until after the feedstock is generated.

From what you have seen, does it make sense to build a CUDA flavor of the qiskit-aer package or is it better to output a qiskit-aer-gpu package? I am not too familiar with either qiskit aer's or conda's interaction with GPU's. I think mainly the question is if the GPU build of qiskit-aer is always an upgrade over the non-GPU one or if you sometimes still want the CPU one?

My main use case for qiskit-aer is for running the unit tests of downstream qiskit projects. I don't have a lot of experience working with it directly.

@wshanks
Copy link
Contributor Author

wshanks commented Dec 22, 2022

The current status of this PR is that I think the recipes are in good shape.

I opened Qiskit/qiskit-aer#1688 about getting qiskit-aer updated so that the remaining patch can be dropped from the recipe, but I don't think the feedstock needs to wait for that.

I opened Qiskit/qiskit-aer#1689 about the build issue on macOS. We can see what the response from qiskit-aer is about that. If qiskit-aer addresses that issue but is not going to release soon, we could backport the change to 0.11.2 as a patch in the recipe and generate the feedstock. Other options would be to generate the feedstock without macOS or to make our own patch for the error, maybe based off of the answer to this StackOverflow question.

@carterbox
Copy link
Member

From what you have seen, does it make sense to build a CUDA flavor of the qiskit-aer package or is it better to output a qiskit-aer-gpu package?

Our build system is set up to build cuda and non-cuda variants. When you add the cuda compiler as a build dependency, we automatically insert a cuda_compiler_version variable that is None, 10, or 11. See https://github.com/conda-forge/pytorch-cpu-feedstock/blob/main/recipe/meta.yaml

@leofang
Copy link
Member

leofang commented Dec 22, 2022

I guess @wshanks is asking if it is necessary to maintain the parity between PyPI and conda-forge. The answer is no, especially with regard to GPU packages. pip has no way to detect GPU variants, so people have to encode -gpu or -cu11 etc in the wheel name. It's not the case at all for conda packages.

There are ways to default to a CPU only build or a GPU build when updating the recipe. It is also possible to make the two builds mutually exclusive, similar to the nompi approach. For example, PyTorch's conda package does that.

@wshanks
Copy link
Contributor Author

wshanks commented Dec 23, 2022

I guess @wshanks is asking if it is necessary to maintain the parity between PyPI and conda-forge.

Yes, partly that, though I think my thinking when answering that question was not totally clear. I was thinking there were two kinds of GPU support -- one where you build against a required GPU-oriented dependency instead of a CPU one and one where you add optional features that require a GPU. It seems like qiskit-aer fits the first case and to switch between versions you need to install or not install the GPU library (some part of CUDA) to signal the preference. If qiskit-aer-gpu added on a way to select CPU or GPU at run time, maybe it would make sense as a separate package but even then maybe for conda it would be better to merge it into the GPU build of the main qiskit-aer.

@leofang
Copy link
Member

leofang commented Dec 23, 2022

On PyPI, AFAIK qiskit-aer-gpu is a superset of qiskit-aer: It includes both CPU and GPU simulators. At runtime, users can still choose to use the CPU simulator by setting AerSimulator(device='CPU', ...) even if using qiskit-aer-gpu. Therefore, Qiskit Aer is not very different from, say, PyTorch or TensorFlow. But for the latter ones, we still have CPU-only builds and "GPU" builds (also being a superset), and allow users to pick one (and only one) among them, the reason being the CPU build usually has less dependencies to install, saving bandwidth and storage.

(This discussion limits to single-node users; in Qiskit Aer, the MPI support is a different story, as there is no runtime option to turn it on/off...)

@leofang
Copy link
Member

leofang commented Feb 1, 2023

@conda-forge-admin, please restart ci

@conda-forge-webservices
Copy link

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 (recipes/muparserx, recipes/qiskit-aer) and found it was in an excellent condition.

@wshanks
Copy link
Contributor Author

wshanks commented Feb 1, 2023

The current status here on my part is that the Linux build seems okay to me now. I was hoping to get at least Linux and Mac supported before merging. I was hoping to get a response to Qiskit/qiskit-aer#1689 for that. Without that though, my plan was to play around with a patch like the one I pointed to there myself, but I have not had time to do that so far. It's fine if anyone wants to help with that. Also, if anyone wants this strongly on Linux, maybe we could merge without Mac and continue work in the new feedstock repo.

@leofang
Copy link
Member

leofang commented Feb 1, 2023

Thanks @wshanks, I was typing while you already responded 🙂 I agree temporarily disabling osx builds is the easiest, we should do that unless you can patch it in ~30 mins or so. As for the patch, this seems to be the way to go:
https://stackoverflow.com/questions/30511579/linux-sc-phys-pages-not-working-on-mac-os-x

@wshanks
Copy link
Contributor Author

wshanks commented Feb 4, 2023

@leofang Okay, I skipped osx for now. I think this is ready for review. Feel free to add yourself as a maintainer if you want 🙂

Also, if anyone reviewing this is interested in qiskit, it would be great to get #21602 reviewed as well.

@wshanks wshanks marked this pull request as ready for review February 4, 2023 15:27
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 overall, no major concerns, only a few quick questions.

I don't have merge privilege here, so let's ping @conda-forge/help-python-c 🙂

- pybind11
- scikit-build >=0.11.0
- setuptools >=40.1.0
- spdlog =1.5.0
Copy link
Member

Choose a reason for hiding this comment

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

Q: Is this necessary? 1.5.0 is quite old.

Copy link
Contributor Author

@wshanks wshanks Feb 12, 2023

Choose a reason for hiding this comment

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

Likely not. I took it from qiskit-aer's cmake configuration. Since then it was bumped to 1.9.2 in Qiskit/qiskit-aer#1637 without comment, so I assume the version bump was incidental and not related to the other code changes.

I am not sure what the best practice is here for conda. Since spdlog is a header only project, I was assuming it would only influence the aer binary and not the rest of the conda environment so it was best to mirror the upstream pin, but we could possibly use just set a minimum version?

Edit: so when the recipe bumps to the new version that specifies spdlog 1.9.2, I will update the spdlog version. The question is if it should be bumped early.

- liblapack
- muparserx =4.0.8
- nlohmann_json =3.1.2
- numpy
Copy link
Member

Choose a reason for hiding this comment

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

Q: Is NumPy header used at build time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was guessing so since it is in the `build-system.requires of qiskit-aer's pyproject.toml. The main simulator is in C++ with Python bindings, so I was thinking the numpy headers would be needed for passing state vector and density matrix results across that interface. I don't see any direct references to numpy headers in the code but I am not that familiar with pybind11 which might provide a layer between numpy and C++? I think the conversion is partially specified here.

recipes/qiskit-aer/meta.yaml Show resolved Hide resolved
@leofang
Copy link
Member

leofang commented Feb 12, 2023

@conda-forge/help-python-c ready for review

@carterbox carterbox merged commit 15d761a into conda-forge:main Feb 14, 2023
@leofang leofang mentioned this pull request Mar 23, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants