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 f-string, since it's not supported by Python 3.5 #5330

Merged
merged 6 commits into from
Feb 21, 2020

Conversation

hcho3
Copy link
Collaborator

@hcho3 hcho3 commented Feb 20, 2020

Closes #5328. Also add Python 3.5 to CI so that f-string does not get accidentally added to the codebase again.

I will need to make a patch and create 1.0.1 patch release.

Aside: Dask now requites Python 3.6+, so I do not run Dask integration tests with Python 3.5.

@hcho3 hcho3 force-pushed the remove_fstring_py35_compat branch from 6f8613c to fb37fa6 Compare February 20, 2020 13:22
Copy link
Member

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

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

Can we run only 1 version of Python? I think it's safe the assume Python 3 is backward compatible.

@hcho3
Copy link
Collaborator Author

hcho3 commented Feb 20, 2020

@trivialfis No, since Dask does not support Python 3.5. What I want is to have almost all parts of XGBoost compatible with Python 3.5 (as promised in setup.py) and only require Python 3.6 for Dask and cuDF integration.

@hcho3 hcho3 requested a review from RAMitchell February 20, 2020 23:36
@hcho3
Copy link
Collaborator Author

hcho3 commented Feb 20, 2020

@JohnZed I'd like to hear from you about supporting Python 3.5. Given that Dask requires 3.6+ we have two options:

  1. Require Python 3.6+ for XGBoost as well OR
  2. Make XGBoost Python 3.5 compatible, except for xgboost.dask module.

Currently, this PR chooses option 2. I prefer having some feedback period before we jump to Python 3.6.

Related: dask/dask#5478, which explains rationale for Dask dropping Python 3.5.

@JohnZed
Copy link
Contributor

JohnZed commented Feb 20, 2020

I would suggest option 2 for now but adding a deprecation warning to release notes (and possibly to setup.py) to warn that python 3.5 support will be removed in the upcoming version 1.1+. That gives anyone stuck on 3.5 a fair warning but does not saddle XGB with long-term support for Python 3.5.

@hcho3
Copy link
Collaborator Author

hcho3 commented Feb 20, 2020

@JohnZed Got it. Let's stick with Python 3.5 for now. I like the idea of adding deprecation notice, so let me do that. We can open a separate thread to discuss when we should drop Python 3.5.

Note that Python 3.5 is scheduled to reach end-of-life on September 13, 2020.

@dmlc dmlc deleted a comment from mli Feb 21, 2020
@hcho3 hcho3 merged commit bf1b2cb into dmlc:release_1.0.0 Feb 21, 2020
@hcho3 hcho3 deleted the remove_fstring_py35_compat branch February 21, 2020 06:47
@hcho3
Copy link
Collaborator Author

hcho3 commented Feb 21, 2020

Merged. I will push out 1.0.1 release now. It's essentially same as 1.0.0 plus this patch.

hcho3 added a commit that referenced this pull request Feb 21, 2020
hcho3 added a commit to hcho3/xgboost that referenced this pull request Feb 21, 2020
* Remove f-string, since it's not supported by Python 3.5

* Add Python 3.5 to CI, to ensure compatibility

* Remove duplicated matplotlib

* Show deprecation notice for Python 3.5

* Fix lint

* Fix lint
hcho3 added a commit that referenced this pull request Feb 21, 2020
* Remove f-string, since it's not supported by Python 3.5 (#5330)

* Remove f-string, since it's not supported by Python 3.5

* Add Python 3.5 to CI, to ensure compatibility

* Remove duplicated matplotlib

* Show deprecation notice for Python 3.5

* Fix lint

* Fix lint

* Fix a unit test that mistook MINOR ver for PATCH ver

* Enforce only major version in JSON model schema

* Bump version to 1.1.0-SNAPSHOT
@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants