Skip to content

Enable parallelism in cudf & dask-cudf pytests - #188

Merged
mike-wendt merged 3 commits into
rapidsai:branch-0.18from
galipremsagar:patch-7
Dec 14, 2020
Merged

Enable parallelism in cudf & dask-cudf pytests#188
mike-wendt merged 3 commits into
rapidsai:branch-0.18from
galipremsagar:patch-7

Conversation

@galipremsagar

@galipremsagar galipremsagar commented Dec 9, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR enables pytest-xdist in cudf and dask-cudf pytests.

@galipremsagar
galipremsagar requested a review from a team as a code owner December 9, 2020 21:49
galipremsagar
galipremsagar commented Dec 9, 2020
View reviewed changes
Comment thread ci/test/cudf.sh
@galipremsagar galipremsagar mentioned this pull request Dec 9, 2020
Merged
@mike-wendt mike-wendt self-assigned this Dec 9, 2020
@mike-wendt mike-wendt added 4 - Needs Reviewer Waiting for reviewer to review or respond 4 - Needs Tests Waiting for test results to make a decision labels Dec 9, 2020
@galipremsagar

galipremsagar commented Dec 9, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I've locally tested pytests with enabling cuda MPS, there isn't any noticeable speed up just by using MPS but however, it would help us with CUDA context switching when there are multiple processes.

@rapidsai/ops-codeowners Could you point where the following set of commands need to be added in this repo? This set of commands except running pytests need to be executed as a root user.

Startup MPS (Execute as root)

export CUDA_VISIBLE_DEVICES="0"
nvidia-smi -i 0 -c EXCLUSIVE_PROCESS
nvidia-cuda-mps-control -d

Run Pytests

...
...

End MPS (Execute as root)

echo quit | nvidia-cuda-mps-control
nvidia-smi -i 0 -c DEFAULT

@galipremsagar galipremsagar changed the title Enable parallelism in cudf pytests Enable parallelism in cudf & dask-cudf pytests Dec 9, 2020
@kkraus14

kkraus14 commented Dec 9, 2020

Copy link
Copy Markdown
Contributor

Can we handle MPS in a follow up? Is there a net benefit without MPS?

@mike-wendt mike-wendt added the DO NOT MERGE Hold off on merging; see PR for details label Dec 9, 2020
@galipremsagar

galipremsagar commented Dec 9, 2020

Copy link
Copy Markdown
Contributor Author

Can we handle MPS in a follow up? Is there a net benefit without MPS?

Sure this can be done as a follow-up and maybe we can add MPS if we see context issues but currently I did not notice any difference in execution times. I've been using parallel pytests without enabling MPS for quite a very long time. So parallel pytests should run properly without enabling MPS too.

@raydouglass

raydouglass commented Dec 9, 2020

Copy link
Copy Markdown
Contributor

I don't think we can enable MPS as running commands as root is not possible inside of the gpuCI docker containers.

@galipremsagar

galipremsagar commented Dec 9, 2020

Copy link
Copy Markdown
Contributor Author

running commands as root is not possible inside of the gpuCI docker containers.

Didn't know that. In that case, we probably go ahead without MPS too.

@mike-wendt

mike-wendt commented Dec 9, 2020

Copy link
Copy Markdown
Contributor

I don't think we can enable MPS as running commands as root is not possible inside of the gpuCI docker containers.

Due to the security concerns this is not an option in gpuCI. If there is a way to do this without root access we can make it work; otherwise, we can only run things are a privileged user to limit system access as these run on a shared environment

@kkraus14

kkraus14 commented Dec 9, 2020

Copy link
Copy Markdown
Contributor

IIRC this is a driver level setting that interacts with the kernel so you can't really run it within a container even with root unless you use the --privileged flag which is an obvious no-no.

Lets move forward without MPS for now and can explore it in the future.

@mike-wendt

mike-wendt commented Dec 14, 2020

Copy link
Copy Markdown
Contributor

rerun tests

@mike-wendt

mike-wendt commented Dec 14, 2020

Copy link
Copy Markdown
Contributor

Blocked until runtime images are fixed

@mike-wendt

mike-wendt commented Dec 14, 2020

Copy link
Copy Markdown
Contributor

rerun tests

1 similar comment
@mike-wendt

mike-wendt commented Dec 14, 2020

Copy link
Copy Markdown
Contributor

rerun tests

@mike-wendt mike-wendt added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 4 - Needs Reviewer Waiting for reviewer to review or respond 4 - Needs Tests Waiting for test results to make a decision DO NOT MERGE Hold off on merging; see PR for details labels Dec 14, 2020
@mike-wendt
mike-wendt merged commit 8882ab4 into rapidsai:branch-0.18 Dec 14, 2020
rapids-bot Bot pushed a commit to NVIDIA/cudf that referenced this pull request Dec 16, 2020
@galipremsagar
Add pytest-xdist to dev environment.yml(#6958)
8c1f01e 
Resolves: #6370 

This PR enables the parallel execution of pytests of `cudf`, `dask_cudf` & `custreamz` in CI. The changes also include adding `pytest-xdist` to dev environments.

With these changes, here is the change in pytest execution times in CI:

| module      | without pytest-xdist | with pytest-xdist(n=6)  |
| ----------- | ----------- | -----------|
| cudf      | 1 hr       |  14 min |
| dask_cudf   |    4 min     | 1 min |
| custreamz  |  6 min |  2 min |


Related Integration changes: rapidsai/integration#188

Authors:
  - galipremsagar <sagarprem75@gmail.com>

Approvers:
  - AJ Schmidt
  - Keith Kraus

URL: #6958
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@mike-wendt mike-wendt mike-wendt approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mike-wendt mike-wendt

Labels

5 - Ready to Merge Testing and reviews complete, ready to merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@galipremsagar @kkraus14 @raydouglass @mike-wendt