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

Jump to bottom

chore(launcher): move artifact metadata to metadata struct field. Fixes #5788 #5793

Merged
merged 5 commits into from
Jun 8, 2021
Merged

chore(launcher): move artifact metadata to metadata struct field. Fixes #5788 #5793

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
188ec92
feat(launcher): move component output metadata to metadata field
Bobgy Jun 3, 2021
23ac9e8
add MLMD validation for metrics_visualization_v2
Bobgy Jun 3, 2021
47e823f
clean up
Bobgy Jun 3, 2021
955ed5a
address comment
Bobgy Jun 4, 2021
66f72a3
fix
Bobgy Jun 4, 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
175 changes: 170 additions & 5 deletions samples/test/metrics_visualization_v2_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,180 @@
# See the License for the specific language governing permissions and
# limitations under the License.

import unittest
from unittest.mock import ANY
Copy link
Member

Choose a reason for hiding this comment

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

nit: import packages or modules, but not classes or functions:
https://google.github.io/styleguide/pyguide.html#22-imports

especially since this ANY could be confused with Any from typing.

Copy link
Contributor Author

@Bobgy Bobgy Jun 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, I'll fix this. I think our sample pipelines and documentation has many violations, it'll be good to fix them too. Is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

from pprint import pprint
import kfp
import kfp_server_api

from .metrics_visualization_v2 import metrics_visualization_pipeline
from .util import run_pipeline_func, TestCase
from .util import run_pipeline_func, TestCase, KfpMlmdClient


def verify(
run: kfp_server_api.ApiRun, mlmd_connection_config, argo_workflow_name: str,
**kwargs
):
t = unittest.TestCase()
t.maxDiff = None # we always want to see full diff
t.assertEqual(run.status, 'Succeeded')
client = KfpMlmdClient(mlmd_connection_config=mlmd_connection_config)
tasks = client.get_tasks(argo_workflow_name=argo_workflow_name)

task_names = [*tasks.keys()]
t.assertCountEqual(
task_names,
['wine-classification', 'iris-sgdclassifier', 'digit-classification'],
'task names'
)

wine_classification: KfpTask = tasks.get('wine-classification')
iris_sgdclassifier: KfpTask = tasks.get('iris-sgdclassifier')
digit_classification: KfpTask = tasks.get('digit-classification')

pprint('======= wine classification task =======')
pprint(wine_classification.get_dict())
pprint('======= iris sgdclassifier task =======')
pprint(iris_sgdclassifier.get_dict())
pprint('======= digit classification task =======')
pprint(digit_classification.get_dict())

zijianjoy marked this conversation as resolved.
Show resolved Hide resolved
t.assertEqual(
wine_classification.get_dict(), {
'inputs': {
'artifacts': [],
'parameters': {}
},
'name': 'wine-classification',
'outputs': {
'artifacts': [{
'metadata': {
zijianjoy marked this conversation as resolved.
Show resolved Hide resolved
'confidenceMetrics': [{
'confidenceThreshold': 2.0,
'falsePositiveRate': 0.0,
'recall': 0.0
}, {
'confidenceThreshold': 1.0,
'falsePositiveRate': 0.0,
'recall': 0.33962264150943394
}, {
'confidenceThreshold': 0.9,
'falsePositiveRate': 0.0,
'recall': 0.6037735849056604
}, {
'confidenceThreshold': 0.8,
'falsePositiveRate': 0.0,
'recall': 0.8490566037735849
}, {
'confidenceThreshold': 0.6,
'falsePositiveRate': 0.0,
'recall': 0.8867924528301887
}, {
'confidenceThreshold': 0.5,
'falsePositiveRate': 0.0125,
'recall': 0.9245283018867925
}, {
'confidenceThreshold': 0.4,
'falsePositiveRate': 0.075,
'recall': 0.9622641509433962
}, {
'confidenceThreshold': 0.3,
'falsePositiveRate': 0.0875,
'recall': 1.0
}, {
'confidenceThreshold': 0.2,
'falsePositiveRate': 0.2375,
'recall': 1.0
}, {
'confidenceThreshold': 0.1,
'falsePositiveRate': 0.475,
'recall': 1.0
}, {
'confidenceThreshold': 0.0,
'falsePositiveRate': 1.0,
'recall': 1.0
}]
},
'name': 'metrics',
'type': 'system.ClassificationMetrics'
}],
'parameters': {}
},
'type': 'kfp.ContainerExecution'
}
)
t.assertEqual(
iris_sgdclassifier.get_dict(), {
'inputs': {
'artifacts': [],
'parameters': {
'test_samples_fraction': 0.3
}
},
'name': 'iris-sgdclassifier',
'outputs': {
'artifacts': [{
'metadata': {
'confusionMatrix': {
'annotationSpecs': [{
'displayName': 'Setosa'
}, {
'displayName': 'Versicolour'
}, {
'displayName': 'Virginica'
}],
'rows': [{ # these numbers can be random during execution
'row': [ANY, ANY, ANY]
}, {
'row': [ANY, ANY, ANY]
}, {
'row': [ANY, ANY, ANY]
}]
}
},
'name': 'metrics',
'type': 'system.ClassificationMetrics'
}],
'parameters': {}
},
'type': 'kfp.ContainerExecution'
}
)
rows = iris_sgdclassifier.get_dict(
)['outputs']['artifacts'][0]['metadata']['confusionMatrix']['rows']
for i, row in enumerate(rows):
for j, item in enumerate(row['row']):
t.assertIsInstance(
item, float,
f'value of confusion matrix row {i}, col {j} is not a number'
)

def verify(run, run_id: str, **kwargs):
assert run.status == 'Succeeded'
t.assertEqual(
digit_classification.get_dict(), {
'inputs': {
'artifacts': [],
'parameters': {}
},
'name': 'digit-classification',
'outputs': {
'artifacts': [{
'metadata': {
'accuracy': 92.0
},
'name': 'metrics',
'type': 'system.Metrics'
}],
'parameters': {}
},
'type': 'kfp.ContainerExecution'
}
)


run_pipeline_func([
TestCase(pipeline_func=metrics_visualization_pipeline, verify_func=verify, mode=kfp.dsl.PipelineExecutionMode.V2_COMPATIBLE),
])
TestCase(
pipeline_func=metrics_visualization_pipeline,
verify_func=verify,
mode=kfp.dsl.PipelineExecutionMode.V2_COMPATIBLE
),
])
67 changes: 37 additions & 30 deletions samples/test/two_step_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,11 @@ def verify_tasks(t: unittest.TestCase, tasks: dict):
}
},
'outputs': {
'artifacts': [{'custom_properties': {'name': 'output_dataset_one'},
'name': 'output_dataset_one',
'type': 'system.Dataset'
}],
'artifacts': [{
'metadata': {},
'name': 'output_dataset_one',
'type': 'system.Dataset'
}],
'parameters': {
'output_parameter_one': 1234
}
Expand All @@ -71,19 +72,21 @@ def verify_tasks(t: unittest.TestCase, tasks: dict):
train.get_dict(), {
'name': 'train-op',
'inputs': {
'artifacts': [{'custom_properties': {'name': 'output_dataset_one'},
'name': 'output_dataset_one',
'type': 'system.Dataset',
}],
'artifacts': [{
'metadata': {},
'name': 'output_dataset_one',
'type': 'system.Dataset',
}],
'parameters': {
'num_steps': 1234
}
},
'outputs': {
'artifacts': [{'custom_properties': {'name': 'model'},
'name': 'model',
'type': 'system.Model',
}],
'artifacts': [{
'metadata': {},
'name': 'model',
'type': 'system.Model',
}],
'parameters': {}
},
'type': 'kfp.ContainerExecution'
Expand All @@ -98,18 +101,19 @@ def verify_artifacts(t: unittest.TestCase, tasks: dict, artifact_uri_prefix):


def verify(
run: kfp_server_api.ApiRun, mlmd_connection_config, argo_workflow_name: str,
**kwargs
run: kfp_server_api.ApiRun, mlmd_connection_config, argo_workflow_name: str,
**kwargs
):
t = unittest.TestCase()
t.maxDiff = None # we always want to see full diff
t.assertEqual(run.status, 'Succeeded')
tasks = get_tasks(mlmd_connection_config, argo_workflow_name)
verify_tasks(t, tasks)


def verify_with_default_pipeline_root(
run: kfp_server_api.ApiRun, mlmd_connection_config, argo_workflow_name: str,
**kwargs
run: kfp_server_api.ApiRun, mlmd_connection_config, argo_workflow_name: str,
**kwargs
):
t = unittest.TestCase()
t.maxDiff = None # we always want to see full diff
Expand All @@ -120,8 +124,8 @@ def verify_with_default_pipeline_root(


def verify_with_specific_pipeline_root(
run: kfp_server_api.ApiRun, mlmd_connection_config, argo_workflow_name: str,
**kwargs
run: kfp_server_api.ApiRun, mlmd_connection_config, argo_workflow_name: str,
**kwargs
):
t = unittest.TestCase()
t.maxDiff = None # we always want to see full diff
Expand All @@ -143,19 +147,22 @@ def verify_with_specific_pipeline_root(
mode=kfp.dsl.PipelineExecutionMode.V1_LEGACY
),
# Verify default pipeline_root with MinIO
TestCase(pipeline_func=two_step_pipeline,
verify_func=verify_with_default_pipeline_root,
mode=kfp.dsl.PipelineExecutionMode.V2_COMPATIBLE,
arguments={
kfp.dsl.ROOT_PARAMETER_NAME: ''},
),
TestCase(
pipeline_func=two_step_pipeline,
verify_func=verify_with_default_pipeline_root,
mode=kfp.dsl.PipelineExecutionMode.V2_COMPATIBLE,
arguments={kfp.dsl.ROOT_PARAMETER_NAME: ''},
),
# Verify overriding pipeline root to MinIO
TestCase(pipeline_func=two_step_pipeline,
verify_func=verify_with_specific_pipeline_root,
mode=kfp.dsl.PipelineExecutionMode.V2_COMPATIBLE,
arguments={
kfp.dsl.ROOT_PARAMETER_NAME: 'minio://mlpipeline/override/artifacts' },
)
TestCase(
pipeline_func=two_step_pipeline,
verify_func=verify_with_specific_pipeline_root,
mode=kfp.dsl.PipelineExecutionMode.V2_COMPATIBLE,
arguments={
kfp.dsl.ROOT_PARAMETER_NAME:
'minio://mlpipeline/override/artifacts'
},
)
])

# %%
25 changes: 10 additions & 15 deletions samples/test/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
from dataclasses import dataclass, asdict
from pprint import pprint
from typing import Dict, List, Callable, Optional
from google.protobuf.json_format import MessageToDict

import kfp
import kfp_server_api
Expand Down Expand Up @@ -222,34 +223,28 @@ class KfpArtifact:
name: str
uri: str
type: str
custom_properties: dict
metadata: dict

@classmethod
def new(
cls, mlmd_artifact: metadata_store_pb2.Artifact,
mlmd_artifact_type: metadata_store_pb2.ArtifactType
):
custom_properties = {}
for k, v in mlmd_artifact.custom_properties.items():
raw_value = None
if v.string_value:
raw_value = v.string_value
if v.int_value:
raw_value = v.int_value
if v.double_value:
raw_value = v.double_value
custom_properties[k] = raw_value
artifact_name = ''
if mlmd_artifact.name != '':
if mlmd_artifact.name:
artifact_name = mlmd_artifact.name
else:
if 'name' in custom_properties.keys():
artifact_name = custom_properties['name']
if 'name' in mlmd_artifact.custom_properties.keys():
artifact_name = mlmd_artifact.custom_properties['name'
].string_value
metadata = MessageToDict(
mlmd_artifact.custom_properties.get('metadata').struct_value
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the usage of transfer mlmd_artifact.custom_properties.get('metadata') to a metadata struct? Can't we just verify the custom_properties in tests?

Copy link
Contributor Author

@Bobgy Bobgy Jun 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The test util script tries to hide MLMD implementation details from KFP data model and let e2e tests decouple from specific MLMD implementation. Because in this way, tests assert on KFP data model -- the metadata struct contains all user logged metadata.

Benefits:

  • e2e test cases are concise
  • e2e test cases mimic how a user would query MLMD and use information from MLMD to get status of a pipeline, we can consider extracting test utils to a user library in the future to make this use-case easier
  • as we standardize in [v2compat] finalize pipeline run MLMD schema #5669, there will be fewer changes needed in e2e pipeline test cases

)
return cls(
name=artifact_name,
type=mlmd_artifact_type.name,
uri=mlmd_artifact.uri,
custom_properties=custom_properties
metadata=metadata
)


Expand Down
Loading