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

merged 5 commits into from
Jun 8, 2021

Conversation

Bobgy
Copy link
Contributor

@Bobgy Bobgy commented Jun 3, 2021

Description of your changes:
Fixes #5788

Checklist:

@google-cla google-cla bot added the cla: yes label Jun 3, 2021
@Bobgy
Copy link
Contributor Author

Bobgy commented Jun 4, 2021

/assign @capri-xiyue

chensun
chensun reviewed Jun 4, 2021
View reviewed changes
samples/test/metrics_visualization_v2_test.py Outdated
@@ -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

@Bobgy Bobgy changed the title chore(launcher): move component emitted metadata to metadata struct field. Fixes #5788 chore(launcher): move artifact metadata to metadata struct field. Fixes #5788 Jun 4, 2021
capri-xiyue
capri-xiyue reviewed Jun 4, 2021
View reviewed changes
samples/test/util.py
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

@Bobgy Bobgy mentioned this pull request Jun 6, 2021
Closed
zijianjoy
zijianjoy reviewed Jun 7, 2021
View reviewed changes
Copy link
Collaborator

@zijianjoy zijianjoy left a comment

Choose a reason for hiding this comment

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

/lgtm

Thank you Yuan for the change!

samples/test/metrics_visualization_v2_test.py Show resolved Hide resolved
samples/test/metrics_visualization_v2_test.py Show resolved Hide resolved
@capri-xiyue
Copy link
Contributor

/test kubeflow-pipelines-samples-v2

@capri-xiyue
Copy link
Contributor

capri-xiyue commented Jun 8, 2021 •
edited
Loading

Looks like there is something wrong with v2 sample test. The downloading src pod will get stuck at container creating stage. It's not related to the PR. Similar issue happens in #5802

@Bobgy
Copy link
Contributor Author

Bobgy commented Jun 8, 2021

/retest

@Bobgy
Copy link
Contributor Author

Bobgy commented Jun 8, 2021 

- name: wait
      state:
        waiting:
          reason: CreateContainerError
          message: context deadline exceeded
      lastState:
        terminated:
          exitCode: 0
          startedAt: null
          finishedAt: null
          containerID: >-
            docker://5b013e4e3be4564a9c7ef0ad40cce13b502ac573d58e5f748cafcde96d346abd
      ready: false
      restartCount: 0
      image: 'gcr.io/ml-pipeline/argoexec:v2.12.9-license-compliance'
      imageID: >-
        docker-pullable://gcr.io/ml-pipeline/argoexec@sha256:9b512a260a215d65054685511dbd867184b0041721c51126c8ef478b4f32bd5e
      containerID: >-
        docker://5b013e4e3be4564a9c7ef0ad40cce13b502ac573d58e5f748cafcde96d346abd
      started: false

In one task, wait container failed to be created, strange issue, seems transient.

@Bobgy
Copy link
Contributor Author

Bobgy commented Jun 8, 2021

/retest

@Bobgy
Copy link
Contributor Author

Bobgy commented Jun 8, 2021

/approve

@google-oss-robot
Copy link

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-robot google-oss-robot merged commit a75a3c7 into kubeflow:master Jun 8, 2021
@Bobgy Bobgy deleted the mlmd-metadata branch June 8, 2021 06:49
@capri-xiyue
Copy link
Contributor

/lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@capri-xiyue capri-xiyue capri-xiyue left review comments

@zijianjoy zijianjoy zijianjoy left review comments

@chensun chensun chensun left review comments

@Ark-kun Ark-kun Awaiting requested review from Ark-kun

@numerology numerology Awaiting requested review from numerology

Assignees

@zijianjoy zijianjoy

@capri-xiyue capri-xiyue

Labels
approved cla: yes lgtm size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[v2compat] move component emitted metadata to metadata struct field
5 participants
@Bobgy @capri-xiyue @google-oss-robot @chensun @zijianjoy