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

Update scripts to generate sdk for all frameworks #1389

Merged
merged 3 commits into from
Sep 28, 2021
Merged

Update scripts to generate sdk for all frameworks #1389

merged 3 commits into from
Sep 28, 2021

Conversation

Jeffwan
Copy link
Member

@Jeffwan Jeffwan commented Aug 29, 2021 •
edited

Scripts to generate sdk for all frameworks

@jinchihe @andreyvelich @kubeflow/wg-training-leads

@Jeffwan Jeffwan mentioned this pull request Aug 29, 2021
Closed
andreyvelich
andreyvelich reviewed Aug 30, 2021
View reviewed changes
hack/python-sdk/main.go Outdated Show resolved Hide resolved
hack/python-sdk/main.go
Comment on lines +46 to +63
switch framework {
case "tensorflow":
oAPIDefs = tfjob.GetOpenAPIDefinitions(func(name string) spec.Ref {
return spec.MustCreateRef("#/definitions/" + common.EscapeJsonPointer(swaggify(name, framework)))
})
case "pytorch":
oAPIDefs = pytorchJob.GetOpenAPIDefinitions(func(name string) spec.Ref {
return spec.MustCreateRef("#/definitions/" + common.EscapeJsonPointer(swaggify(name, framework)))
})
case "mxnet":
oAPIDefs = mxJob.GetOpenAPIDefinitions(func(name string) spec.Ref {
return spec.MustCreateRef("#/definitions/" + common.EscapeJsonPointer(swaggify(name, framework)))
})
case "xgboost":
oAPIDefs = xgboostJob.GetOpenAPIDefinitions(func(name string) spec.Ref {
return spec.MustCreateRef("#/definitions/" + common.EscapeJsonPointer(swaggify(name, framework)))
})
}
Copy link
Member

Choose a reason for hiding this comment

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

@kubeflow/wg-training-leads Do we want to have to have multiple or one Swagger for each framework?

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean compress all swagger into a big one?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I am just thinking what is the best way to locate these swaggers.

Copy link
Member Author

@Jeffwan Jeffwan Sep 6, 2021 •
edited

Choose a reason for hiding this comment

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

We still need to generate each for different framework. Then I mixin into one and use it to generate SDK.

hack/python-sdk/main.go
Comment on lines -40 to -42
oAPIDefs := tfjob.GetOpenAPIDefinitions(func(name string) spec.Ref {
return spec.MustCreateRef("#/definitions/" + common.EscapeJsonPointer(swaggify(name)))
})
Copy link
Member

Choose a reason for hiding this comment

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

@Jeffwan We might need to generate OpenAPI Definition for our APIs to avoid corev1 definition as you mentioned here: #1389 (comment).
Then, our Swagger will have only Training related APIs.
What do you think ?

Copy link
Member Author

@Jeffwan Jeffwan Aug 30, 2021 •
edited

Choose a reason for hiding this comment

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

@andreyvelich

Yeah. I think this is the problem. Not sure what corev1 are populated. Do you have any ideas? Seems something is misconfigured in this hack/python-sdk/main.go?

Copy link
Member

Choose a reason for hiding this comment

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

Do we need to use code-generator to generate it ? Similar to this: https://github.com/kubeflow/katib/blob/master/hack/update-openapigen.sh#L49-L53.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. Same tool. I update the --input-dirs to exclude those unrelated files. Thanks @andreyvelich

@jinchihe
Copy link
Member

jinchihe commented Sep 2, 2021

@Jeffwan are you want to copy the script to each framework repo? Personally I think we should generate swagger file for each repo.
Others LGTM.

@Jeffwan
Slim OpenAPI definitions by removing unrelated api
f298703 
openapi-gen input-dirs includes a few k8s.io/api and k8s.io/apimachinery packages which makes openapi_generated.go very large. Swagger files created by them are large as well.
This change help remove unnecessary references.

Signed-off-by: Jiaxin Shan <seedjeffwan@gmail.com>
@Jeffwan
Update OpenAPI generation scripts
c28712a 
1. Update gen-sdk.sh and main.go to support all frameworks
2. Generate swagger.json for all frameworks

Signed-off-by: Jiaxin Shan <seedjeffwan@gmail.com>
@Jeffwan
Merge swagger files into one
893acfb 
The swagger.json can not be merged smartly. I manually update info.description, info.title in hack/python-sdk/swagger.json

Signed-off-by: Jiaxin Shan <seedjeffwan@gmail.com>
@alembiewski
Copy link
Member

The PR hasn't been updated for quite some time now. Are there any plans to finalize and merge this one?

@Jeffwan
Copy link
Member Author

Jeffwan commented Sep 21, 2021

@alembiewski Hi, I am kind of overwhelmed recently for some internal work. If anyone has time, feel free to pick it up. otherwise, I will find some time to make it.

@alembiewski
Copy link
Member

@Jeffwan, trying to understand what needs to be done in terms of tooling here. I was able to generate Python models using the script, provided in this PR. I could start working on updating/adding the models to sdk/python and update the existing API clients (we should probably copy the pytorch_job client here as well) to operate with the new models, updating the tests etc. Does that sound good?

@alembiewski alembiewski mentioned this pull request Sep 22, 2021
Merged
@Jeffwan Jeffwan changed the title [WIP] Update scripts to generate sdk for all frameworks Update scripts to generate sdk for all frameworks Sep 24, 2021
@Jeffwan
Copy link
Member Author

Jeffwan commented Sep 24, 2021

@alembiewski Can you help review this change as well? I don't have further changes for this PR.

@alembiewski
Copy link
Member

Thanks for updating the tooling, @Jeffwan! LGTM!

@Jeffwan
Copy link
Member Author

Jeffwan commented Sep 25, 2021

@andreyvelich Alex has used update tools to generate sdk in #1420. Please help double check if further changes are needed.

@andreyvelich
Copy link
Member

/lgtm

@Jeffwan
Copy link
Member Author

Jeffwan commented Sep 28, 2021

/approve

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jeffwan

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

@google-oss-robot google-oss-robot merged commit f1b2b13 into kubeflow:master Sep 28, 2021
@Jeffwan Jeffwan mentioned this pull request Sep 29, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreyvelich andreyvelich andreyvelich left review comments

@jinchihe jinchihe Awaiting requested review from jinchihe

@terrytangyuan terrytangyuan Awaiting requested review from terrytangyuan

Assignees

@andreyvelich andreyvelich

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@Jeffwan @jinchihe @alembiewski @andreyvelich @google-oss-robot