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

Added component definition files for our components #539

Merged
merged 13 commits into from
Apr 4, 2019
Merged

Added component definition files for our components #539

merged 13 commits into from
Apr 4, 2019

Conversation

Ark-kun
Copy link
Contributor

@Ark-kun Ark-kun commented Dec 14, 2018 •
edited
Loading

Added definitions for the following components:
dataflow/predict
dataflow/tfdv
dataflow/tft
dataflow/tma
kubeflow/deployer
kubeflow/dnntrainer
kubeflow/katib-launcher
local/confusion_matrix
local/roc

Also, the kubeflow-training-classification sample now loads the components from component files.

This change is Reviewable

@Ark-kun Ark-kun changed the title Added component artifacts for our components [WIP]Added component artifacts for our components Dec 14, 2018
@Ark-kun Ark-kun changed the title [WIP]Added component artifacts for our components [WIP]Added component definition files for our components Dec 14, 2018
@Ark-kun
Copy link
Contributor Author

Ark-kun commented Dec 14, 2018

/cc @paveldournov

@Ark-kun Ark-kun changed the title [WIP]Added component definition files for our components Added component definition files for our components Dec 18, 2018
@Ark-kun Ark-kun force-pushed the Added-component-definitions-to-our-components branch from eb53ce6 to 791fd5c Compare December 26, 2018 22:47
@Ark-kun Ark-kun force-pushed the Added-component-definitions-to-our-components branch from 791fd5c to 896cd4d Compare January 11, 2019 22:22
qimingj
qimingj reviewed Jan 13, 2019
View reviewed changes
Copy link
Contributor

@qimingj qimingj left a comment

Choose a reason for hiding this comment

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

A few comments.

components/dataflow/predict/component.yaml Outdated Show resolved Hide resolved
samples/kubeflow-tf/kubeflow-training-classification.py Outdated Show resolved Hide resolved
samples/kubeflow-tf/kubeflow-training-classification.py Show resolved Hide resolved
neuromage
neuromage reviewed Jan 24, 2019
View reviewed changes
Copy link
Contributor

@neuromage neuromage left a comment

Choose a reason for hiding this comment

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

Were these files generated or manually authored? Shouldn't the canonical version of component YAML be ultimately generated by the DSL?

@gaoning777
Copy link
Contributor

echoed @neuromage. BTW, let's merge this once we decide upon the component spec format

@Ark-kun Ark-kun force-pushed the Added-component-definitions-to-our-components branch 2 times, most recently from 2366621 to e8ecdee Compare March 26, 2019 23:38
@Ark-kun Ark-kun force-pushed the Added-component-definitions-to-our-components branch from 17c73b7 to c3009e2 Compare March 29, 2019 19:32
@Ark-kun
Copy link
Contributor Author

Ark-kun commented Mar 29, 2019

/test kubeflow-pipeline-e2e-test

2 similar comments
@Ark-kun
Copy link
Contributor Author

Ark-kun commented Mar 29, 2019

/test kubeflow-pipeline-e2e-test

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Apr 1, 2019

/test kubeflow-pipeline-e2e-test

@Ark-kun Ark-kun force-pushed the Added-component-definitions-to-our-components branch 2 times, most recently from 6c9de1d to 3f76567 Compare April 4, 2019 01:37
@Ark-kun
Removed all properties from GCPPath
dd55d25 
This will confuse our users and make type checking worse, but Hongye and Ajay requested that.

`s/type: (\{GCSPath:.*?}})(.*)/type: GCPPath$2 # type: $1/g`
@Ark-kun
Removed the usage of the ComponentStore
a3fba17 
Now the samples are invalid until they're merged to master, but Hongye asked for that.
@Ark-kun Ark-kun force-pushed the Added-component-definitions-to-our-components branch from 3f76567 to a3fba17 Compare April 4, 2019 01:38
hongye-sun
hongye-sun reviewed Apr 4, 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.

/lgtm

Thanks for the changes.

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Apr 4, 2019

/lgtm
Thanks for the changes.

Thank you!!!

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ark-kun

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: Ark-kun

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 6b402f8 into kubeflow:master Apr 4, 2019
@Ark-kun Ark-kun deleted the Added-component-definitions-to-our-components branch April 4, 2019 19:27
HumairAK pushed a commit to red-hat-data-services/data-science-pipelines that referenced this pull request Mar 11, 2024
@pugangxa
Support the simple condition for any sequencer (kubeflow#539)
ae56385
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

@qimingj qimingj qimingj left review comments

@gaoning777 gaoning777 gaoning777 left review comments

@neuromage neuromage neuromage left review comments

@paveldournov paveldournov Awaiting requested review from paveldournov

Assignees

@IronPan IronPan

@gaoning777 gaoning777

@paveldournov paveldournov

@hongye-sun hongye-sun

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

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