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

Use the new HLG pack/unpack API in Dask #4489

Merged
merged 8 commits into from
Feb 26, 2021

Conversation

madsbk
Copy link
Contributor

@madsbk madsbk commented Feb 8, 2021

This PR makes Distributed use the new high level graph pack/unpack API in Dask: dask/dask#7179

  • Tests added / passed
  • Passes black distributed / flake8 distributed

@madsbk madsbk changed the title Use the HLG pack/unpack from Dask Use the new HLG pack/unpack API in Dask Feb 8, 2021
Copy link
Collaborator

@ian-r-rose ian-r-rose left a comment

Choose a reason for hiding this comment

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

👍

@madsbk madsbk force-pushed the hlg_pack_move_to_dask branch from 349e26e to 2550bee Compare February 19, 2021 09:21
Copy link
Member

@jrbourbeau jrbourbeau 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 your work here @madsbk! I pushed an update to run CI against dask/dask#7179 to ensure that the distributed test suite passes too.

We'll also want to update the minimum supported version of dask to make sure HighLevelGraphs have the new __dask_distributed_(un)pack__ API. Historically what we've done in this case is update distributeds requirements.txt to point to the master branch of dask and add a TODO comment to update to the latest dask release prior to releasing distributed

@madsbk madsbk force-pushed the hlg_pack_move_to_dask branch from d5718f9 to b6bb295 Compare February 25, 2021 13:54
@madsbk
Copy link
Contributor Author

madsbk commented Feb 25, 2021

Thanks @jrbourbeau, both PRs should work now. I have added some tests of the issue in Dask and renamed anno => annotations here.

Some of the CI runs fails in test_register_backend_entrypoint, maybe related to the CI pointing to Dask PR dask/dask#7179 ?

@jrbourbeau
Copy link
Member

Some of the CI runs fails in test_register_backend_entrypoint, maybe related to the CI pointing to Dask PR dask/dask#7179 ?

Yeah you're correct, that failure is be related to pointing CI to your branch. Since our versioning system, versioneer, uses commit tags to determine the package version, and your branch most recent tag is 1.2.2, we're not meeting some minimum requirement in the dask version that test relies on.

pkg_resources.VersionConflict: (dask 1.2.2+1025.g9d91e6a0 (/usr/share/miniconda3/envs/dask-distributed/lib/python3.6/site-packages), Requirement.parse('dask>=2021.02.0'))

This is okay as it's only an artifact of pointing to a fork of dask

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @madsbk! This LGTM. See my comment at dask/dask#7179 (review) about waiting a day to merge this PR

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