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

Modify setup.py to explicitly support Py3.5 and above #471

Merged
merged 3 commits into from
Apr 25, 2020

Conversation

Amertz08
Copy link
Contributor

Title

Please pick a concise, informative and complete title for your PR.
The title is important because it will appear in our change log.

Motivation

Please explain the motivation behind this PR in the description.

If you're fixing a bug, link to the issue number like so:

Only allow Python3.5 and above

Checklist

Before you create the PR, please make sure you have:

  • Picked a concise, informative and complete title
  • Clearly explained the motivation behind the PR
  • Linked to any existing issues that your PR will be solving
  • Included tests for any new functionality
  • Checked that all unit tests pass

Workflow

Please avoid rebasing and force-pushing to the branch of the PR once a review is in progress.
Rebasing can make your commits look a bit cleaner, but it also makes life more difficult from the reviewer, because they are no longer able to distinguish between code that has already been reviewed, and unreviewed code.

@Amertz08
Copy link
Contributor Author

I put only 3.5 as everything else has reached EOL.

@Amertz08
Copy link
Contributor Author

This should probably be part of a 1.11.2

@piskvorky
Copy link
Owner

piskvorky commented Apr 11, 2020

I'm +1 on a major version bump (e.g. 2.0 or 3.0), like @gojomo said here. This is a major change.

Plus IMO we should disable the 1.11.0 and 1.11.1 releases, so they don't get registered (and installed) for py2 users as the "latest supported version". The latest version for py2 is 1.10.0.

@Amertz08
Copy link
Contributor Author

I have no skin in the game as far as versioning goes. If you do decide to do a major version bump then dropping the cloud dependencies with it would be nice.

@Amertz08
Copy link
Contributor Author

Also for bigger changes if you're unsure I would consider using pre-release builds. So something like 1.11dev1 or 1.11rc1. These won't get installed unless you explicitly do pip install --pre smart_open. Release those and wait a bit to see if anyone has issues. Just a thought.

https://www.python.org/dev/peps/pep-0440/

@mpenkov
Copy link
Collaborator

mpenkov commented Apr 11, 2020

@piskvorky My preference is to release this particular PR as a minor, because it's an incremental change over what's already available, and we can handle it relatively quickly to resolve at least some of the problems Py2 users are having.

I think a version bump to 3.0 is good idea, but we should prepare for that, which will take time.

What do you think?

@piskvorky
Copy link
Owner

piskvorky commented Apr 11, 2020

And what will py2 users get when they do pip install smart_open (or pip install gensim)?

I am more concerned with the whole thing working, than the version numbers as such. The versions are meant as a quick indication of "how severe are the changes in this release?" but otherwise nobody cares.

Dropping half (?) of our user base is a pretty severe change, so I'm in favour of a major version bump.

@mpenkov
Copy link
Collaborator

mpenkov commented Apr 11, 2020

And what will py2 users get when they do pip install smart_open (or pip install gensim)?

@Amertz08 Can you answer this definitively? OTOH, I don't know.

(Looking at the docs suggests that the Py2 users will get the latest version that still supports Py2, according to setup.py, which is good).

I am more concerned with the whole thing working, than the versions. The versions are meant as a quick indication of "how severe are the changes in this release?" but otherwise nobody cares.

Yes, I agree.

And dropping half (?) of our user base is a pretty severe change.

I agree that doing that as part of 1.11.0 was a poor choice. In retrospect, that should've been a major bump.

Let's work on a plan to get us back into good shape. How about this?

  1. Merge this PR
  2. Release what we have as 3.0
  3. Disable the 1.11.* versions

Once we do that, will the problem for Py2 gensim users be resolved? Or will we still need to release gensim 3.8.2 to perform the version pin? @menshikh-iv

@Amertz08
Copy link
Contributor Author

This is what happens when you attempt to install via py2.

✔ ~/development/smart_open [python-requires|✔] 
10:09 $ python2.7 -m pip install .
DEPRECATION: Python 2.7 will reach the end of its life on January 1st, 2020. Please upgrade your Python as Python 2.7 won't be maintained after that date. A future version of pip will drop support for Python 2.7.
Processing /Users/adammertz/development/smart_open
smart-open requires Python '>=3.5.*' but the running Python is 2.7.16
✘-1 ~/development/smart_open [python-requires|✔] 
10:09 $ 

@Amertz08
Copy link
Contributor Author

I don't believe it will point a python2 user to the last version that supports it. Just that it stops the user from installing it.

@piskvorky
Copy link
Owner

Oh, OK. In that case, we'll have to pin 1.10.0 in Gensim.

Is a conditional pin possible? I.e. "if you're on Py2.7, pin smart_open==1.10.0; otherwise, install the latest smart_open".

@Amertz08
Copy link
Contributor Author

Yeah you could. But you can't bring any of the new features in smart_open into Gensim

# setup.py

PY2 = sys.version_info.major == 2

requirements = ["a", "b"]

if PY2:
    requirements =  requirements + ["smart_open<1.11"]
else:
    requirements = requirements + ["smart_open"]

setup(install_requires=requirements)

But as I said Gensim could not take advantage of any of the new smart_open features. It would at least allow the developer to use the new smart_open features in their code base. I believe it would also depend on what changes you make in future version of smart_open.

@Amertz08
Copy link
Contributor Author

Gensim looks to be used quite a bit. Dropping py2 support while it has be EOL since the beginning of the year still is a big change. I would probably hold off on this and work on gathering a clear roadmap of features and deprecations. Maybe a poll of your users just to see how many are actually still using python2. Not sure exactly just kinda spit balling here.

My thought on a rough roadmap would be something like this

  • Nix 1.11
  • Release 1.12 w/ Azure support
  • 2.0 drop cloud dependencies and be the last version with Python2 support
    • Keep a release/2.X branch outstanding for bug fixes
  • 3.0 Python 3.5+ support only

@gojomo
Copy link
Contributor

gojomo commented Apr 12, 2020

Python 2's EOL, as of a few months ago, technically means that even severe security issues won't get fixes. Anyone who can should rush for the exits, anyone who can't should be happy using older versions of libraries.

(Supporting a "new" release, with new features, for an underlying platform – Py2 – that's without official maintenance seems to me an unwise example to users.)

@mpenkov mpenkov changed the base branch from master to develop April 25, 2020 06:50
@mpenkov mpenkov changed the title Support only Python3.5 and above Modify setup.py to explicitly support Py3.5 and above Apr 25, 2020
@mpenkov mpenkov merged commit 31cc346 into piskvorky:develop Apr 25, 2020
@@ -1,5 +1,7 @@
# Unreleased

- Modify setup.py to explicitly support Py3.5 and above (PR [#471](https://github.com/RaRe-Technologies/smart_open/pull/471), [@Amertz08](https://github.com/Amertz08))
Copy link
Owner

Choose a reason for hiding this comment

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

Modify setup.py to explicitly support only Py3.5 and above

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Amertz08 Amertz08 deleted the python-requires branch April 28, 2020 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New smart-open 1.11.0 is missing Requires-Python metadata and causes issues under Python 2.
4 participants