-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix(sdk): Fix compiler bug for legacy outputs mlpipeline-ui-metadata and mlpipeline-metrics. Fixes #6311 #6325
fix(sdk): Fix compiler bug for legacy outputs mlpipeline-ui-metadata and mlpipeline-metrics. Fixes #6311 #6325
Conversation
mlpipeline-metrics.
/assign @chensun |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
@@ -1062,6 +1062,15 @@ def __init__(self, argument, input=None, path=None): | |||
self.path = path | |||
|
|||
|
|||
def _is_legacy_output_name(output_name: str) -> Tuple[bool, str]: | |||
normalized_output_name = re.sub('[^a-zA-Z0-9]', '-', output_name.lower()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have this transform in many places, shall we extract a common util?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's what I just did here? :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the difference between this and
def sanitize_k8s_name(name, allow_capital_underscore=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and
pipelines/sdk/python/kfp/components/_naming.py
Lines 51 to 53 in cc83e10
def _sanitize_file_name(name): import re return re.sub('[^-_.0-9a-zA-Z]+', '_', name) pipelines/sdk/python/kfp/dsl/_pipeline_param.py
Lines 25 to 41 in cc83e10
def sanitize_k8s_name(name, allow_capital_underscore=False): """Cleans and converts the names in the workflow. Args: name: original name, allow_capital_underscore: whether to allow capital letter and underscore in this name. Returns: A sanitized name. """ if allow_capital_underscore: return re.sub('-+', '-', re.sub('[^-_0-9A-Za-z]+', '-', name)).lstrip('-').rstrip('-') else: return re.sub('-+', '-', re.sub('[^-0-9a-z]+', '-', name.lower())).lstrip('-').rstrip('-') pipelines/sdk/python/kfp/dsl/dsl_utils.py
Lines 51 to 58 in fdab2e6
def _sanitize_name(name: str) -> str: """Sanitizes name to comply with IR naming convention. The sanitized name contains only lower case alphanumeric characters and dashes. """ return re.sub('-+', '-', re.sub('[^-0-9a-z]+', '-', name.lower())).lstrip('-').rstrip('-')
looks like there's an implementation everywhere...
this is a non blocker for this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't see your point here. These different functions are used in different places. I don't think the goal of this PR is to refactor and clean up this code.
Thank you for fixing this quickly! |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Bobgy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixes the bug introduced by PR #6268
Fixes #6311