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

Specify HLG layer name during delayed object creation in fit #898

Merged
merged 3 commits into from
Jan 5, 2022

Conversation

ayushdg
Copy link
Contributor

@ayushdg ayushdg commented Jan 5, 2022

Fixes #895.

Updated the fit method in the _partial module to specify the layer during delayed object creation as required after dask/dask#8452.

Also added a commit unrelated to the issue replacing the usage of distutils.version.LooseVersion with packaging.version.parse as a side effect of needing it while testing locally. Happy to remove it or open a separate pr for those changes if preferred.

@@ -20,7 +20,7 @@

# Copied from scikit-learn/sklearn/utils/fixes.py, can be removed once we drop
# support for scikit-learn < 0.18.1 or numpy < 1.12.0.
if LooseVersion(np.__version__) < "1.12.0":
if packaging.version.parse(np.__version__) < packaging.version.parse("1.12.0"):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These individual checks can probably be moved to the _compat module similar to how it's done in other parts of the codebase.

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, given that today the minimum supported NumPy version for Dask is 1.18, we could probably just bump the minimum supported version for Dask-ML too (though let's handle that in a separate PR)

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 @ayushdg. @gjoseph92 would you mind taking a quick look at the changes here?

Copy link

@gjoseph92 gjoseph92 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 fixing @ayushdg, LGTM. Sorry about the breaking change 😞

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 @ayushdg @gjoseph92 -- just pushed an empty commit to trigger a CI build against the main branch of dask (so we should now be exercising both the if DASK_2022_01_0 and else code paths below)

@@ -20,7 +20,7 @@

# Copied from scikit-learn/sklearn/utils/fixes.py, can be removed once we drop
# support for scikit-learn < 0.18.1 or numpy < 1.12.0.
if LooseVersion(np.__version__) < "1.12.0":
if packaging.version.parse(np.__version__) < packaging.version.parse("1.12.0"):
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, given that today the minimum supported NumPy version for Dask is 1.18, we could probably just bump the minimum supported version for Dask-ML too (though let's handle that in a separate PR)

@jrbourbeau
Copy link
Member

Whoops, forgot the upstream build is failing for unrelated reasons : / Just confirmed locally that tests/test_incremental.py::test_incremental_sparse_inputs passes with the current dask main branch 👍

@jrbourbeau jrbourbeau changed the title Specify HLG layer name during delayed object creation in fit (Upstream Dask compatibility) Specify HLG layer name during delayed object creation in fit Jan 5, 2022
@jrbourbeau jrbourbeau merged commit 5466bec into dask:main Jan 5, 2022
This was referenced Jan 20, 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.

Test failures with latest dask from main
3 participants