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

Sensor name convention hashed #1597

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

Conversation

mikesteroonie
Copy link

Made a new implementation for the naming convention of the sensor objects with a hash, to ensure all instances fit within th 63 character limit as issue #1591 outlined that metaflow executed normal operations despite the object not being deployed.

Label added to sensor object to maintain data integrity of the original workflow name.

Copy link
Collaborator

@saikonen saikonen left a comment

Choose a reason for hiding this comment

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

All in all great stuff. I would hold off on this for just a bit until #1608 moves forward so we know how to proceed with regards to sensors.

A big question with both the general argo naming convention changes, and this PR is, how to cleanly handle existing deployments so that no resources are left dangling when redeploying with a new naming schema.

if self._sensor:
# -----Added----
# Set the original workflow name as a label on the sensor
self._sensor.labels = {self.name}
Copy link
Collaborator

@saikonen saikonen Oct 27, 2023

Choose a reason for hiding this comment

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

#1608 is aiming to fix this, but in general, the full workflow name can be quite lengthy, so I would suggest recording it in annotations instead in order to not hit any length limits.

@@ -82,8 +82,18 @@ def list_executions(self, state_machine_arn, states):
)

def terminate_execution(self, state_machine_arn, execution_arn):
# TODO
pass
try:
Copy link
Collaborator

Choose a reason for hiding this comment

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

unrelated changes to this PR

hashed_name = hashlib.sha256(self.name.encode()).hexdigest()[
:10
] # Get first 10 chars of the hash
sensor_name = f"s-{hashed_name}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

does argo append the random suffix in all sensor deployments, or only in cases where the length exceeds 63 characters? what we usually aim to do is retain the original name, but if truncation needs to happen, chop it up and append a hash of the value as a suffix.

This way at at least part of the name remains descriptive

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

2 participants