-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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] Drop Python 2 support #3581
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look great to me, thanks! I left one suggestion for an unrelated change to be moved.
This reverts commit 4ff6ffc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really well-done! I think you got everything that's dependent on Python 2. I can't think of anything else.
Tagging @aldanor @synapticarbors as maintainers of https://github.com/conda-forge/lightgbm-feedstock, to let them know that the next LightGBM minor release (3.2.0) will drop Python 2 support.
One more thing is string formatting unification ( |
|
||
|
||
# DeprecationWarning is not shown by default, so let's create our own with higher level | ||
class LGBMDeprecationWarning(UserWarning): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could there be some user code downstream catching/filtering this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, could you elaborate on what code you'd expect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably ok, I don't think anything needs to be done :) What I meant is that you could say it's technically a breaking change since there can be user code like below:
from lightgbm.compat import LGBBDeprecationWarning
import warnings
warnings.simplefilter('ignore', LGBMDeprecationWarning)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, OK, got it!
Yeah, after this PR is merged, one should change
from lightgbm.compat import LGBMDeprecationWarning
to
from lightgbm.basic import LGBMDeprecationWarning
But we are planing to include this PR in 3.2.0
(not 3.1.1
patch) version and it has breaking
label in the changelog. So, I believe it is OK...
@guolinke Are you OK with this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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. |
Do not include in any patch
3.1.x
release. Should be merged only in3.2.0
version.