-
Notifications
You must be signed in to change notification settings - Fork 706
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
Update scripts to generate sdk for all frameworks #1389
Conversation
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))) | ||
}) | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
oAPIDefs := tfjob.GetOpenAPIDefinitions(func(name string) spec.Ref { | ||
return spec.MustCreateRef("#/definitions/" + common.EscapeJsonPointer(swaggify(name))) | ||
}) |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
@Jeffwan are you want to copy the script to each framework repo? Personally I think we should generate swagger file for each repo. |
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 <[email protected]>
1. Update gen-sdk.sh and main.go to support all frameworks 2. Generate swagger.json for all frameworks Signed-off-by: Jiaxin Shan <[email protected]>
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 <[email protected]>
The PR hasn't been updated for quite some time now. Are there any plans to finalize and merge this one? |
@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. |
@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 |
@alembiewski Can you help review this change as well? I don't have further changes for this PR. |
Thanks for updating the tooling, @Jeffwan! LGTM! |
@andreyvelich Alex has used update tools to generate sdk in #1420. Please help double check if further changes are needed. |
/lgtm |
/approve |
[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 |
Scripts to generate sdk for all frameworks
@jinchihe @andreyvelich @kubeflow/wg-training-leads