Streamlining the destructors in the UCX API

[madsbk]

This PR make sure that all calls to the UCX is using valid UCX handles and streamlines the destruction of UCX handles.

[madsbk]

cc. @beckernick @VibhuJawa

[quasiben]

Running with this PR i am seeing the following error:

Task exception was never retrieved
future: <Task finished coro=<_listener_handler() done, defined at /gpfs/fs1/bzaitlen/miniconda3/envs/20200417/lib/python3.7/site-packages/ucp/core.py:116> exception=ValueError('Both peers must set guarantee_msg_order identically')>
Traceback (most recent call last):
  File "/gpfs/fs1/bzaitlen/miniconda3/envs/20200417/lib/python3.7/site-packages/ucp/core.py", line 127, in _listener_handler
    guarantee_msg_order=guarantee_msg_order,
  File "/gpfs/fs1/bzaitlen/miniconda3/envs/20200417/lib/python3.7/site-packages/ucp/core.py", line 56, in exchange_peer_info
    raise ValueError("Both peers must set guarantee_msg_order identically")
ValueError: Both peers must set guarantee_msg_order identically

This is in the middle of a workflow

[quasiben]

Apologies, this is with an IB stress test with 4 nodes: https://gist.github.com/quasiben/73cf6d7c2131fe41370014ddc21ecb56

Perhaps this is still the bug @pentschev, myself, and others have been tracking

[pentschev]

I've done some testing with this and I have some carefully optimistic news:

1. TCP: not a single hang or crash in about 3 runs;
2. NVLink: not a single hang or crash in about 3 runs;
3. IB: not a single hang or crash in 10+ runs;
4. IB+NVLink: in some 7-8 runs, it hanged all times except for two consecutive ones.

All tests above were on a 4 node/32 GPU cluster.

I'm carefully optimistic because IB was hanging and crashing most of the time and I haven't been able to reproduce that in any runs. The amount of observation is still not enough to ensure our bugs have been fixed. The hangs with IB+NVLink also point to some remaining issues.

Finally, just as the merge operation starts I still the following error several times when new endpoints are getting created to do worker-worker connection:

future: <Task finished name='Task-1578' coro=<_listener_handler() done, defined at /datasets/pentschev/miniconda3/envs/pydbg/lib/python3.8/site-packages/ucp/core.py:116> exception=ValueError('Both peers must set guarantee_msg_order identically')>
    raise ValueError("Both peers must set guarantee_msg_order identically")
ValueError: Both peers must set guarantee_msg_order identically

For @VibhuJawa and @beckernick , please mind the NVLink+IB hang if you're testing. If you have the chance a test with IB only (NVLink disabled) would be useful as well to see whether you experience any hangs or segfaults.

[pentschev]

I intend to do some more testing tomorrow to confirm whether I can't reproduce IB errors anymore.

[jakirkham]

Thanks Mads! Happy to see these cleanup improvements. 😄

Made some suggestion below. Where possible tried to include a code suggestion for simplicity.

[jakirkham]

Can we file an issue about this?

[madsbk]

Added #505

[jakirkham]

Thanks Mads! Looks much nicer 😄

Had a couple follow-up comments on UCXEndpoint so that we can use __cinit__ as is typical.

[pentschev]

I did more testing today and results were very similar to what I observed yesterday. Having NVLink enabled makes the cluster more likely to hang. IB was (almost) hang-free, it did hang once in some 30 runs. I also did various runs with larger data -- multiplying the sizes of the workflow in #402 (comment) by 8 -- which takes roughly 4 minutes to create the data and another 11-12 minutes for the merge to complete, and they all seemed to behave the same as the original size, hanging sometimes with NVLink and passing with IB. Due to the long wait between runs, I did 3 successful runs of each only, where I observed 2 or 3 hangs for NVLink (totalling 5-6 runs in that case).

I am very suspicious still of what happens during exchange_peer_info, it seems that dask-cuda-workers listener's will receive corrupted responses or not receive a response at all (not sure which one of the two is really happening), causing tags and guarantee_msg_order to be invalid. Perhaps this will eventually be fixed by #503, but it isn't the case at the time of writing. To be more specific, the reason I believe the hangs are connected to exchange_peer_info is that hangs happen just a little while after many of the workers raise

ucx-py/ucp/core.py

Line 56 in 37e4445

raise ValueError("Both peers must set guarantee_msg_order identically")

, something that happens at the beginning of the merge task stream when workers are establishing connections to each other.

[quasiben]

How do folks generally feel about merging this PR in given that other work is now dependent on this ?

[jakirkham]

I made a few minor suggestions above that would be nice to see addressed first.

[quasiben]

I made a few minor suggestions above that would be nice to see addressed first.

Definitely agreed -- I wanted to get a sense where folks were at.

[jakirkham]

Yeah after that's addressed I'm +1 on merging.

[pentschev]

I'm +1 on merging this as well.

[madsbk]

am very suspicious still of what happens during exchange_peer_info

@pentschev I think you are on to something: #506 !

[quasiben]

Thank you @Mads and @jakirkham for the reviews

[pentschev]

LGTM, any final comments before we merge @jakirkham ?

[jakirkham]

Thanks Mads for the PR! Also thanks Peter and Ben for the reviews!