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-aqua: init at 0.6.5 #83447

Merged
merged 2 commits into from Apr 6, 2020

Conversation

@drewrisinger
Copy link
Contributor

@drewrisinger drewrisinger commented Mar 26, 2020

Motivation for this change

Qiskit Aqua: An extensible library of quantum computing algorithms.

This commit follows the new Qiskit scheme of breaking one large package
into smaller packages (terra, aer, etc), and then having a single
meta-package "qiskit" that comprises them.

Broken out of #78772.
Waiting on #83306 #80662 #83372 to pass, builds cleanly when they're cherry-picked.
python38Packages.qiskit-aqua will be broken due to upstream broken python38Packages.scikit-build (#83305 )

I disabled a lot of tests here, but there's still some 900 that run. So it's pretty well-tested.

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.
@drewrisinger drewrisinger requested review from FRidh and jonringer as code owners Mar 26, 2020
@ofborg ofborg bot added the 6.topic: python label Mar 26, 2020
@drewrisinger drewrisinger changed the title [WAITING on #83306, 80662, 83372] pythonPackages.qiskit-aqua: init at 0.6.5 [WAITING on #83306, 80662] pythonPackages.qiskit-aqua: init at 0.6.5 Mar 27, 2020
@drewrisinger
Copy link
Contributor Author

@drewrisinger drewrisinger commented Mar 27, 2020

@jonringer @FRidh at this point it's just #83306 & #80662 left to finish #78772. I could wrap up the rest of the qiskit fixes in one PR, there's no new packages at this point. That way I could also demonstrate via bot that they all build.

Summary of remaining qiskit fixes (all done, just didn't want to keep spamming PRs):

  • Add pythonPackages.qiskit-[aer, ignis, aqua]. See #83306, #80662
  • Delete pythonPackages.IBMQuantumExperience (superseded by pythonPackages.qiskit-ibmq-provider in merged #83320 )
  • bump pythonPackages.qiskit -> 0.16.2, remove broken tag, make it a superset of the qiskit-[terra,aer,ignis,aqua] packages. #78772
@drewrisinger drewrisinger force-pushed the drewrisinger:dr-pr-python-qiskit-aqua branch from 798df9b to 01a3401 Apr 2, 2020
@drewrisinger drewrisinger changed the title [WAITING on #83306, 80662] pythonPackages.qiskit-aqua: init at 0.6.5 pythonPackages.qiskit-aqua: init at 0.6.5 Apr 2, 2020
@drewrisinger
Copy link
Contributor Author

@drewrisinger drewrisinger commented Apr 2, 2020

@GrahamcOfBorg eval
running build locally (both via nixpkgs-review and nix-build). Will update with status

@drewrisinger
Copy link
Contributor Author

@drewrisinger drewrisinger commented Apr 2, 2020

Update: builds clean via nixpkgs-review pr 83447.

2 package built:
python37Packages.qiskit-aqua python38Packages.qiskit-aqua

@drewrisinger
Copy link
Contributor Author

@drewrisinger drewrisinger commented Apr 3, 2020

@bhipple @timokau @bcdarwin got a second to review? Clean eval, build is clean locally & via nixpkgs-review pr 83447.

Copy link
Member

@bcdarwin bcdarwin left a comment

commit/PR should refer to python3Packages

"--ignore-glob=*qgan.py"
];
disabledTests = [
# Disabled due to missing pyscf

This comment has been minimized.

@bcdarwin

bcdarwin Apr 3, 2020
Member

If pyscf can't be added to checkInputs for some reason, probably worth noting. (Indeed, consider adding a comment explaining why it's removed from the dependencies?)

This comment has been minimized.

@drewrisinger

drewrisinger Apr 3, 2020
Author Contributor

I tried packaging pyscf in #78872, and myself & jonringer ended up deciding that it's being too difficult/ugly to package (issue mostly coming down to pyscf tests being very slow, failures difficult to diagnose, and long list of disabled tests that is hard to maintain). I can add note to that effect in code.

@drewrisinger drewrisinger changed the title pythonPackages.qiskit-aqua: init at 0.6.5 python3Packages.qiskit-aqua: init at 0.6.5 Apr 3, 2020
@drewrisinger drewrisinger force-pushed the drewrisinger:dr-pr-python-qiskit-aqua branch from 01a3401 to d283a0a Apr 3, 2020
@bcdarwin
Copy link
Member

@bcdarwin bcdarwin commented Apr 3, 2020

Looks fine to me, will try to build later.

postPatch = ''
# Make pyscf optional, can be included at runtime via pip shell
substituteInPlace setup.py \
--replace "pyscf; sys_platform == 'linux' or (python_version < '3.8' and sys_platform != 'win32')" ""

This comment has been minimized.

@timokau

timokau Apr 3, 2020
Member

How will that work at runtime? Imagine you're a user that just installed this package from nixpkgs and expects it to work. Will there be unexpected crashes? If so, we should patch in a proper error message explaining the situation.

This comment has been minimized.

@drewrisinger

drewrisinger Apr 3, 2020
Author Contributor

Good point. I'd be fine adding error message on import. Will try to patch in.

@drewrisinger drewrisinger force-pushed the drewrisinger:dr-pr-python-qiskit-aqua branch 2 times, most recently from b7ff273 to 51770fd Apr 3, 2020
@drewrisinger
Copy link
Contributor Author

@drewrisinger drewrisinger commented Apr 3, 2020

Let's see if we can actually build it now via borg after #84124...
@GrahamcOfBorg build python37Packages.qiskit-aqua
@GrahamcOfBorg build python38Packages.qiskit-aqua
Took everyone's code review suggestions into account, and added an ugly-ish description of why pyscf #78872 isn't included, along with an ImportWarning when someone would try to use the appropriate chemistry section of this package.

Copy link
Member

@timokau timokau left a comment

Looks like the aarch64 build is failing. Please restrict meta.platforms to x86-linux.

Qiskit Aqua: An extensible library of quantum computing algorithms.

This commit follows the new Qiskit scheme of breaking one large package
into smaller packages (terra, aer, etc), and then having a single
meta-package "qiskit" that comprises them.
Build was failing on ofborg on platforms.aarch64 due to
missing muparserx library built for aarch64. Added notes about
this issue & when build could be expanded.
@drewrisinger drewrisinger force-pushed the drewrisinger:dr-pr-python-qiskit-aqua branch from 51770fd to ab4336f Apr 6, 2020
@drewrisinger
Copy link
Contributor Author

@drewrisinger drewrisinger commented Apr 6, 2020

@GrahamcOfBorg build python37Packages.qiskit-aqua python38Packages.qiskit-aqua
Left python3Packages.pyscf discussions "unresolved" above, even though they're actually resolved, as future notes for anyone who finds this issue linked in description.

Traced failing build to pythonPackages.qiskit-aer which had original unfree dependency and so couldn't test against aarch64 on ofborg. Left notes about what the issue is; fully resolving it would require packaging https://github.com/beltoforion/muparserx, which I'll leave for another day.

@ofborg ofborg bot added the 8.has: clean-up label Apr 6, 2020
@timokau
timokau approved these changes Apr 6, 2020
@timokau
Copy link
Member

@timokau timokau commented Apr 6, 2020

Looks good, thanks!

@timokau timokau merged commit ae55e4e into NixOS:master Apr 6, 2020
16 checks passed
16 checks passed
python37Packages.qiskit-aqua, python38Packages.qiskit-aqua on aarch64-linux No attempt
Details
python37Packages.qiskit-aqua, python38Packages.qiskit-aqua on x86_64-linux Timed out, unknown build status
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ab4336f"; rev="ab4336f7b76bdf6b300c6063259cd64c40b63c58"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ab4336f"; rev="ab4336f7b76bdf6b300c6063259cd64c40b63c58"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ab4336f"; rev="ab4336f7b76bdf6b300c6063259cd64c40b63c58"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ab4336f"; rev="ab4336f7b76bdf6b300c6063259cd64c40b63c58"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ab4336f"; rev="ab4336f7b76bdf6b300c6063259cd64c40b63c58"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ab4336f"; rev="ab4336f7b76bdf6b300c6063259cd64c40b63c58"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ab4336f"; rev="ab4336f7b76bdf6b300c6063259cd64c40b63c58"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@drewrisinger drewrisinger deleted the drewrisinger:dr-pr-python-qiskit-aqua branch Apr 6, 2020
@drewrisinger drewrisinger mentioned this pull request Apr 6, 2020
4 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.