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

chore(launcher): revert metadata to custom properties & artifact name -> event path #5842

Merged
merged 9 commits into from
Jun 13, 2021

Conversation

Bobgy
Copy link
Contributor

@Bobgy Bobgy commented Jun 11, 2021

Description of your changes:
Reasons:

  • in the future, when MLMD supports filtering, only top level fields in custom properties will be supported, so we'd better keep using them
  • move artifact name to event path field, because
// Event path is conceptually artifact name for the execution.
// We cannot store the name as a property of artifact "a", because for example:
// 1. In first task "preprocess", there's an output artifact "processed_data".
// 2. In second task "train", there's an input artifact "dataset" passed from "preprocess"
// task's "processed_data" output.
//
// Now the same artifact is called "processed_data" in "preprocess" task, but "dataset" in
// "train" task, because artifact name is related to the context it's used.
// Therefore, we should store artifact name as a property of the artifact's events
// (connects artifact and execution) instead of the artifact's property.

Checklist:

@zijianjoy
Copy link
Collaborator

/lgtm

Thank you Yuan for the change!

@zijianjoy
Copy link
Collaborator

/test kubeflow-pipelines-samples-v2

The previous test's sub-pipeline seems finished successfully https://4e18c21c9d33d20f-dot-datalab-vm-staging.googleusercontent.com/#/runs/details/cbbb47bf-f409-4943-a2a9-387f55e942b8, but parent-pipeline didn't get the run status.

@google-oss-robot
Copy link

New changes are detected. LGTM label has been removed.

@Bobgy
Copy link
Contributor Author

Bobgy commented Jun 11, 2021

Thanks @zijianjoy! I fixed it

@Bobgy Bobgy added the lgtm label Jun 13, 2021
@Bobgy
Copy link
Contributor Author

Bobgy commented Jun 13, 2021

/approve
Getting the big changes in, feel free to keep reviewing this.

@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 f1f99c0 into kubeflow:master Jun 13, 2021
@Bobgy Bobgy deleted the revert-metadata-field branch June 16, 2021 12:09
@Bobgy Bobgy changed the title chore(launcher): revert meadata to custom properties & artifact name -> event path chore(launcher): revert metadata to custom properties & artifact name -> event path Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants