Don't proceed into setup.py too far if Python version is unsupported #42870

xuhdev · 2020-08-11T19:02:03Z

This prevents confusing errors when the interpreter encounters some
syntax errors in the middle.

dr-ci · 2020-08-11T19:11:25Z

💊 CI failures summary and remediations

As of commit a9e5ff7 (more details on the Dr. CI page):

✅ None of the CI failures appear to be your fault 💚

1/1 broken upstream at merge base 4fc9e95 since Aug 20

🚧 1 ongoing upstream failure:

These were probably caused by upstream breakages that are not fixed yet:

pytorch_linux_backward_compatibility_check_test since Aug 20
- 🔁 rerun

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 13 times.

xuhdev · 2020-08-11T19:32:57Z

Please let me know if we also want to cap the maximum version here. For example, this confusion #42832 (comment) seems to be caused by Python 3.9, greater than our maximum supported version: #42832 (comment)

The downside is that we might have to also allow people to experiment newer version of Python by giving them some "backdoor" to go through.

setup.py

xuhdev · 2020-08-13T05:48:54Z

@pytorchbot merge this please

ezyang · 2020-08-13T17:40:24Z

setup.py

I'm surprised there isn't a more compact way to write this code

me, too (not I'm aware of)

https://pypi.org/project/semver/ may make the code simple, but need to introduce a new dependency...

How about the following?

Suggested change

python_max_version_str = '.'.join((str(num) for num in python_max_version))

python_max_version_str = '.'.join(map(str, python_max_version))

ezyang · 2020-08-14T03:13:02Z

The max version check seems unnecessary and going to force us to keep modifying this bound, even if we are actually compatible with a new version of Python?

xuhdev · 2020-08-14T17:40:26Z

Yeah, that's the downside of having a maximum version imposed. I think it really depends on what we would like to achieve.

Pros:

Avoid users experience unsupported Python version.
Users won't face strange newer-Python version bugs before we have tested.

Cons:

Prevent users from experimenting.

Of course, we can add a switch, but I think that would be too complicated for this purpose.

Please let me know which way the PyTorch team would like to go; I'm fine with either.

malfet · 2020-08-14T23:50:49Z

setup.py

Hmm, why not use some python3 features like f-strings here?

Suggested change

import platform

python_min_version = (3, 6, 1)

python_min_version_str = '.'.join((str(num) for num in python_min_version))

python_max_version = (3, 9, 0)

python_max_version_str = '.'.join((str(num) for num in python_max_version))

if sys.version_info < python_min_version or sys.version_info >= python_max_version:

print("You are using Python {}. Python >={},<{} is required.".format(platform.python_version(),

python_min_version_str,

python_max_version_str))

sys.exit(-1)

def tuple2str(t, sep='.'):

return sep.join(map(str, t))

python_min_version = (3, 6, 1)

python_max_version = (3, 9, 0)

if sys.version_info < python_min_version or sys.version_info >= python_max_version:

print(f"You are using Python {tuple2str(sys.version_info[:3]}. Python >={tuple2str(python_min_version)},<{tuple2str(python_max_version)} is required.")

sys.exit(-1)

You would receive syntax error if Python is < 3.6, and that's exactly the situation where we are trying to have a reasonable error message.

xuhdev · 2020-08-20T05:00:54Z

So... would we proceed?

ezyang · 2020-08-20T15:02:46Z

I'm going to pull out my veto power, and say, purely on the perspective of not wanting to have to update hardcoded Python version every time a new Python version comes out, let's get rid of the upper bound.

This prevents confusing errors when the interpreter encounters some syntax errors in the middle.

xuhdev · 2020-08-20T18:41:11Z

setup.py

-        ],
+            'Programming Language :: C++',
+            'Programming Language :: Python :: 3',
+        ] + ['Programming Language :: Python :: 3.{}' for i in range(python_min_version[1], python_max_version[1])],


@ezyang Do you think this is a reasonable solution? You've got to change the classifier any way.

facebook-github-bot

@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-08-28T18:14:51Z

@ezyang merged this pull request in 9063bce.

pytorchbot added the open source label Aug 11, 2020

xuhdev force-pushed the detect-min-py-version branch from b199d25 to 7930853 Compare August 11, 2020 22:15

xuhdev requested review from dreiss and orionr August 11, 2020 22:16

nbcsm reviewed Aug 12, 2020

View reviewed changes

setup.py Outdated Show resolved Hide resolved

zhangguanheng66 added module: build Build system issues triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Aug 12, 2020

xuhdev force-pushed the detect-min-py-version branch from 7930853 to 05cc473 Compare August 12, 2020 21:25

xuhdev changed the title ~~Don't proceed into setup.py too far if Python is earlier than minimum~~ Don't proceed into setup.py too far if Python version is unsupported Aug 12, 2020

xuhdev requested a review from nbcsm August 12, 2020 21:26

nbcsm approved these changes Aug 13, 2020

View reviewed changes

pytorchbot added the merge-this-please Was marked for merge with @pytorchbot merge this please label Aug 13, 2020

ezyang reviewed Aug 13, 2020

View reviewed changes

malfet reviewed Aug 14, 2020

View reviewed changes

Don't proceed into setup.py too far if Python version doesn't fit

a9e5ff7

This prevents confusing errors when the interpreter encounters some syntax errors in the middle.

xuhdev force-pushed the detect-min-py-version branch from 05cc473 to a9e5ff7 Compare August 20, 2020 18:40

xuhdev commented Aug 20, 2020

View reviewed changes

facebook-github-bot reviewed Aug 21, 2020

View reviewed changes

facebook-github-bot closed this in 9063bce Aug 28, 2020

facebook-github-bot added the merged label Aug 28, 2020

xuhdev deleted the detect-min-py-version branch August 28, 2020 20:26

mruberry added the Merged label Oct 28, 2020

	python_max_version_str = '.'.join((str(num) for num in python_max_version))
	python_max_version_str = '.'.join(map(str, python_max_version))

-import platform
-python_min_version = (3, 6, 1)
-python_min_version_str = '.'.join((str(num) for num in python_min_version))
-python_max_version = (3, 9, 0)
-python_max_version_str = '.'.join((str(num) for num in python_max_version))
-if sys.version_info < python_min_version or sys.version_info >= python_max_version:
-    print("You are using Python {}. Python >={},<{} is required.".format(platform.python_version(),
-                                                                         python_min_version_str,
-                                                                         python_max_version_str))
-    sys.exit(-1)
+def tuple2str(t, sep='.'):
+   return sep.join(map(str, t))
+python_min_version = (3, 6, 1)
+python_max_version = (3, 9, 0)
+if sys.version_info < python_min_version or sys.version_info >= python_max_version:
+    print(f"You are using Python {tuple2str(sys.version_info[:3]}. Python >={tuple2str(python_min_version)},<{tuple2str(python_max_version)} is required.")
+    sys.exit(-1)

Don't proceed into setup.py too far if Python version is unsupported #42870

Don't proceed into setup.py too far if Python version is unsupported #42870

Uh oh!

Conversation

xuhdev commented Aug 11, 2020

Uh oh!

dr-ci bot commented Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

🚧 1 ongoing upstream failure:

Uh oh!

xuhdev commented Aug 11, 2020

Uh oh!

Uh oh!

xuhdev commented Aug 13, 2020

Uh oh!

ezyang Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

xuhdev Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

nbcsm Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

malfet Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

ezyang commented Aug 14, 2020

Uh oh!

xuhdev commented Aug 14, 2020

Uh oh!

malfet Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

xuhdev Aug 15, 2020

Choose a reason for hiding this comment

Uh oh!

xuhdev commented Aug 20, 2020

Uh oh!

ezyang commented Aug 20, 2020

Uh oh!

xuhdev Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Aug 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dr-ci bot commented Aug 11, 2020 •

edited

Loading