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

feature: custom annotations revisited #1568

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

saikonen
Copy link
Collaborator

@saikonen saikonen commented Oct 4, 2023

reintroduces the feature first merged in #1442 which had to be reverted in #1516 due to a regression that broke annotations for recording user and project info.

This feature keeps the 'system annotations' outside of the decorator scope, as in many cases the current singleton is not initialized to its full extent.

Also reintroduces the ability to set custom Kubernetes labels via the decorator, instead of only the environment variable.

@saikonen saikonen linked an issue Oct 4, 2023 that may be closed by this pull request
@saikonen saikonen marked this pull request as ready for review October 9, 2023 21:51
@saikonen saikonen force-pushed the feature/custom-annotations-fixed branch from 734de96 to b9ce0f2 Compare October 12, 2023 15:35
@tylerpotts
Copy link
Contributor

@saikonen Took this for a spin and everything is working as expected!

@saikonen saikonen force-pushed the feature/custom-annotations-fixed branch from b9ce0f2 to 8464b54 Compare October 12, 2023 20:48
Comment on lines +333 to +354
if self.parameters:
annotations.update({"metaflow/parameters": json.dumps(self.parameters)})

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved the new convenience annotations to this helper as well. One thing to note though is that this applies all of them to the generic self.kubernetes_annotations, which is applied generously as a baseline set of annotations (including for Sensors)

Do these annotations make sense for all objects, or should they be kept out of this helper? This simplifies the setup somewhat at least.

Copy link
Contributor

Choose a reason for hiding this comment

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

@savingoyal Do you have an opinion on this?

@saikonen saikonen force-pushed the feature/custom-annotations-fixed branch from 8464b54 to d4a2936 Compare January 15, 2024 13:28
@saikonen saikonen force-pushed the feature/custom-annotations-fixed branch from d4a2936 to e6ab77c Compare March 18, 2024 12:00
@nflx-mf-bot
Copy link
Collaborator

Testing[678] @ 6257d47

@nflx-mf-bot
Copy link
Collaborator

Testing[678] @ 6257d47 had 3 FAILUREs.

@saikonen saikonen requested a review from madhur-ob April 18, 2024 16:06
@saikonen saikonen force-pushed the feature/custom-annotations-fixed branch from 6257d47 to 617ead4 Compare April 29, 2024 10:26
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.

Annotations aren't working with the latest release of Metaflow
3 participants