Skip to content

Conversation

@Matt711
Copy link
Member

@Matt711 Matt711 commented May 19, 2022

This PR replaces the call to the RPC object in favor of the new scheduler HTTP API.
Related Issue: #5935
Related PR: #6270

Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

Nice to see this! A couple of comments.

Also, this does depend on an unreleased feature in distributed so we need to wait for a release before we can merge this and bump the minimum required distributed version.

Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

Thanks for getting these fixes in @Matt711.

I tested this locally and it didn't quite seem to work so I pushed another commit with a fix.

This should be good to merge once dask/community/issues/245 is done and we can set distributed>=2022.05.1 in requirements.txt.

@mmccarty
Copy link
Member

Also, this does depend on an unreleased feature in distributed so we need to wait for a release before we can merge this and bump the minimum required distributed version.

Did we get an answer to this?

@jacobtomlinson
Copy link
Member

@mmccarty see #499 (review), just waiting on the next release now before we can merge 😁

@jacobtomlinson
Copy link
Member

Given that the HTTP API is still disabled by default in the latest release I've implemented some fallback options:

  • If the operator fails to use the HTTP API it will try the RPC
  • If the operator fails to use the RPC it will fall back to dumb last-in-first-out scaling down

This should unblock us to merge now and also make it easier to support multiple Dask versions in the future.

@jacobtomlinson jacobtomlinson merged commit a55083a into dask:main May 27, 2022
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.

3 participants