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

[tests][dask] Add voting_parallel algorithm in tests (fixes #3834) #4088

Merged
merged 10 commits into from
Apr 1, 2021

Conversation

jmoralez
Copy link
Collaborator

@jmoralez jmoralez commented Mar 20, 2021

This includes the voting_parallel tree_learner for test_regressor, test_classifier and test_ranker in the tests for the dask module and removes the warning about experimental support that was previously triggered (because it wasn't tested).

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.

Thanks for doing this! I'm glad it was as easy as just changing the tests. I have a few suggested changes for the tests.

tests/python_package_test/test_dask.py Outdated Show resolved Hide resolved
tests/python_package_test/test_dask.py Outdated Show resolved Hide resolved
@jmoralez
Copy link
Collaborator Author

Hi, James. Do you have any suggestions on what to work on next?

@jmoralez
Copy link
Collaborator Author

I noticed a failing test due to graphviz I believe. This time it failed in Linux_latest_gpu_source. Here are the errors:

Warning: Could not load "/home/AzDevOps_azpcontainer/miniconda/envs/test-env/lib/graphviz/libgvplugin_pango.so.6" - It was found, so perhaps one of its dependents was not.  Try ldd.
Warning: Could not load "/home/AzDevOps_azpcontainer/miniconda/envs/test-env/lib/graphviz/libgvplugin_gd.so.6" - It was found, so perhaps one of its dependents was not.  Try ldd.

I believe this test was also failing on some builds before I merged the latest master.

The other one is related to #4095 (comment).

@jameslamb
Copy link
Collaborator

Hi, James. Do you have any suggestions on what to work on next?

are you mostly interested in Dask? If so, this issue would be a good one to pick up next: #3896. If not, let me know and I can recommend something else.

@jameslamb jameslamb self-requested a review March 28, 2021 04:21
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.

Thanks for making that most recent round of changes. I don't have any other suggestions. Could you please update to the latest master?

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.

Great to know that voting parallel is working without any modifications!
Please consider checking a few my minor comments below:

tests/python_package_test/test_dask.py Show resolved Hide resolved
tests/python_package_test/test_dask.py Outdated Show resolved Hide resolved
tests/python_package_test/test_dask.py Outdated Show resolved Hide resolved
@jmoralez
Copy link
Collaborator Author

jmoralez commented Mar 31, 2021

@jameslamb do you know what's the advantage of using the client fixture? (Apart from avoiding the client construction). I just tried running test_regressor by initializing the client at the top and the time goes from 137s down to 98s.
Here's the profile with the fixture:
Screenshot from 2021-03-30 20-22-14
And without:
image
So basically with the fixture 70% of the time is spent in setting up and tearing down the cluster. Without it only 60% of the time is spent there and seems to be faster.

Followup
I've been investigating more and found dask/distributed#3540, which led me to the implementation by seanlaw, here: https://github.com/TDAmeritrade/stumpy/blob/68092c931610db725f2c74b6a5155868666eb14f/tests/test_mstumped.py#L10-L14 and holy cow that's fast. Using that test_regressor runs in 18 seconds. Would you support a PR adding this? It'd basically be replacing the client fixture with the dask_cluster fixture and instantiating a client from it in every test. When the client gets closed it's memory gets released so I believe this is the same as what we have right now, every test gets a fresh client but it's significantly faster because we're not creating a cluster everytime.

@StrikerRUS
Copy link
Collaborator

@jmoralez

do you know what's the advantage of using the client fixture?

Awesome research! Could you please copy-paste your comment here: #3829 (comment). I believe that place is the best one to continue the discussion.

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.

LGTM!

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.

thanks for this!

@jameslamb jameslamb merged commit d517ba1 into microsoft:master Apr 1, 2021
@jmoralez jmoralez deleted the test-voting_parallel branch April 3, 2021 03:54
@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.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants