-
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 install option to enable printing of time costs #5497
[python-package] add install option to enable printing of time costs #5497
Conversation
9a68944
to
086be2f
Compare
086be2f
to
d69b063
Compare
d69b063
to
2bfde86
Compare
@jameslamb the dask tests seem to be getting stuck and we get timeouts on the jobs, (Linux regular this PR, several Linux jobs in another PR, CUDA jobs in that PR). Should we open an issue for this? I'll try to replicate the problem later. |
@jmoralez Could I do something to help? I can re-push something to launch the CI jobs again but maybe some problems are remaining? Let me know :) |
@Remy-Luciani LightGBM's CI is facing some significant issues right now (see #5509 and #5510), and we won't be able to come back to this PR until they've been addressed. Sorry for the inconvenience. |
@jmoralez to answer your question directly.... yes I think we should open a separate issue for the Dask timeout thing, and I'd be grateful if you have time to investigate it. (#5510 (comment)) |
hey @Remy-Luciani , happy to say that we've fixed LightGBM's CI issues (#5510)! Thanks very much for your patience. If you can please merge the latest changes on |
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.
One minor comment, otherwise this looks good to me and I support adding this option to the Python package.
But I'd also like @jmoralez or @StrikerRUS to approve before this is merged.
2bfde86
to
36f3a35
Compare
According to the [install guide](https://lightgbm.readthedocs.io/en/latest/Installation-Guide.html): > Users who want to perform benchmarking can make LightGBM output time costs for different internal routines by adding -DUSE_TIMETAG=ON to CMake flags. This feature was unfortunately not available when installing LightGBM from the Python package. This commit fixes this.
36f3a35
to
6b0b35a
Compare
All feedbacks have been taken and the branch is up to date with master! :) Let me know if there are another feedbacks |
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 for the changes! looks good to me.
closing and re-opening to try to fix this flaky That seemed to fix it last time we saw something similar: #5160 (comment) |
@microsoft-github-policy-service agree [company="Theodo"] |
I'm so sorry @Remy-Luciani . These comments and commands look new. The CLA bot is something controlled by Microsoft at an organization level, not something that we (the maintainers of LightGBM) control or configure 😬 . So that's bot's comments are as confusing to me as they probably are to you 😭 |
No worries. I don't know if you can make some feedback to someone at Microsoft since the first message was misleading by adding brackets to the command syntax. 😛 |
@microsoft-github-policy-service agree company="Theodo" |
Ooooh actually brackets meant optional argument... Well, OK... So I think the first message should provide examples like the second one. |
Thanks again @Remy-Luciani ! Hope whatever benchmarking you're working on (that led you to this feature) goes well! We'd love to have you come back and contribute any time 👋 |
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. |
According to the install guide:
This feature was unfortunately not available when installing LightGBM from the Python package. This PR attempts to fix this. :)