Parameterize the artifact path for mlpipeline ui-metadata and metrics

[Tomcli]

We have some use case where our containerOps doesn't have permission to access the root directory. Therefore, we want to parameterize the artifact path for mlpipeline ui-metadata and metrics because those artifact paths currently are hard coded in root.

Example usage:

containerOps().set_mlpipeline_ui_metadata_path(path)

cc: @animeshsingh

---

This change is 

[k8s-ci-robot]

Hi @Tomcli. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

Hi @Tomcli. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[Ark-kun]

Hi @Tomcli
Thank you for tackling this problem. I'd love to fix it in a slightly different way. I'd like to get your feedback on this solution.

Instead of extending the user-facing API surface (adding methods to ContainerOp), let's add proper support for output artifacts the same way output parameters are supported (e.g. similar to file_outputs):

def my_op():
  return ContainerOp(
    ...,
    output_artifact_paths = {
      'mlpipeline-ui-metadata': '/outputs/mlpipeline-ui-metadata/data',
      'mlpipeline-metrics': '/outputs/mlpipeline-metrics/data',
    }
  )

This is a pretty trivial change to the ContainerOp constructor and the DSL compiler.

To preserve backwards compatibility the compiler can still add the mlpipeline-ui-metadata and mlpipeline-metrics artifacts with default paths if they're not configured in output_artifact_paths.

Could you try to do it like that?

[Ark-kun]

/ok-to-test

[Tomcli]

Hi @Ark-kun, I updated the implementation to take the artifact paths as containerOps arguments. Does it look good to you? Thank you for the reviews and suggestions.

[Tomcli]

/retest

[Tomcli]

Looks like Travis CI failed on getting some go packages.

[hongye-sun]

/hold

@Ark-kun
Hold the PR as I have question on why exposing output_artifact_paths on ContainerOp constructor. It is very hard to change and deprecate in the future. Why not we just adding argo artifacts low level APIs on the ContainerOp?

[Ark-kun]

Hold the PR as I have question on why exposing output_artifact_paths on ContainerOp constructor. It is very hard to change and deprecate in the future. Why not we just adding argo artifacts low level APIs on the ContainerOp?

We're exposing Argo artifacts API.

Argo artifacts API for container templates looks like this:

outputs:
  artifacts:
  - name: art1
    path: /tmp/art1.txt
  - name: art2
    path: /tmp/art2.txt

This is a mapping between the artifact names and artifact files. We're exposing this Argo artifact mapping as output_artifact_paths (again, using Argo terminology).

It is in the official Argo sample on artifact passing. https://github.com/argoproj/argo/blob/bb4520a6f65d4e8e765ce4d426befa583721c194/examples/artifact-passing.yaml#L26

We should expose this Argo functionality so that our customers can create components that produce artifacts.

[Ark-kun]

Looks like Travis CI failed on getting some go packages.

I've restarted the tests.

[Ark-kun]

hey I am sorry, just saw this PR.
Cuz I needed this feature too, so I made a similar PR at #1064
Might want to take a look?

I'd say that your PR is orthogonal. This PR is about specifying the list of output artifacts and their paths, while your PR is about configuring the storage (which I think we should configure more globally - per pipeline or per cluster), not per output.

[animeshsingh]

@Ark-kun @neuromage coming back to it. This one is needed for us to be use k8sapi executor from Argo - since IBM cloud no longer supports docker, and KFP and Argo use docker executor by default
cc @vicaire

[Ark-kun]

Let's merge this PR.
It's a good small PR that's been waiting for too long.

[animeshsingh]

+1

[hongye-sun]

Discussed with @neuromage offline. With our current thinking of metadata design, it seems that exposing argo's artifact in ContainerOp won't cause conflicts. I will unhold the PR to unblock the hardcoded uimetadata and metrics path issue.

/hold cancel

[Tomcli]

Thanks @hongye-sun. @Ark-kun I updated the code with the latest master branch and all the tests have passed. Is this looking good to you?

[vicaire]

/lgtm

[animeshsingh]

@neuromage @hongye-sun - how far is the metadata effort (pipeline metadata being sent to metadata tracking server) from realization? any more concrete design docs there?

[neuromage]

@animeshsingh We're still working on the design. We will share more once things get more concrete. Thanks.