-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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-package] add type hints on some functions in basic.py #5362
Conversation
Thanks very much @makquel ! I just updated the title and description of this PR. In the future, when you're working on something that has already been discussed in another place, please link to that discussion so maintainers understand why you're proposing these changes. For example, I've added "Contributes to #3756". For pull request titles in this project, please make them the type of entry you'd expect to find in release notes, not a branch name. In this project, pull request titles become release note items. See https://github.com/microsoft/LightGBM/releases/tag/v3.3.1 for an example. |
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.
Thanks @makquel !
This pull request is not yet ready to review. It looks like maybe you've run black
over python-package/lightgbm/basic.py
, which has resulted in significant unrelated changes.
This project does not use black
. Please revert all of those changes, so that the only thing left in this pull request are the changes related to type hinting.
Thanks, @jameslamb for the code review, I've just reverted black format changes, removed the pre-commit file, and finished the proposed scope. |
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.
Thanks very much. Please see my next set of suggestions.
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.
Looks good, thanks very much for doing this!
I'll merge this if the CI jobs run successfully. If you're open to it, we'd love more help from you on #3756.
hmmm I'm not sure what's happening, but Azure DevOps is not making progress on running the Linux jobs for this PR. It's been stuck in this state for several hours, despite there not being any other jobs running: I don't see any warnings in the logs (build link). @shiyu1994 can you please check this? Maybe you received an email about a problem with our account, like hitting service limits? |
@jameslamb I'll probably work on the 3867 issue. |
@makquel could you please try pushing an empty commit to this branch? git commit --allow-empty -m "empty commit"
git push origin feature/static-typing Maybe this Azure CI build got into a weird state that can't be resolved by me clicking "retry". Sorry for the extra work. |
All subsequent PR after this one did not trigger the running of CI tasks on Linux machine. But I did not receive any email about this issue from Azure DevOps. I'm checking the status of the Linux agent. |
ah yeah you're right, I can see that builds from other PRs are also stuck in queued. Thanks for looking into it! I don't have access to see or manage the agents in the |
oh yikes! Ok thanks for looking into that @shiyu1994 . I hope they can give you admin access and register your email address as the owner, so we'll be notified of such things in the future before they expire. |
@shiyu1994 could you please check your email and let me know if you got any response about how to fix Azure DevOps? I'd love to merge some of the open, approved PRs over the weekend if possible, but don't want to do that if we can't test them in CI. |
@jameslamb Sorry but there's no response so far. I think we may reopen a virtual machine in our Azure Subscription to replace the existing Linux machine for CI test in Azure DevOps. |
I'll fix this within this weekend. It is blocking the merging of all the PRs. |
ok thanks very much for checking! |
I've created #5387 to track the CI issue. @shiyu1994 let's please continue the conversation there. |
Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
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. |
Contributes to #3756.
Update static type for the following functions that belong to the basic package: