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.

Sign up for GitHub

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 elif in Dask deserialize check #4537

Merged
merged 1 commit into from
Mar 17, 2020
Merged

Use elif in Dask deserialize check #4537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@
- PR #4406 Fix sorted merge issue with null values and ascending=False
- PR #4445 Fix string issue for parquet reader and support `keep_index` for `scatter_to_tables`
- PR #4423 Tighten up Dask serialization checks
- PR #4537 Use `elif` in Dask deserialize check
- PR #4438 Fix repl-template error for replace_with_backrefs
- PR #4434 Fix join_strings logic with all-null strings and non-null narep
- PR #4465 Fix use_pandas_index having no effect in libcudf++ parquet reader
Expand Down
2 changes: 1 addition & 1 deletion python/cudf/cudf/comm/serialize.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ def deserialize_cudf_object(header, frames):
# some frames are empty -- meta/empty partitions/etc
if len(f) > 0:
assert hasattr(f, "__cuda_array_interface__")
if header["serializer"] == "dask":
elif header["serializer"] == "dask":
Copy link
Collaborator

@kkraus14 kkraus14 Mar 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Moving discussion to here. I understand that this should be elif, but how could both conditions evaluate to True here regardless?

Copy link
Member Author

Choose a reason for hiding this comment

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

It wouldn't be possible.

Copy link
Member

Choose a reason for hiding this comment

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

@kkraus14 are you suggesting this should be else and not elif ?

Copy link
Collaborator

@kkraus14 kkraus14 Mar 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

No, I wasn't aware that header['serializer'] could be a list of elements, but I'm still not clear on how both conditions could trigger here because it's ==.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that is incorrect. header["serializer"] should only be a single str with a serializer named.

Ben was mentioning that multiple serializers could be used to transform the data (and its various components), which is different from the value seen here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jakirkham So then is this a fix for something or just preventing an additional conditional check?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just preventing an additional check. Something I stumbled across recently when looking at the serialization code.

frames = [memoryview(f) for f in frames]

cudf_typ = pickle.loads(header["type-serialized"])
Expand Down