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

Support for library specific annotations #464

Merged
merged 4 commits into from
Nov 14, 2022

Conversation

magnatelee
Copy link
Contributor

This PR adds an Annotation class with which client libraries can attach client specific annotations other than the provenance to operations. Here's an example of how an operator name added as an annotation in cuNumeric gets rendered:

image

@magnatelee magnatelee added the category:new-feature PR introduces a new feature and will be classified as such in release notes label Nov 3, 2022
@magnatelee magnatelee requested a review from bryevdv November 3, 2022 03:08
return len(self._entries) == 0

def __getitem__(self, key: str) -> Optional[str]:
return self._entries[key] if key in self._entries else None
Copy link
Contributor

Choose a reason for hiding this comment

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

also an option:

Suggested change
return self._entries[key] if key in self._entries else None
return self._entries.get(key, None)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

__get_item__ and __set_item__ are removed

)

def __repr__(self) -> str:
return str(self)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could move __str__ implementation here, and then get __str__ for free

Copy link
Contributor Author

Choose a reason for hiding this comment

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


def __enter__(self) -> None:
for key, value in self._pairs.items():
self._annotation[key] = value
Copy link
Contributor

Choose a reason for hiding this comment

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

I think self._annotation.update(**pairs) would also work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

return str(self)


PROVENANCE_KEY = "Provenance"
Copy link
Contributor

Choose a reason for hiding this comment

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

I usually tack on some dunders for good measure if I am reserving a "special" key name that I don't want to collide with anything a user might supply

Copy link
Contributor

Choose a reason for hiding this comment

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

Though as long as there is already a class LibraryAnnotations couldn't the provenance be stored separately instead of in the same dict?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

took the suggestion that the provenance can be separate

@magnatelee magnatelee requested a review from bryevdv November 11, 2022 01:25
Comment on lines 93 to 97
return "|".join(
pairs
if self._provenance is None
else chain(pairs, (f"Provenance,{self._provenance}",))
)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a little simpler, and avoids the itertools import:

Suggested change
return "|".join(
pairs
if self._provenance is None
else chain(pairs, (f"Provenance,{self._provenance}",))
)
if self._provenance is not None:
pairs += (f"Provenance,{self._provenance}",)
return "|".join(pairs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok I didn't know you could add a tuple to a generator, but wouldn't that materialize the generator into some sequence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like I can't even do it

TypeError: unsupported operand type(s) for +=: 'generator' and 'tuple'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean, I'm fine with materializing the generator into a list, I guess I was trying to be too clever...

Copy link
Contributor

@bryevdv bryevdv Nov 11, 2022

Choose a reason for hiding this comment

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

oh right, you'd have to call tuple on it first, either way seems fine (I'm assuming it's a pretty small list of things)

@magnatelee magnatelee merged commit 002f828 into nv-legate:branch-22.12 Nov 14, 2022
@magnatelee magnatelee deleted the extended-provenance-pr branch November 14, 2022 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-feature PR introduces a new feature and will be classified as such in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants