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

test_global_option fails #627

Closed
jakirkham opened this issue May 26, 2021 · 3 comments · Fixed by #738
Closed

test_global_option fails #627

jakirkham opened this issue May 26, 2021 · 3 comments · Fixed by #738

Comments

@jakirkham
Copy link
Member

This test recently started failing in PR ( #623 ). Marked as xfail, but would be good to investigate what's going on and fix if possible

08:54:28 =================================== FAILURES ===================================
08:54:28 ______________________________ test_global_option ______________________________
08:54:28 
08:54:28     def test_global_option():
08:54:28         for seg_size in ["2K", "1M", "2M"]:
08:54:28             p = mp.Process(target=_test_global_option, args=(seg_size,))
08:54:28             p.start()
08:54:28             p.join()
08:54:28 >           assert not p.exitcode
08:54:28 E           AssertionError: assert not 1
08:54:28 E            +  where 1 = <SpawnProcess name='SpawnProcess-89' pid=13771 parent=10006 stopped exitcode=1>.exitcode
08:54:28 
08:54:28 dask_cuda/tests/test_ucx_options.py:58: AssertionError

https://gpuci.gpuopenanalytics.com/job/rapidsai/job/gpuci-v0.20/job/dask-cuda/job/prb/job/dask-cuda-gpu-test/CUDA=11.2,GPU_LABEL=gpu,OS=ubuntu18.04,PYTHON=3.8/104/console

@jakirkham
Copy link
Member Author

cc @pentschev @madsbk

@pentschev
Copy link
Member

pentschev commented May 26, 2021

The reason for this is dask/distributed#4850 . I thought we were not allowing UCP variables via UCX configs, such as UCX_SEG_SIZE can be passed as DASK_UCX__SEG_SIZE. This can be dangerous as we also have Dask internal variables in the same namespace, such as DASK_UCX__NVLINK, which I believe are fine today but we can't guarantee that in the future UCX won't add an environment variable that would clash with some of those we define in Dask today, thus this could cause issues in the future.

I'm not aware of any use today where we need to pass variables through the Dask environment variable namespace, but if we need that we should probably have another level for those, such as DASK_UCX__UCP_SEG_SIZE or something similar, otherwise I would suggest we remove that capability entirely, making the only possible way to pass such variables through the UCX_SEG_SIZE, for example.

Any opinions on the above @madsbk @quasiben @jakirkham ?

@jakirkham
Copy link
Member Author

Agree with removal. We can always revisit when there is a need

@rapids-bot rapids-bot bot closed this as completed in #738 Sep 24, 2021
rapids-bot bot pushed a commit that referenced this issue Sep 24, 2021
Support for setting UCX global options was dropped in
dask/distributed#4850, as the conflict of Dask
configs and UCX configs can be dangerous since both used to live in the
same namespace. Setting global UCX options can still be done via
environment variables, such as `UCX_*`, and is the preferred method now.

Fixes #627

Authors:
  - Peter Andreas Entschev (https://github.com/pentschev)

Approvers:
  - Mads R. B. Kristensen (https://github.com/madsbk)

URL: #738
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 a pull request may close this issue.

2 participants