Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[dask] allow tight control over ports #3994
[dask] allow tight control over ports #3994
Changes from 14 commits
58beb97
a79b13d
30828d3
6f538ea
5c36083
ff6f50b
26bbc48
bbe0d10
c352eb5
58c6470
69dea53
0cc9d67
52e0c39
f00b3a7
25462ea
5bdf5be
deeab63
eeb75f5
0c81f60
e1a4d4d
da1c0ea
e36b169
dcae2d0
d507210
e474c37
1e9244d
c36ec28
4fc9f70
040ad1f
b3c8a2c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This totally just my opinion, but
"machines": a list of all machines in the cluster, plus a port to communicate..."
is kind of circular - the second use of "machines" is referring to IP addresses, but the original use ofmachines
(IMO) is an IP:port comboThere 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.
oh yeah I probably should not use the word "machines" again in the description haha, thank you
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.
ok updated in 25462ea, thanks for catching this
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.
I believe it is important to notify users about this behavior.
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, I accepted this suggestion but I think now that we should only apply it to
num_machines
, notnum_threads
.This results in a warning that users cannot suppress.
Caused by the fact that
n_jobs
is an alias ofnum_threads
LightGBM/python-package/lightgbm/dask.py
Line 489 in 646267d
LightGBM/python-package/lightgbm/sklearn.py
Line 516 in 646267d
I believe that every warning should be something that can be changed by user code changes. Otherwise, we're just adding noise to logs that might cause people to start filtering out ALL warnings.
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.
I think it is important to notify users about
num_threads
as well before implementing #3714. Silently ignore parameter is more serious problem compared to unfixable warning, I believe.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.
I disagree in this specific case about the meaning of "ignore", since this is a parameter default and not something explicitly passed in. However, since
num_threads
isn't directly related to the purpose of this PR and since I don't want to delay this PR too long because I'd like to merge #3823 soon after it, I'll leave this warning in for now and propose another PR in a few days where we could discuss it further.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.
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 you explain why you think we should include a newline?
I'm concerned that in logs, it will look like an exception with only the text before the newline followed by a separate print statement.
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.
OK, it is up to you. Feel free to revert new line. I personally don't like long line warnings/errors.
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.
alright I'm not going to accept this suggestion then if it's just a matter a matter of personal preference.
I've had problems in the past with external logs-management systems and log messages that have newline characters. You can read about that general problem at https://www.datadoghq.com/blog/multiline-logging-guide/#the-multi-line-logging-problem if you're interested.
Long log messages will also be wrapped automatically in Jupyter notebooks
and in
python
REPLs