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

SDK - Separated the generated api client package #1214

Conversation

Ark-kun
Copy link
Contributor

@Ark-kun Ark-kun commented Apr 24, 2019

Separating the generated API client brings many benefits.

  • It's much easier to build the main package - No need to install Java, download swagger-generator, and run the script that cobbles together a package in style of Frankenstein's monster.
  • No you can just run python setup.py sdist to build or python setup.py install to install the package
  • This change is required for automatic API documentation generation services that need a package source in easy to install/use form
  • This change also simplifies the Client code: Instead of having 5 separate but identical configuration classes and 5 separate but identical API client classes it now only has 1.

This change is Reviewable

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Apr 24, 2019

/test kubeflow-pipeline-e2e-test

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Apr 24, 2019

/test kubeflow-pipeline-sample-test

Copy link
Contributor

@kevinbache kevinbache left a comment

Choose a reason for hiding this comment

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

This is a fairly fundamental set of changes. Do you have any tests to prove that they don't break any functionality? Also, where are the changes to the swagger client generation?

@Ark-kun Ark-kun force-pushed the SDK---Separated-the-generated-api-client-package branch from dd23973 to f1f937f Compare April 26, 2019 01:16
@Ark-kun
Copy link
Contributor Author

Ark-kun commented Apr 26, 2019

Also, where are the changes to the swagger client generation?

Here they are: https://github.com/kubeflow/pipelines/blob/dfca0f208ee5cdfa27ae97e44b1c653a4ef65988/sdk/python/build_kfp_server_api_package.sh

@Ark-kun Ark-kun changed the title [WIP]SDK - Separated the generated api client package SDK - Separated the generated api client package Apr 26, 2019
Copy link
Contributor

@kevinbache kevinbache left a comment

Choose a reason for hiding this comment

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

Good start

sdk/python/build.sh Show resolved Hide resolved
sdk/python/build_kfp_server_api_package.sh Outdated Show resolved Hide resolved
sdk/python/build_kfp_server_api_package.sh Outdated Show resolved Hide resolved
sdk/python/build_kfp_server_api_package.sh Outdated Show resolved Hide resolved
sdk/python/kfp/_client.py Outdated Show resolved Hide resolved
sdk/python/kfp/_client.py Outdated Show resolved Hide resolved
sdk/python/setup.py Outdated Show resolved Hide resolved
@Ark-kun Ark-kun force-pushed the SDK---Separated-the-generated-api-client-package branch from 4abe0a5 to d9fa012 Compare April 27, 2019 00:35
Copy link
Contributor

@kevinbache kevinbache left a comment

Choose a reason for hiding this comment

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

/lgtm pending == -> >= in the setup.py script

sdk/python/setup.py Outdated Show resolved Hide resolved
@kevinbache
Copy link
Contributor

/lgtm; package version update note needs to go into the release playbook

@kevinbache
Copy link
Contributor

/lgtm

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Apr 29, 2019

package version update note needs to go into the release playbook

Sure. I'll add it.

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Apr 29, 2019

/approve

@@ -0,0 +1,73 @@
#!/bin/bash -e
Copy link
Member

Choose a reason for hiding this comment

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

can this be put in sdk/ folder? backend/api is intend to be language agnostic. all language specific clients are currently located in the client folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's where I put it initially...

@IronPan
Copy link
Member

IronPan commented Apr 29, 2019

/lgtm

@IronPan
Copy link
Member

IronPan commented Apr 29, 2019

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 467adb5 into kubeflow:master Apr 29, 2019
@Ark-kun Ark-kun deleted the SDK---Separated-the-generated-api-client-package branch April 29, 2019 22:54
@gaoning777
Copy link
Contributor

How often is the api sdk released? is it going to be the same as the pipeline release?
If so, Could we build the api sdk in the sample test to make sure samples work for the current SDK?

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