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.

Sign up for GitHub

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

Jump to bottom

python3Packages.cirq: init at 0.6.1 #78779

Merged
merged 2 commits into from Jan 31, 2020
Merged

python3Packages.cirq: init at 0.6.1 #78779

merged 2 commits into from Jan 31, 2020

Conversation

drewrisinger
Copy link
Contributor

@drewrisinger drewrisinger commented Jan 29, 2020 •
edited

Motivation for this change

Add Google's Cirq package.
Uses: Quantum information science.

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 force-pushed the dr-pr-python-cirq branch 2 times, most recently from 4a988b3 to 0b8a9aa Compare January 30, 2020 15:46
@ofborg ofborg bot requested review from lovek323 and timokau January 30, 2020 17:10
timokau
timokau approved these changes Jan 30, 2020
View reviewed changes
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 very good to me! Still, some nitpicks. They are not particularly important, but I tend to keep the bar for new packages quite high.

pkgs/development/python-modules/cirq/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/cirq/default.nix Show resolved Hide resolved
pkgs/development/python-modules/cirq/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/cirq/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/cirq/default.nix Show resolved Hide resolved
pkgs/development/python-modules/cirq/default.nix Show resolved Hide resolved
pkgs/development/python-modules/cirq/default.nix
pyyaml
pygraphviz
];
# TODO: enable op_serializer_test. Error is type checking, for some reason wants bool instead of numpy.bool_. Not sure if protobuf or internal issue
Copy link
Member

Choose a reason for hiding this comment

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

Is there an upstream issue for that?

Also please add a comment that explains why we ignore dev_tools.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I have no idea why op_serializer test is failing. Upstream seems fine.

Copy link
Member

Choose a reason for hiding this comment

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

Can you look into it? Maybe opening an issue upstream is still a good idea, they might have an idea what could be going wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened: quantumlib/Cirq#2727

pkgs/development/python-modules/cirq/default.nix Show resolved Hide resolved
pkgs/development/python-modules/sympy/default.nix Outdated Show resolved Hide resolved
maintainers/maintainer-list.nix Show resolved Hide resolved
timokau
timokau requested changes Jan 30, 2020
View reviewed changes
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.

Meant to request changes, not approve.

@drewrisinger
add maintainer: drewrisinger
fb112ee 
University of Maryland PhD Student. Quantum Computing.
@drewrisinger
Copy link
Contributor Author

drewrisinger commented Jan 30, 2020 •
edited

Tried with cirq v0.7.0, and got following failures. See quantumlib/Cirq#2728 for full details & debugging.

@drewrisinger drewrisinger mentioned this pull request Jan 31, 2020
Closed
@drewrisinger
python3Packages.cirq: init at 0.6.1
74a0d4d 
Add Google's Cirq package.
Uses: Quantum information science.
@drewrisinger drewrisinger mentioned this pull request Jan 31, 2020
Closed
@timokau
Copy link
Member

timokau commented Jan 31, 2020 •
edited

Looks good to me now. It would be nice to resolve the outstanding issues and update to the latest release though. Let's give upstream some time to reply to your issue.

If nothing has happened until monday, feel free to ping me again and I'll merge in the current state (once I get to it, my time is pretty limited right now; if another committer wants to merge, go ahead).

@timokau
Copy link
Member

timokau commented Jan 31, 2020

Thinking about this some more, I think it can already be pretty useful in its current state and the 0.7 update (and hopefully the fix of the remaining issues) can be done as a follow-up. Thank you!

@timokau timokau merged commit 769928e into NixOS:master Jan 31, 2020
@drewrisinger
Copy link
Contributor Author

drewrisinger commented Jan 31, 2020 via email

anna328p pushed a commit to anna328p/nixpkgs that referenced this pull request Feb 2, 2020
@timokau @anna328p
Merge pull request NixOS#78779 from drewrisinger/dr-pr-python-cirq
40f8941 
python3Packages.cirq: init at 0.6.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonringer jonringer jonringer left review comments

@timokau timokau timokau approved these changes

@FRidh FRidh Awaiting requested review from FRidh

@lovek323 lovek323 Awaiting requested review from lovek323

Assignees
No one assigned
Labels
6.topic: python 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@drewrisinger @timokau @jonringer