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

python3Packages.qiskit: 0.25.0 -> 0.26.2 #121606

Merged
merged 8 commits into from
Jun 3, 2021

Conversation

drewrisinger
Copy link
Contributor

@drewrisinger drewrisinger commented May 3, 2021

Motivation for this change

Update super-package & all subpackages to the latest version. https://github.com/Qiskit/qiskit/releases/tag/0.25.3 https://github.com/Qiskit/qiskit/releases/tag/0.26.2

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested a review from pandaman64 May 3, 2021 18:07
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 labels May 3, 2021
@r-rmcgibbo
Copy link

r-rmcgibbo commented May 3, 2021

Result of nixpkgs-review pr 121606 at 94f29042 run on aarch64-linux 1

12 packages failed to build:
2 packages built successfully:
  • python38Packages.yfinance
  • python39Packages.yfinance

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement.


Result of nixpkgs-review pr 121606 at 9a36c867 run on aarch64-linux 1

1 package built successfully:
  • zile

@rmcgibbo
Copy link
Contributor

rmcgibbo commented May 4, 2021

Perhaps these should be marked as broken on aarch64. It seems unlikely that aarch64-linux is a supported platform for upstream.

@drewrisinger
Copy link
Contributor Author

@rmcgibbo it is definitely supported upstream (see e.g. aarch64 wheels on https://pypi.org/project/qiskit-terra/#files), so I'm confused by these build errors.

I'm leaning in favor of just disabling these tests, b/c the build for qiskit-terra is passing on hydra for aarch64 (well, it was until something broke it last night?).
Thoughts?

How can I get your bot to re-run aarch64 tests? I just pushed updates that should fix this.
@GrahamcOfBorg build python38Packages.qiskit

@drewrisinger drewrisinger changed the title python3Packages.qiskit: 0.25.0 -> 0.25.3 python3Packages.qiskit: 0.25.0 -> 0.25.4 May 5, 2021
@rmcgibbo
Copy link
Contributor

rmcgibbo commented May 6, 2021

@rmcgibbo it is definitely supported upstream (see e.g. aarch64 wheels on https://pypi.org/project/qiskit-terra/#files), so I'm confused by these build errors.

I stand corrected.

How can I get your bot to re-run aarch64 tests? I just pushed updates that should fix this.

I just did it manually, but hopefully I'll add that feature soon (rmcgibbo/r-rmcgibbo#3). Edit: the bot ran and updated the prior comment, it looks like it only built a single package... Not sure what that's all about.

If the failures persist, perhaps opening an upstream for guidance would be appropriate. Or does one already exist?

@rmcgibbo
Copy link
Contributor

rmcgibbo commented May 6, 2021

@ofborg eval

@drewrisinger
Copy link
Contributor Author

Whoops, missed the stdenv import. Fixed with latest force-push.

@drewrisinger
Copy link
Contributor Author

Labeling as ZHF b/c of the (hopefully) fixed Darwin build of retworkx, which fixes all the downstream qiskit packages. Don't have a Darwin machine to use for testing.
ZHF: #122042

Copy link
Contributor

@rmcgibbo rmcgibbo left a comment

Choose a reason for hiding this comment

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

Result of nixpkgs-review pr 121606 run on x86_64-darwin 1

10 packages marked as broken and skipped:
  • python38Packages.qiskit
  • python38Packages.qiskit-aer
  • python38Packages.qiskit-aqua
  • python38Packages.qiskit-ibmq-provider
  • python38Packages.qiskit-ignis
  • python39Packages.qiskit
  • python39Packages.qiskit-aer
  • python39Packages.qiskit-aqua
  • python39Packages.qiskit-ibmq-provider
  • python39Packages.qiskit-ignis
6 packages built:
  • python38Packages.qiskit-terra
  • python38Packages.retworkx
  • python38Packages.yfinance
  • python39Packages.qiskit-terra
  • python39Packages.retworkx
  • python39Packages.yfinance

@drewrisinger drewrisinger changed the title python3Packages.qiskit: 0.25.0 -> 0.25.4 python3Packages.qiskit: 0.25.0 -> 0.26.0 May 11, 2021
@risicle
Copy link
Contributor

risicle commented May 13, 2021

nixpkgs-review happy for me, nixos x86_64, macos 10.15 (apart from those marked broken on darwin of course)

@drewrisinger
Copy link
Contributor Author

Latest force-push ONLY updates comment on qiskit-aer patch to note that the PR was accepted. Qiskit/qiskit-aer#1250

@drewrisinger
Copy link
Contributor Author

I've addressed all comments and updated to the latest version of qiskit (0.26.2), as well as squashed some of the commits together. Would you prefer a force-push to this branch or a new PR?

@dotlambda

This comment has been minimized.

@drewrisinger drewrisinger changed the title python3Packages.qiskit: 0.25.0 -> 0.26.0 python3Packages.qiskit: 0.25.0 -> 0.26.2 May 25, 2021
@dotlambda

This comment has been minimized.

@drewrisinger
Copy link
Contributor Author

Result of nixpkgs-review pr 121606 run on x86_64-linux 1
4 packages failed to build:
12 packages built:

@dotlambda Due to your request to move pybind11 to nativeBuildInputs. Will revert that.

ERROR: Could not find a version that satisfies the requirement pybind11>=2.6 (from qiskit-aer)
ERROR: No matching distribution found for pybind11>=2.6

@dotlambda
Copy link
Member

@drewrisinger No, this is not related to pybind11. Some test failed while building python38Packages.qiskit-aer but that might just have been a flaky test cause I can't reproduce it now.
Please put pybind11 in nativeBuildInputs again.

@dotlambda
Copy link
Member

You will need to add it to qiskit's nativeBuildInputs as well.

@drewrisinger
Copy link
Contributor Author

You will need to add it to qiskit's nativeBuildInputs as well.

That doesn't make sense. Here are their setup files:

Specifically, pybind11 is listed as both a setup_requirement and an install_requirement for qiskit-aer only, with the caveat that they admit that it's not strictly an install requirement. https://github.com/Qiskit/qiskit-aer/blob/629374cf0d1333e2f1c2f4fd38d2790d76f91911/setup.py#L72-L75. Why would we package it differently than what the original developers explicitly intended?

I have a clean build via nix-review on x86-64-linux without the changes you are suggesting @dotlambda.

16 packages built:
python38Packages.qiskit python38Packages.qiskit-aer python38Packages.qiskit-aqua python38Packages.qiskit-ibmq-provider python38Packages.qiskit-ignis python38Packages.qiskit-terra python38Packages.retworkx python38Packages.yfinance python39Packages.qiskit python39Packages.qiskit-aer python39Packages.qiskit-aqua python39Packages.qiskit-ibmq-provider python39Packages.qiskit-ignis python39Packages.qiskit-terra python39Packages.retworkx python39Packages.yfinance

@dotlambda
Copy link
Member

So they even admit it's wrong. Please test whether the packages work as expected if you apply

diff --git a/pkgs/development/python-modules/qiskit-aer/default.nix b/pkgs/development/python-modules/qiskit-aer/default.nix
index 82414b87813..550a61320ea 100644
--- a/pkgs/development/python-modules/qiskit-aer/default.nix
+++ b/pkgs/development/python-modules/qiskit-aer/default.nix
@@ -48,16 +48,20 @@ buildPythonPackage rec {
       sha256 = "0ldwzxxfgaad7ifpci03zfdaj0kqj0p3h94qgshrd2953mf27p6z";
     })
   ];
+
   # Remove need for cmake python package
+  # pybind11 should not be in install_requires
   postPatch = ''
     substituteInPlace setup.py \
-      --replace "'cmake!=3.17,!=3.17.0'," ""
+      --replace "'cmake!=3.17,!=3.17.0'," "" \
+      --replace "'pybind11>=2.6'" ""
   '';
 
   nativeBuildInputs = [
     cmake
     ninja
     scikit-build
+    pybind11
   ];
 
   buildInputs = [
@@ -73,7 +77,6 @@ buildPythonPackage rec {
     cvxpy
     cython  # generates some cython files at runtime that need to be cython-ized
     numpy
-    pybind11
   ];
 
   # Disable using conan for build

Maybe I'm wrong and pybind11 is actually used at runtime.

drewrisinger added a commit to drewrisinger/nur-packages that referenced this pull request Jun 1, 2021
Backports changes made to retworkx in NixOS/nixpkgs#121606
to support Darwin by adding a buildInput.
drewrisinger added a commit to drewrisinger/nur-packages that referenced this pull request Jun 1, 2021
Backports changes made to retworkx in NixOS/nixpkgs#121606
to support Darwin by adding a buildInput.
drewrisinger added a commit to drewrisinger/nur-packages that referenced this pull request Jun 1, 2021
Backports changes made to retworkx in NixOS/nixpkgs#121606
to support Darwin by adding a buildInput.
Also fixes retworkx Darwin build.

Fixes linker error for retworkx that required libiconv.
Error log:
error: linking with `/nix/store/47vpv5i10dwfg1cf5wca1k40f982g5fm-clang-wrapper-7.1.0/bin/cc` failed: exit code: 1
    ...
    ld: library not found for -liconv
    clang-7: error: linker command failed with exit code 1 (use -v to see invocation)

Also fix tests running on build, that didn't get converted when switched
to the new way of running Cargo builds for python packages.
Also unbreak build on master by disabling tests that compare visual
matplotlib images.

Also remove force deletes from postCheck.
Use a patch submitted via PR instead of custom substitute.

Remove pybind11 install requirement, as requested by @dotlambda
drewrisinger added a commit to drewrisinger/nur-packages that referenced this pull request Jun 1, 2021
Backports changes made to retworkx in NixOS/nixpkgs#121606
to support Darwin by adding a buildInput.
Also remove unused patch & unneeded docs delete (they're not in $out).
Remove unneeded docs delete, they're no longer in $out.
Add optional subpackages as arguments. Most of these are not in nixpkgs,
but can be found in https://github.com/drewrisinger/nur-packages/.
@drewrisinger
Copy link
Contributor Author

Addressed all comments in latest force-push.

  • All instances of rm -rf -> rm -r
  • Removed pybind11 from propagatedBuildInputs
  • Removed all optional inputs from python3Packages.qiskit
  • Updated python3Packages.retworkx 0.8.0 -> 0.9.0

@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).
If you have any questions or problems please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 121606 run on x86_64-linux 1

16 packages built:
  • python38Packages.qiskit
  • python38Packages.qiskit-aer
  • python38Packages.qiskit-aqua
  • python38Packages.qiskit-ibmq-provider
  • python38Packages.qiskit-ignis
  • python38Packages.qiskit-terra
  • python38Packages.retworkx
  • python38Packages.yfinance
  • python39Packages.qiskit
  • python39Packages.qiskit-aer
  • python39Packages.qiskit-aqua
  • python39Packages.qiskit-ibmq-provider
  • python39Packages.qiskit-ignis
  • python39Packages.qiskit-terra
  • python39Packages.retworkx
  • python39Packages.yfinance

@SuperSandro2000 SuperSandro2000 merged commit 5bba6db into NixOS:master Jun 3, 2021
@drewrisinger drewrisinger deleted the dr-pr-qiskit-bump branch June 3, 2021 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants