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

Add a convention to make sure experiment_name is parameterized in notebook sample. #2112

Merged

Conversation

numerology
Copy link

@numerology numerology commented Sep 13, 2019

Otherwise it's hard for the sample test infra to pick up the pipeline run results after submitting for a run.\

See #2110 .


This change is Reviewable

@numerology
Copy link
Author

/test kubeflow-pipeline-e2e-test

@numerology
Copy link
Author

/test kubeflow-pipeline-sample-test

1 similar comment
@numerology
Copy link
Author

/test kubeflow-pipeline-sample-test

@Ark-kun
Copy link
Contributor

Ark-kun commented Sep 13, 2019

/lgtm

If we modify Client so that it can take the default experiment name from an environment variable, then we won't need to explicitly pass the experiment names in the samples.
The Experiment feature does not seem to be used a lot, so the team has added support for "default" experiment.

@numerology
Copy link
Author

/lgtm

If we modify Client so that it can take the default experiment name from an environment variable, then we won't need to explicitly pass the experiment names in the samples.
The Experiment feature does not seem to be used a lot, so the team has added support for "default" experiment.

'default' experiment actually sounds sweet. Although there is a caveat: currently the sample test infra assumes that each sample test is running under an experiment where there is only one pipeline run. If we put all runs under the same default experiment, we need to change that logic accordingly.

Thanks for proposing the environment variable approach, that could be convenient, but we need to design a mechanism to make sure the domain of its function. Let's think about it a bit more.

@numerology
Copy link
Author

/approve

samples/README.md Outdated Show resolved Hide resolved
@gaoning777
Copy link
Contributor

/hold

samples/README.md Outdated Show resolved Hide resolved
samples/README.md Outdated Show resolved Hide resolved
@gaoning777
Copy link
Contributor

/hold cancel

@gaoning777
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gaoning777, numerology

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

@numerology
Copy link
Author

/test kubeflow-pipeline-sample-test

@numerology
Copy link
Author

/test kubeflow-pipline-sample-test

@numerology
Copy link
Author

/test kubeflow-pipeline-sample-test

1 similar comment
@numerology
Copy link
Author

/test kubeflow-pipeline-sample-test

@k8s-ci-robot k8s-ci-robot merged commit d6d3785 into kubeflow:master Sep 17, 2019
@numerology numerology deleted the add-experiment-name-parameter branch September 17, 2019 23:19
magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023
Signed-off-by: Suresh Nakkeran <suresh.n@ideas2it.com>
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.

5 participants