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

fix(sdk): Fix compiler bug for legacy outputs mlpipeline-ui-metadata and mlpipeline-metrics. Fixes #6311 #6325

Merged
merged 2 commits into from
Aug 13, 2021
Merged

fix(sdk): Fix compiler bug for legacy outputs mlpipeline-ui-metadata and mlpipeline-metrics. Fixes #6311 #6325

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
58137b9
Fix compiler bug for legacy outputs mlpipeline-ui-metadata and
neuromage Aug 13, 2021
2b8dcde
fix - only delete legacy outputs in non-v2 pipelines.
neuromage Aug 13, 2021
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
27 changes: 21 additions & 6 deletions sdk/python/kfp/dsl/_container_op.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

import re
import warnings
from typing import Any, Dict, Iterable, List, TypeVar, Union, Callable, Optional, Sequence
from typing import Any, Callable, Dict, List, Optional, Sequence, TypeVar, Tuple, Union

from kubernetes.client import V1Toleration, V1Affinity
from kubernetes.client.models import (V1Container, V1EnvVar, V1EnvFromSource,
Expand Down Expand Up @@ -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())
Copy link
Contributor

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?

Copy link
Contributor Author

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? :-)

Copy link
Contributor

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

pipelines/sdk/python/kfp/compiler/_k8s_helper.py

Line 20 in e7c281b

def sanitize_k8s_name(name, allow_capital_underscore=False):
?

Copy link
Contributor

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

Copy link
Contributor Author

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.

if normalized_output_name in [
'mlpipeline-ui-metadata',
'mlpipeline-metrics',
]:
return True, normalized_output_name
return False, output_name

class ContainerOp(BaseOp):
"""Represents an op implemented by a container image.

Expand Down Expand Up @@ -1257,11 +1266,9 @@ def _decorated(*args, **kwargs):
# outputs that should always be saved as artifacts
# TODO: Remove when outputs are always saved as artifacts
for output_name, path in dict(file_outputs).items():
normalized_output_name = re.sub('[^a-zA-Z0-9]', '-', output_name.lower())
if normalized_output_name in [
'mlpipeline-ui-metadata', 'mlpipeline-metrics'
]:
output_artifact_paths[normalized_output_name] = path
is_legacy_name, normalized_name = _is_legacy_output_name(output_name)
if is_legacy_name:
output_artifact_paths[normalized_name] = path
del file_outputs[output_name]

# attributes specific to `ContainerOp`
Expand Down Expand Up @@ -1382,6 +1389,14 @@ def _set_metadata(self, metadata, arguments: Optional[Dict[str, Any]] = None):
output_filename = _components._generate_output_file_name(output.name)
self.file_outputs[output.name] = output_filename

if not kfp.COMPILING_FOR_V2:
for output_name, path in dict(self.file_outputs).items():
is_legacy_name, normalized_name = _is_legacy_output_name(output_name)
if is_legacy_name:
self.output_artifact_paths[normalized_name] = path
del self.file_outputs[output_name]
del self.outputs[output_name]

if arguments is not None:
for input_name, value in arguments.items():
self.artifact_arguments[input_name] = str(value)
Expand Down