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

[python] enforce a floor of Python 3.6 (fixes #5004) #5006

Merged
merged 1 commit into from
Feb 14, 2022
Merged

[python] enforce a floor of Python 3.6 (fixes #5004) #5006

merged 1 commit into from
Feb 14, 2022

Conversation

PyVCEchecker
Copy link
Contributor

@PyVCEchecker PyVCEchecker commented Feb 14, 2022

Fixes #5004.
Replaces #5005.

Add python_requires='>=3.6' for setup().

@jameslamb jameslamb changed the title [python] add "python_requires" keyword argument for setup() (targeting master branch) [python] enforce a floor of Python 3.6 Feb 14, 2022
@jameslamb jameslamb changed the title [python] enforce a floor of Python 3.6 [python] enforce a floor of Python 3.6 (fixes #5004) Feb 14, 2022
Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Makes sense to me! Thanks for doing this!

I have two small notes for when you make future open source contributions.

  1. I recommend creating a new branch from your fork, instead of directly committing to master there. That makes it possible to work on multiple pull requests at once, and ensures that there will never be commits on master of your fork that don't exist on the upstream project.
  2. I've added (fixes #5004) to the title of this PR, so that the corresponding issue will be automatically closed when this is merged.

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

@PyVCEchecker Thanks a lot for this contribution, LGTM!

@StrikerRUS StrikerRUS merged commit 800e1e2 into microsoft:master Feb 14, 2022
@jmoralez
Copy link
Collaborator

@StrikerRUS @jameslamb given that Python 3.6 has reached its EOL shouldn't we set the floor to 3.7 for future releases?

@StrikerRUS
Copy link
Collaborator

StrikerRUS commented Feb 14, 2022

@jmoralez My position is that LightGBM should be as much permissive as possible. This is by the way not only my personal position, but historically supported general policy in this repo. Absence of artificial restrictions allows to run LightGBM in a wide range of different environments and hence increase its' popularity among users. As an example, I can name Dockerized Linux artifacts building in Ubuntu 14, CI jobs with Visual Studio 2015 and CUDA 9.0. There are a lot of cases of restricted environments which users cannot update due to different reasons. I believe it's important to let users at least try to run LightGBM in such environments.

@jameslamb
Copy link
Collaborator

My position is that LightGBM should be as much permissive as possible.

I 100% agree with @StrikerRUS on this. Libraries like LightGBM should use as few ceilings / floors on language version and dependencies as possible.

In this case, I supported a floor of python>=3.6 because with the use of type hints and f-strings throughout the package, older versions of Python don't even recognize the package as syntactically-valid Python code.

In case it helps describe my personal opinion on this better, I summarized some thoughts on this for library dependencies (not language version) over here: snowflakedb/snowflake-connector-python#604 (comment)

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants