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 sample test for multiple output #2328

Merged
merged 6 commits into from
Oct 10, 2019
Merged

Add sample test for multiple output #2328

merged 6 commits into from
Oct 10, 2019

Conversation

gaoning777
Copy link
Contributor

@gaoning777 gaoning777 commented Oct 8, 2019

This change is Reviewable

@Ark-kun
Copy link
Contributor

Ark-kun commented Oct 8, 2019

Thank you. Sorry for not doing this sooner. I was busy revamping other samples and the sample test system.

@Ark-kun
Copy link
Contributor

Ark-kun commented Oct 8, 2019

I see the following error in the tests:
Is there any particular reason to require the GCS path to end with a letter or number?

run-sample-tests-loop(19:multiple_outputs):	     36     # The first and las characters must be alphanumeric.
run-sample-tests-loop(19:multiple_outputs):	     37     if not all([name[0].isalnum(), name[-1].isalnum()]):
run-sample-tests-loop(19:multiple_outputs):	---> 38         raise ValueError("Bucket names must start and end with a number or letter.")
run-sample-tests-loop(19:multiple_outputs):	     39     return name
run-sample-tests-loop(19:multiple_outputs):	     40 
run-sample-tests-loop(19:multiple_outputs):	ValueError: Bucket names must start and end with a number or letter.

@gaoning777 gaoning777 changed the title [WIP] add sample test for multiple output Add sample test for multiple output Oct 9, 2019
@gaoning777
Copy link
Contributor Author

Please review @Ark-kun

@gaoning777
Copy link
Contributor Author

fixing #1813

@Ark-kun
Copy link
Contributor

Ark-kun commented Oct 9, 2019

/lgtm
P.S. The fix for multiline decorators is here: #2345

@k8s-ci-robot k8s-ci-robot removed the lgtm label Oct 10, 2019
@Ark-kun
Copy link
Contributor

Ark-kun commented Oct 10, 2019

/lgtm

@gaoning777
Copy link
Contributor Author

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gaoning777

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

@k8s-ci-robot k8s-ci-robot merged commit d7f0c1e into kubeflow:master Oct 10, 2019
@gaoning777 gaoning777 deleted the add-multiple-output-sample-test branch October 10, 2019 04:08
@monicatao
Copy link

Hi How can I create multiple outputs with dsl.ContainerOp function without saving the results in file_outputs?

magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023
Signed-off-by: Dan Sun <dsun20@bloomberg.net>
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.

4 participants