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

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 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 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):

@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

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

@drewrisinger
Copy link
Contributor Author

Update: builds clean via nixpkgs-review pr 83447.

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

@drewrisinger
Copy link
Contributor Author

@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

Choose a reason for hiding this comment

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

commit/PR should refer to python3Packages

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

@bcdarwin bcdarwin Apr 3, 2020

Choose a reason for hiding this comment

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

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?)

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 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
@bcdarwin
Copy link
Member

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')" ""
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

pkgs/development/python-modules/qiskit-aqua/default.nix Outdated Show resolved Hide resolved
@drewrisinger drewrisinger force-pushed the dr-pr-python-qiskit-aqua branch 2 times, most recently from b7ff273 to 51770fd Compare April 3, 2020 23:09
@drewrisinger
Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

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

pkgs/development/python-modules/qiskit-aqua/default.nix Outdated Show resolved Hide resolved
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
Copy link
Contributor Author

@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.

@timokau
Copy link
Member

timokau commented Apr 6, 2020

Looks good, thanks!

@timokau timokau merged commit ae55e4e into NixOS:master Apr 6, 2020
@drewrisinger drewrisinger deleted the dr-pr-python-qiskit-aqua branch April 6, 2020 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants