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

SDK/Components - Added Json Schema spec for the component format #669

Merged
merged 15 commits into from
Nov 12, 2019
Merged

SDK/Components - Added Json Schema spec for the component format #669

merged 15 commits into from
Nov 12, 2019

Conversation

Ark-kun
Copy link
Contributor

@Ark-kun Ark-kun commented Jan 11, 2019 •
edited
Loading

See the much shorter outline of the schema: https://github.com/kubeflow/pipelines/blob/ae7cefede4cc005b5d2f443156450f317220a99e/sdk/python/kfp/components/structures/components.json_schema.outline.yaml

It's not an example. It's an outline. It's a component schema with sub-schemas inlined and without any extra info.

This change is Reviewable

@Ark-kun Ark-kun removed the approved label Jan 11, 2019
@Ark-kun Ark-kun force-pushed the SDK/Components---Added-JsonSchema-spec-for-the-component-format branch from 9cb1baa to 6cbb6f2 Compare January 11, 2019 22:09
@Ark-kun Ark-kun force-pushed the SDK/Components---Added-JsonSchema-spec-for-the-component-format branch from 6cbb6f2 to 64ca29d Compare January 11, 2019 22:10
@Ark-kun Ark-kun removed the approved label Jan 11, 2019
@Ark-kun Ark-kun force-pushed the SDK/Components---Added-JsonSchema-spec-for-the-component-format branch from 64ca29d to ae7cefe Compare January 12, 2019 00:03
@Ark-kun Ark-kun removed the approved label Jan 12, 2019
@Ark-kun
Copy link
Contributor Author

Ark-kun commented Jan 12, 2019

/test kubeflow-pipeline-build-image

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Jan 12, 2019

I wonder should we leave the spec nesting or should we squash it like Argo does? https://github.com/argoproj/argo/blob/master/pkg/apis/workflow/v1alpha1/types.go#L152
@IronPan @vicaire , what do you think?

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Jan 12, 2019

Also, WDYT about the k8sPodOptions naming? I wanted to show that these options are k8s-specific unlike many other options that are k8s-agnostic.

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Jan 12, 2019

/test kubeflow-pipeline-build-image

1 similar comment
@Ark-kun
Copy link
Contributor Author

Ark-kun commented Jan 14, 2019

/test kubeflow-pipeline-build-image

@IronPan
Copy link
Member

IronPan commented Jan 14, 2019

/assign @IronPan

@Ark-kun Ark-kun force-pushed the SDK/Components---Added-JsonSchema-spec-for-the-component-format branch from ae7cefe to d56ff7c Compare January 15, 2019 21:59
@Ark-kun Ark-kun removed the approved label Jan 15, 2019
@Ark-kun
Fixed missing "required"
f8ada55
@Ark-kun
Replaced PrimitiveTypes with just string
feae5df
@Ark-kun
Renamed CommandlineArgumentType to StringOrPlaceholder
cdfa0ff 
Made more ContainerSpec properties support placeholders
@Ark-kun
Removed support for type inheritance and generic types as requested b…
4bd6c20 
…y Ning and Ajay
@Ark-kun
Some people are scared of graphs/pipelines - removing them
c8677d3
@Ark-kun
Some people do not like optional inputs and conditionals - removing them
8c47f28 
Sorry, Yasser and Bradley.
@Ark-kun
Some people might be scared of predicates or conditional execution - …
b7fb1b7 
…removing them

Sure, Argo and DSL supports it, but some people care more about the spec file size even when that means dropping already supported features.
Sorry, Bradley and Riley.
@Ark-kun
Copy link
Contributor Author

Ark-kun commented Aug 9, 2019

Could we drop the graph component part of the component spec in this PR?

I've done that on February 5

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Nov 12, 2019

I've updated the schema to match the current effective component specification.

@Ark-kun Ark-kun requested review from numerology and removed request for qimingj November 12, 2019 22:17
numerology
numerology reviewed Nov 12, 2019
View reviewed changes
Copy link

@numerology numerology left a comment

Choose a reason for hiding this comment

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

some nit comments. Otherwise LGTM

sdk/python/kfp/components/structures/components.json_schema.json Show resolved Hide resolved
sdk/python/kfp/components/structures/components.json_schema.json Show resolved Hide resolved
@numerology
Copy link

/lgtm

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Nov 12, 2019

/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 k8s-ci-robot merged commit da5cbb8 into kubeflow:master Nov 12, 2019
@Ark-kun Ark-kun mentioned this pull request Nov 21, 2019
Merged
magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023
@alexwennerberg
Update quick_install.sh (kubeflow#669)
caa0271 
Add set -e to exit on first failure
HumairAK pushed a commit to red-hat-data-services/data-science-pipelines that referenced this pull request Mar 11, 2024
@ScrapCodes
fix(pipelineloop): sort params to fix flakiness in tests. (kubeflow#669)
f1f1a1c
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

@rileyjbauer rileyjbauer rileyjbauer left review comments

@vicaire vicaire vicaire requested changes

@numerology numerology numerology left review comments

@neuromage neuromage neuromage left review comments

@yebrahim yebrahim Awaiting requested review from yebrahim

@IronPan IronPan Awaiting requested review from IronPan

@gaoning777 gaoning777 Awaiting requested review from gaoning777

@paveldournov paveldournov Awaiting requested review from paveldournov

Assignees

@IronPan IronPan

@numerology numerology

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

Successfully merging this pull request may close these issues.
9 participants
@Ark-kun @IronPan @vicaire @gaoning777 @numerology @k8s-ci-robot @neuromage @rileyjbauer @hongye-sun