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

Simplify zodiac team logic #288

Closed
wants to merge 1 commit into from
Closed

Simplify zodiac team logic #288

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
5 changes: 3 additions & 2 deletions metaflow/plugins/aip/aip.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1062,8 +1062,9 @@ def _get_flow_labels(
# the Zodiac service and team labels are added to the AIP pods and set. These labels are not added
# by the AIP webhook to support user-supplied Zodiac service per AIP Notebook. Workflows launched
# in project CICD profiles will still have these labels added via the AIP webhook.
if ZILLOW_ZODIAC_SERVICE and ZILLOW_ZODIAC_TEAM:
if ZILLOW_ZODIAC_SERVICE:
ret_flow_labels[f"{zodiac_prefix}/service"] = ZILLOW_ZODIAC_SERVICE
if ZILLOW_ZODIAC_TEAM:
ret_flow_labels[f"{zodiac_prefix}/team"] = ZILLOW_ZODIAC_TEAM
Copy link
Collaborator

Choose a reason for hiding this comment

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

fyi: these are applied at compile time during CICD.

Should these be injected by the webhook?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do want to link a workflow to a zodiac service at compile time. This ensures that the workflow manifest is linked to a service no matter where it is.

I'm considering removing zodiac team label. The workflow - zodiac service association is much more stable then the workflow - zodiac team association, and zodiac team can be inferred from the zodiac service.


ret_flow_labels[f"{zodiac_prefix}/product"] = "batch"
Expand Down Expand Up @@ -1098,7 +1099,7 @@ def _set_container_labels(self, container_op: ContainerOp):
"tags.ledger.zgtools.net/ai-experiment-name", self.experiment
)

if ZILLOW_ZODIAC_SERVICE and ZILLOW_ZODIAC_TEAM:
Copy link

Choose a reason for hiding this comment

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

This looks fine to me, since ZILLOW_ZODIAC_TEAM is not used within the if statement.

I don't have an understanding of how the code works together. Is there any risk if the add_pod_annotation() gets called in the case where there is no ZILLOW_ZODIAC_TEAM?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm marking this PR as draft since I haven't fully worked through it. The changes here do not provide my desired result yet.

if ZILLOW_ZODIAC_SERVICE:
# Add a logging topic annotation specific to the Zodiac service.
# This is done to support user-supplied Zodiac service per AIP Notebook.
# Please see comments on how and why ZILLOW_ZODIAC_SERVICE label for more.
Expand Down
Loading