feature: custom annotations revisited #1568
Conversation
saikonen
force-pushed
the
feature/custom-annotations-fixed
branch
from
734de96 to
b9ce0f2
Compare
|
@saikonen Took this for a spin and everything is working as expected! |
saikonen
force-pushed
the
feature/custom-annotations-fixed
branch
from
b9ce0f2 to
8464b54
Compare
| if self.parameters: | ||
| annotations.update({"metaflow/parameters": json.dumps(self.parameters)}) | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@savingoyal Do you have an opinion on this?
saikonen
force-pushed
the
feature/custom-annotations-fixed
branch
from
8464b54 to
d4a2936
Compare
saikonen
force-pushed
the
feature/custom-annotations-fixed
branch
from
d4a2936 to
e6ab77c
Compare
|
Testing[678] @ 6257d47 |
|
Testing[678] @ 6257d47 had 3 FAILUREs. |
saikonen
force-pushed
the
feature/custom-annotations-fixed
branch
from
6257d47 to
617ead4
Compare
saikonen
force-pushed
the
feature/custom-annotations-fixed
branch
from
617ead4 to
ef85434
Compare
|
Anything further need to be done here to push this PR along? |
… labels/annotations. only use these for the kubernetes task pods
saikonen
force-pushed
the
feature/custom-annotations-fixed
branch
from
d6f068b to
8f983a5
Compare
| .annotations(annotations) | ||
| .labels(self.kubernetes_labels) |
There was a problem hiding this comment.
@saikonen is there a reason that you remove these labels from the podMetadata section and instead repeat them in each task?
There was a problem hiding this comment.
the podMetadata applying to all pods from the template wasn't a good fit for setting task-pod-specific labels, e.g.
@kubernetes(labels={"test-label": "first-step"})
@step
def first_step(self)
...
@kubernetes(labels={"test-label": "second-step"})
@step
def second_step(self)
...
as on the template level we can't make a choice which deco to pull the 'common' labels from.
The previous implementation did treat the common labels from the environment variable as shared, and set them in the podMetadata. Functionally there shouldn't be any difference though whether those are set here, or individually per-pod?
There was a problem hiding this comment.
Is there an issue with repeating common labels on the pod level?
We can extend on this, as there is a todo for the future to provide separate label/annotation global env configuration for argo-workflows which would enable labeling all relevant resources (templates, sensors etc.)
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
currentsingleton 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.