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

dsl generate zip file #855

Merged
merged 10 commits into from
Mar 26, 2019
Merged

dsl generate zip file #855

merged 10 commits into from
Mar 26, 2019

Conversation

gaoning777
Copy link
Contributor

@gaoning777 gaoning777 commented Feb 25, 2019 •
edited by jlewi
Loading

This change is Reviewable

@gaoning777 gaoning777 mentioned this pull request Feb 25, 2019
Closed
@gaoning777
Copy link
Contributor Author

Hold until the backend supports the zip interface.

@IronPan
Copy link
Member

IronPan commented Feb 27, 2019

Tests should pass after #874 is in

@Ark-kun
Copy link
Contributor

Ark-kun commented Feb 28, 2019

Would you mind to call the compiled pipelines 'something.pipeline.zip'?
Basically s/.zip/.pipeline.zip/g

@Ark-kun
Copy link
Contributor

Ark-kun commented Feb 28, 2019

Also, check the auto-loaded sample list in the Backend. Does it have .tar there?

@IronPan
Copy link
Member

IronPan commented Mar 1, 2019

/test kubeflow-pipeline-e2e-test

@Ark-kun
Copy link
Contributor

Ark-kun commented Mar 1, 2019

/test kubeflow-pipeline-e2e-test

@Ark-kun Ark-kun closed this Mar 1, 2019
@Ark-kun Ark-kun reopened this Mar 1, 2019
@Ark-kun
Copy link
Contributor

Ark-kun commented Mar 1, 2019

/test kubeflow-pipeline-e2e-test

@Ark-kun
Copy link
Contributor

Ark-kun commented Mar 1, 2019 •
edited
Loading

Looks like my suspicion was correct:

pipelines/backend/src/apiserver/config/sample_config.json

Line 5 in 7847b74

"file":"/samples/xgboost-spark/xgboost-training-cm.py.tar.gz"

@IronPan
Copy link
Member

IronPan commented Mar 4, 2019

FYI Here is the code that compiles the samples
https://github.com/kubeflow/pipelines/blob/master/backend/Dockerfile#L37

@Ark-kun Ark-kun mentioned this pull request Mar 6, 2019
Merged
@Ark-kun
Copy link
Contributor

Ark-kun commented Mar 6, 2019

I've discovered the complications that were not foreseen: This change will break all existing python pipelines (.py pipelines, notebooks etc).
See #919 for a fix.

@hongye-sun
Copy link
Contributor

Since the backend is in, shall we continue this PR to generate package by file extension?

@Ark-kun
Copy link
Contributor

Ark-kun commented Mar 20, 2019 •
edited
Loading

Since the backend is in, shall we continue this PR to generate package by file extension?

Yes.
AFAIK, the solution we agreed on was:
When the user calls compile with a path, that's the exact file path where the compiled pipeline will be stored.
The file format is decided based on the file extension:

  • .tar.gz, .tgz -> Gzipped TAR
  • .zip -> ZIP
  • ? .yaml, .yml -> YAML

@gaoning777 gaoning777 mentioned this pull request Mar 25, 2019
Closed
hongye-sun
hongye-sun reviewed Mar 25, 2019
View reviewed changes
Copy link
Contributor

@hongye-sun hongye-sun left a comment

Choose a reason for hiding this comment

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

Could you add a update all readme and notebooks in the whole repo? GCP component samples and readme are under components folder.

@Ark-kun
Copy link
Contributor

Ark-kun commented Mar 26, 2019

Could you add a update all readme and notebooks in the whole repo? GCP component samples and readme are under components folder.

Files can be found using grep -R ".tar.gz'"

@Ark-kun
Copy link
Contributor

Ark-kun commented Mar 26, 2019

/lgtm

@gaoning777
Copy link
Contributor Author

/test kubeflow-pipeline-e2e-test

@hongye-sun
Copy link
Contributor

/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
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 554731e into kubeflow:master Mar 26, 2019
@gaoning777 gaoning777 deleted the dsl-zip branch March 26, 2019 23:07
@Ark-kun Ark-kun mentioned this pull request Mar 27, 2019
Closed
magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023
@ellistarn
Renaming ellis-bigelow -> ellistarn (kubeflow#855)
5e3b529
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hongye-sun hongye-sun hongye-sun left review comments

@Ark-kun Ark-kun Ark-kun approved these changes

@IronPan IronPan Awaiting requested review from IronPan

Assignees

@Ark-kun Ark-kun

@IronPan IronPan

@hongye-sun hongye-sun

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

Successfully merging this pull request may close these issues.
5 participants
@gaoning777 @IronPan @Ark-kun @hongye-sun @k8s-ci-robot