Skip to content
This repository was archived by the owner on Jul 10, 2024. It is now read-only.

SUBMARINE-516. Automatically generate python API from swagger#314

Closed
pingsutw wants to merge 13 commits intoapache:masterfrom
pingsutw:SUBMARINE-516
Closed

SUBMARINE-516. Automatically generate python API from swagger#314
pingsutw wants to merge 13 commits intoapache:masterfrom
pingsutw:SUBMARINE-516

Conversation

@pingsutw
Copy link
Copy Markdown
Member

@pingsutw pingsutw commented Jun 17, 2020

What is this PR for?

Generate Python SDK in one command /dev-support/pysubmarine/gen-sdk.sh

  1. could we merge SUBMARINE-526. Use yapf to format Python code #312 firstly, the code generated by swagger has some code style issues.
  2. I will add an abstract class for current client API in the next JIRA so that users can more easily use this API.

Here is a Jupyter example

What type of PR is it?

[Improvement]

Todos

  • - Task

What is the Jira issue?

https://issues.apache.org/jira/browse/SUBMARINE-516

How should this be tested?

https://travis-ci.org/github/pingsutw/hadoop-submarine/builds/699154337

Screenshots (if appropriate)

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? No

Copy link
Copy Markdown
Member

@xunliu xunliu left a comment

Choose a reason for hiding this comment

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

@pingsutw Thank you contribution this feature.
I have a problem. Now I have made a lot of changes to pysubmarine and swagger, but I have not seen the corresponding test cases.
If necessary, I think it is possible to add PR and increase test coverage in travis. I am worried that the changes are frequent and will cause problems.

Comment on lines +60 to +75
'1i# Licensed to the Apache Software Foundation (ASF) under one or more\
# contributor license agreements. See the NOTICE file distributed with\
# this work for additional information regarding copyright ownership.\
# The ASF licenses this file to You under the Apache License, Version 2.0\
# (the "License"); you may not use this file except in compliance with\
# the License. You may obtain a copy of the License at\
#\
# http://www.apache.org/licenses/LICENSE-2.0\
#\
# Unless required by applicable law or agreed to in writing, software\
# distributed under the License is distributed on an "AS IS" BASIS,\
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.\
# See the License for the specific language governing permissions and\
# limitations under the License.\
'\
"$filename"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it would be better to put the license header in a license-header.txt file and then add this license-header.txt file context to the generated file.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

great, let's do it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done

@tangzhankun
Copy link
Copy Markdown
Contributor

@pingsutw , the #312 has been merged. Please go ahead.

Copy link
Copy Markdown
Contributor

@tangzhankun tangzhankun left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! @pingsutw

@@ -0,0 +1,363 @@
{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm wondering do we need to maintain this openapi.json file since we have the automation script to generate it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

should we remove it?
Maybe someone will want to check openapi.json structure, and they don't want to build the project by themself. (currently, if we want to generate openapi.json, we need to build project firstly)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's keep it then.

" spec={\"Worker\": worker_spec}) \n"
]
},
{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The response is outdated and needs an update.

"            'spec': {'librarySpec': {'cmd': 'python '\n",
       "                                            '/var/tf_mnist/mnist_with_summaries.py '\n",
       "                                            '--log_dir=/train/log '\n",
       "                                            '--learning_rate=0.01 '\n",
       "                                            '--batch_size=150',\n",
       "                                     'envVars': {'ENV1': 'ENV1'},\n",
       "                                     'image': 'gcr.io/kubeflow-ci/tf-mnist-with-summaries:1.0',\n",
       "                                     'name': 'tensorflow',\n",
       "                                     'version': '2.1.0'},\n",

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks, I've updated it

@pingsutw
Copy link
Copy Markdown
Member Author

@liuxunorg @tangzhankun Thanks for the review.

I have a problem. Now I have made a lot of changes to pysubmarine and swagger, but I have not seen the corresponding test cases.
If necessary, I think it is possible to add PR and increase test coverage in travis. I am worried that the changes are frequent and will cause problems.

Make sense
I have test SDK client locally, it works well.
Swagger doesn't generate test code, I will add the unit test and integration test in separate Jira.

@tangzhankun
Copy link
Copy Markdown
Contributor

LGTM. Will commit it if no more comments

@asfgit asfgit closed this in 0f281ca Jun 18, 2020
@pingsutw
Copy link
Copy Markdown
Member Author

Thanks for the review.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants