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

Unpack namedtuple #9361

Merged
merged 4 commits into from
Aug 10, 2022
Merged

Unpack namedtuple #9361

merged 4 commits into from
Aug 10, 2022

Conversation

hendrikmakait
Copy link
Member

@hendrikmakait hendrikmakait commented Aug 8, 2022

This PR adds support for unpacking namedtuples in unpack_collections and to_task_graph.

  • Closes #xxxx
  • Tests added / passed
  • Passes pre-commit run --all-files

@hendrikmakait
Copy link
Member Author

CI flake: #8795

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.

Thanks @hendrikmakait, I think this is a nice feature. Can we add similar functionality to dask.base.unpack_collections?

dask/delayed.py Outdated Show resolved Hide resolved
@hendrikmakait hendrikmakait self-assigned this Aug 10, 2022
@hendrikmakait hendrikmakait marked this pull request as ready for review August 10, 2022 12:50
@hendrikmakait
Copy link
Member Author

@ian-r-rose, I adjusted dask.base.unpack_collections accordingly, this PR is now ready for another review.

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.

LGTM, thanks @hendrikmakait!


Note: This function checks for the existence of the methods and
attributes that make up the namedtuple API, so it will return True
IFF obj's type implements that API.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I don't think iff is accurate here, I could easily create an object that has these attributes and is not a namedtuple.

Copy link
Member Author

Choose a reason for hiding this comment

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

This might be arguing over semantic details but in that case your object implements the namedtuple API. I actually tried to word this carefully to NOT say that the object is a namedtuple. LMK what you think :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Definitely semantic, I don't expect it to come up. But there's no guarantee that these attributes do the same thing as the namedtuple APIs.

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.

None yet

3 participants