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

Remove PySCF from the plugin setup.py #103

Merged
merged 3 commits into from
Sep 21, 2020
Merged

Remove PySCF from the plugin setup.py #103

merged 3 commits into from
Sep 21, 2020

Conversation

josh146
Copy link
Member

@josh146 josh146 commented Sep 21, 2020

No description provided.

@@ -27,8 +27,7 @@
"numpy",
"networkx>=2.2;python_version>'3.5'",
# Networkx 2.4 is the final version with python 3.5 support.
"networkx>=2.2,<2.4;python_version=='3.5'",
"pyscf<=1.7.2"
Copy link
Member

Choose a reason for hiding this comment

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

not necessary? how come it was in there?

Copy link
Member Author

Choose a reason for hiding this comment

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

PySCF has been a buggy for a while; they have released a couple of versions with broken binaries.

PennyLane and Qiskit require mutually exclusive versions of PySCF, and this causes the PennyLane-qiskit plugin to fail at pip installation if Qiskit is already installed (PL-qchem required <=1.7.2, and Qiskit required >1.7.3). We have two options:

  • Including this here forces pip to downgrade pyscf if previously installed by qiskit, allowing qiskit + pl-qiskit + pl-qchem to all be installed together. The downside is that pyscf setup.py file has a bug on windows

  • Removing this will no longer allow qiskit + pl-qiskit + pl-qchem to all be installed together. However, it will allow pennylane-qiskit to be installed on windows.

In retrospect, this is mostly pip's fault at not being able to properly resolve dependencies, and there isn't a good solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would pyscf had to be removed from requirements.txt too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops! I missed that

Copy link
Contributor

Choose a reason for hiding this comment

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

Made a quick patch #104 :)

@codecov
Copy link

codecov bot commented Sep 21, 2020

Codecov Report

Merging #103 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #103   +/-   ##
=======================================
  Coverage   99.23%   99.23%           
=======================================
  Files           7        7           
  Lines         263      263           
=======================================
  Hits          261      261           
  Misses          2        2           
Impacted Files Coverage Δ
pennylane_qiskit/converter.py 100.00% <ø> (ø)
pennylane_qiskit/qiskit_device.py 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dca2e39...85db719. Read the comment docs.

@josh146
Copy link
Member Author

josh146 commented Sep 21, 2020

Okay, this is a lot more subtle than I first thought.

  • PySCF<1.7.2 can use any version of SciPy.

  • PySCF>=1.7.2 requires SciPy<1.5

Upgrading the PySCF version causes a huge number of dependency conflicts.

@josh146
Copy link
Member Author

josh146 commented Sep 21, 2020

See PennyLaneAI/pennylane#814

Copy link
Contributor

@antalszava antalszava left a comment

Choose a reason for hiding this comment

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

💯 Nice! Perhaps the upcoming change to pip in October could help us out in some way here.

@josh146 josh146 merged commit e6cb8bc into master Sep 21, 2020
@josh146 josh146 deleted the josh146-patch-2 branch September 21, 2020 14:35
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.

3 participants