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

Improve CUDAWorker scheduler-address parsing and __init__ #397

Conversation

necaris
Copy link
Contributor

@necaris necaris commented Sep 10, 2020

Allows scheduler argument to be optional, and uses dask.config to check the scheduler-address rather than dictionary lookup (this allows e.g. environment variables like DASK_SCHEDULER_ADDRESS to be correctly parsed)

This means that e.g. setting the environment variable
`DASK_SCHEDULER_ADDRESS` will work
@necaris necaris requested a review from a team as a code owner September 10, 2020 18:06
@GPUtester
Copy link
Contributor

Can one of the admins verify this patch?

2 similar comments
@GPUtester
Copy link
Contributor

Can one of the admins verify this patch?

@GPUtester
Copy link
Contributor

Can one of the admins verify this patch?

@jrbourbeau
Copy link
Contributor

For context, setting the DASK_SCHEDULER_ADDRESS environment variable puts scheduler_address (note the underscore) into Dask's config system. We generally try to use dask.config.get when interacting with config values as it treats underscores and hyphens the same (xref dask/distributed#2676)

The scheduler=None change is so CUDAWorker can better handle other ways in which the scheduler address can be specified (e.g. scheduler_file or scheduler-address configuration option). There's additional logic here

if not scheduler and not scheduler_file and "scheduler-address" not in config:
raise ValueError(
"Need to provide scheduler address like\n"
"dask-worker SCHEDULER_ADDRESS:8786"
)

that will still catch cases when no scheduler address is specified

@quasiben
Copy link
Member

ok to test

@quasiben
Copy link
Member

Thank you @necaris and the comment @jrbourbeau !

dask_cuda/cuda_worker.py Outdated Show resolved Hide resolved
@quasiben
Copy link
Member

@necaris i made this change on your behalf -- hope you don't mind

Copy link
Member

@quasiben quasiben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again @necaris

@necaris
Copy link
Contributor Author

necaris commented Sep 10, 2020

@quasiben very welcome!

@codecov-commenter
Copy link

codecov-commenter commented Sep 10, 2020

Codecov Report

Merging #397 into branch-0.16 will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.16     #397      +/-   ##
===============================================
+ Coverage        60.07%   60.13%   +0.05%     
===============================================
  Files               17       17              
  Lines             1345     1342       -3     
===============================================
- Hits               808      807       -1     
+ Misses             537      535       -2     
Impacted Files Coverage Δ
dask_cuda/cuda_worker.py 72.28% <100.00%> (+0.52%) ⬆️
dask_cuda/local_cuda_cluster.py 82.95% <100.00%> (+0.93%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3811951...d536ad5. Read the comment docs.

@quasiben quasiben merged commit 2fa52fe into rapidsai:branch-0.16 Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants