Skip to content

Relax dask.dataframe assert_eq type checks#9989

Merged
rjzamora merged 3 commits intodask:mainfrom
mrocklin:relax-assert-eq-typechecks
Feb 22, 2023
Merged

Relax dask.dataframe assert_eq type checks#9989
rjzamora merged 3 commits intodask:mainfrom
mrocklin:relax-assert-eq-typechecks

Conversation

@mrocklin
Copy link
Copy Markdown
Member

This allows other dask.dataframe-like projects to leverage assert_eq and shouldn't negatitvely affect the power of current tests within dask.dataframe.

This allows other dask.dataframe-like projects to leverage assert_eq and
shouldn't negatitvely affect the power of current tests within
dask.dataframe.
@mrocklin
Copy link
Copy Markdown
Member Author

cc @rjzamora

rjzamora
rjzamora previously approved these changes Feb 22, 2023
Copy link
Copy Markdown
Member

@rjzamora rjzamora left a comment

Choose a reason for hiding this comment

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

This make sense to me. I was initially a bit uncomfortable until I realized that the modified lines are within an if hasattr(dsk, "__dask_graph__") block, which is equivalent to asserting that is_dask_collection(dsk) is True. Therefore, I agree that we shouldn't be hindering existing test coverage.

if result is None:
result = dsk.compute(scheduler=scheduler)
if isinstance(dsk, dd.Index):
if is_index_like(dsk._meta):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like dask_cudf.Index does not satisfy is_index_like when the underlying meta is a MultiIndex, because a cudf.MultiIndex does not have a dtype attribute (while pd.MultiIndex does) :/

I'm sure this can changed in cudf, but we could also do something like if isinstance(dsk, dd.Index) or is_index_like(dsk._meta).

@rjzamora rjzamora dismissed their stale review February 22, 2023 04:12

Need to avoid dask_cudf breakage

@rjzamora rjzamora merged commit c182a29 into dask:main Feb 22, 2023
@mrocklin mrocklin deleted the relax-assert-eq-typechecks branch February 24, 2023 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants