Skip to content
This repository was archived by the owner on Jan 3, 2023. It is now read-only.

Make required numpy version for setup and install explicit. #3163

Merged
merged 6 commits into from
Jul 23, 2019

Conversation

silee2
Copy link
Contributor

@silee2 silee2 commented Jul 2, 2019

Trying to generalize #3162 and avoid building numpy from source
numpy 1.15 is the last binary wheel to support python 3.4
numpy 1.16 seems like the last binary wheel to support python 2.7
Also trying to avoid the issue mentioned here
pytest-dev/pytest-xdist#136
This PR assumes nGraph supports python 2.7, 3.4, 3.5, 3.6, 3.7

@silee2 silee2 requested a review from postrational as a code owner July 2, 2019 23:36
@@ -1,4 +1,4 @@
six
numpy==1.15.4; python_version == "3.4"
numpy; python_version != "3.4"
numpy==1.16.4; python_version != "3.4"
Copy link
Member

Choose a reason for hiding this comment

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

I think we should we hard-code only for Python 2.7 and 3.4.
Let the user use latest version (or version required by other packages) in other cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

python/setup.py Outdated
@@ -363,6 +363,22 @@ def build_extensions(self):
build_ext.build_extensions(self)


setup_requires = [
'numpy==1.15.4; python_version == "3.4"',
'numpy==1.16.4; python_version != "3.4"',
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid replicating this code by parsing the requirements.txt file?
This should work:
setup_requires = open('requirements.txt').read().splitlines()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@silee2
Copy link
Contributor Author

silee2 commented Jul 9, 2019

@aslepko @postrational I have made all the requested changes. Should we merge this PR or #3164 to master? #3162 which is identical to #3164 was merged to r0.23

@diyessi diyessi requested a review from postrational July 19, 2019 15:28
@postrational postrational added the Fully Reviewed PR is approved by all reviewers. label Jul 23, 2019
@diyessi diyessi merged commit 4268291 into master Jul 23, 2019
@diyessi diyessi deleted the silee2/numpy branch July 23, 2019 20:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fully Reviewed PR is approved by all reviewers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants