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

[DNM] Scatter shuffle proof-of-concept #5473

Closed
wants to merge 17 commits into from

Commits on Oct 27, 2021

  1. Consistent worker Client instance in get_client

    Fixes dask#4959
    
    `get_client` was calling the private `Worker._get_client` method when it ran within a task. `_get_client` should really have been called `_make_client`, since it created a new client every time. The simplest correct thing to do instead would have been to use the `Worker.client` property, which caches this instance.
    
    In order to pass the `timeout` parameter through though, I changed `Worker.get_client` to actually match its docstring and always return the same instance.
    gjoseph92 committed Oct 27, 2021
    Configuration menu
    Copy the full SHA
    d895141 View commit details
    Browse the repository at this point in the history
  2. Check for Futures from the wrong Client in gather

    If you accidentally pass Futures created by a different Client into `Client.gather`, you'll get a `CancelledError`. This is confusing and misleading.
    
    An explicit check for this would have made discovering dask#5466 much easier. And since there are probably plenty of other race conditions regarding default clients in multiple threads, hopefully a better error message will save someone else time in the future too.
    gjoseph92 committed Oct 27, 2021
    Configuration menu
    Copy the full SHA
    5e8fcd4 View commit details
    Browse the repository at this point in the history
  3. Configuration menu
    Copy the full SHA
    0cd61df View commit details
    Browse the repository at this point in the history
  4. Set current client in worker while deserializing dependencies

    This is probably a good idea in general (xref dask#4959), but it particularly helps with performance deserializing Futures, which have a fastpath through `Client.current` that bypasses a number of unnecessarily slow things that `get_client` does before it checks `Client.current`.
    gjoseph92 committed Oct 27, 2021
    Configuration menu
    Copy the full SHA
    30af07e View commit details
    Browse the repository at this point in the history
  5. Don't report back locally-scattered keys

    When a key is scattered from a task and written directly to worker storage, the Client immediately sets the Future's state to `"finished"`. There's no need for the scheduler to also tell the client that that key is finished; it already knows. This saves a bit of scheduler time and a comms roundtrip.
    gjoseph92 committed Oct 27, 2021
    Configuration menu
    Copy the full SHA
    3399830 View commit details
    Browse the repository at this point in the history
  6. Configuration menu
    Copy the full SHA
    555be9b View commit details
    Browse the repository at this point in the history
  7. Configuration menu
    Copy the full SHA
    8b2d7b0 View commit details
    Browse the repository at this point in the history
  8. faster names to key_split

    gjoseph92 committed Oct 27, 2021
    Configuration menu
    Copy the full SHA
    1898673 View commit details
    Browse the repository at this point in the history
  9. Preserve contextvars during comm offload

    Helps with setting the current client in worker while deserializing. Implementation referenced from python/cpython#9688
    gjoseph92 committed Oct 27, 2021
    Configuration menu
    Copy the full SHA
    90e1c47 View commit details
    Browse the repository at this point in the history
  10. Configuration menu
    Copy the full SHA
    6d9d090 View commit details
    Browse the repository at this point in the history
  11. FIXME remove address coercion in update_data

    This was really slow and probably doesn't matter when the future is coming from a worker. But probably not safe to remove in general?
    gjoseph92 committed Oct 27, 2021
    Configuration menu
    Copy the full SHA
    673ea24 View commit details
    Browse the repository at this point in the history
  12. Configuration menu
    Copy the full SHA
    4369028 View commit details
    Browse the repository at this point in the history
  13. Configuration menu
    Copy the full SHA
    d485eaa View commit details
    Browse the repository at this point in the history
  14. REVERTME no-report cancellation

    We don't need to report back to the client that its key was cancelled. But this shouldn't be exposed and may be wrong.
    gjoseph92 committed Oct 27, 2021
    Configuration menu
    Copy the full SHA
    23b1e65 View commit details
    Browse the repository at this point in the history
  15. REVERTME remove cancel logs

    gjoseph92 committed Oct 27, 2021
    Configuration menu
    Copy the full SHA
    a46c507 View commit details
    Browse the repository at this point in the history
  16. REVERTME don't cancel

    Just want to see how it affects performance
    gjoseph92 committed Oct 27, 2021
    Configuration menu
    Copy the full SHA
    78c337f View commit details
    Browse the repository at this point in the history
  17. HACK don't inform on deserialized keys

    Hoping this speeds up the transfer of Futures; makes no sense in general though.
    gjoseph92 committed Oct 27, 2021
    Configuration menu
    Copy the full SHA
    aefe78a View commit details
    Browse the repository at this point in the history