Openapi generated client by MalteEbner · Pull Request #125 · lightly-ai/lightly

MalteEbner · 2021-02-11T13:29:27Z

PR consist of following steps / commits:

copied templates to generate client -> all changed files in the folder openapi_client_generator/python/
wrote script to generated the client at openapi_client_generator/generate_openapi_client.sh-> should be tested
generated the client using the script -> all changed files in the folder lightly/openapi_generated/
changed the .mustache template to allow required and nullable values to be None (e.g. the previousTagId). First the model.mustache template was changed, then the script war rerun, which caused the generated code to change.

codecov · 2021-02-11T13:34:53Z

Codecov Report

Merging #125 (bde4a85) into develop_active_learning (e9e1b3d) will decrease coverage by 0.12%.
The diff coverage is n/a.

@@                     Coverage Diff                     @@
##           develop_active_learning     #125      +/-   ##
===========================================================
- Coverage                    82.00%   81.87%   -0.13%     
===========================================================
  Files                           58       56       -2     
  Lines                         1800     1788      -12     
===========================================================
- Hits                          1476     1464      -12     
  Misses                         324      324

Impacted Files	Coverage Δ
lightly/models/__init__.py
lightly/api/__init__.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9e1b3d...bde4a85. Read the comment docs.

philippmwirth

Please correct me if I'm wrong.

In my understanding, the following points hold:

We have a part of this repo in our repo
Everytime we build the api from scratch, we also need to make changes to tag_data.py and model.mustache

If so, please make changes such that the necessary code from the other repo is not part of our repository. Instead, give instructions on how to get the required code or clone it automatically in the generator script. Also, can we apply the necessary changes to the two files as a patch or similar?

Let me know if anything is unclear.

philippmwirth · 2021-02-11T13:50:22Z

+
+# install swagger-codegen following the instructions at https://github.com/swagger-api/swagger-codegen#compatibility
+swagger-codegen generate -l python -i $PATH_TO_SPEC  -o lightly/openapi_generated  --template-dir openapi_client_generator/python --template-engine mustache
+


I don't think we should have the openapi_client_generator code as part of our repository..

There are two options which I would prefer:

Add a line to this script which clones the generator repo at a specified commit

Describe in the README how someone who wants to build the package from scratch can clone the repo and then set the path

philippmwirth · 2021-02-11T14:04:15Z

+six >= 1.10
+python_dateutil >= 2.5.3
+setuptools >= 21.0.0
+urllib3 >= 1.15.1


This introduces new requirements, they should be specified in the PIP package requirements.

philippmwirth · 2021-02-11T14:05:36Z

+            _return_http_data_only=params.get('_return_http_data_only'),
+            _preload_content=params.get('_preload_content', True),
+            _request_timeout=params.get('_request_timeout'),
+            collection_formats=collection_formats)


What do we need the annotations api for?

philippmwirth · 2021-02-11T14:08:33Z

+            _return_http_data_only=params.get('_return_http_data_only'),
+            _preload_content=params.get('_preload_content', True),
+            _request_timeout=params.get('_request_timeout'),
+            collection_formats=collection_formats)


I'm pretty sure that this code should not be public. Can you remove all the generated API code which is not being used?

philippmwirth · 2021-02-11T14:12:53Z

+        if not isinstance(other, AnnotationState):
+            return True
+
+        return self.to_dict() != other.to_dict()


All the annotation data / requests are not needed by the PIP package, or am I wrong?

philippmwirth · 2021-02-11T14:13:42Z

+        if not isinstance(other, CreateCFBucketActivityRequest):
+            return True
+
+        return self.to_dict() != other.to_dict()


This is not needed in the PIP package

philippmwirth · 2021-02-11T14:18:14Z

+{{/nullable}}
 {{/required}}
 {{#isEnum}}
 {{#isContainer}}


Ok do we have to change this everytime we generate the API code? In that case I would make an additional change in the generate_openapi_client.sh script which applies this as a patch.

MalteEbner · 2021-02-11T15:55:01Z

@philippmwirth:
Do I understand you correctly, that you basically wish the following changes?

If one generates a new python client from a new api.yml, no manual changes, to anything (neither .mustache files, nor generated files) should be necessary. This point is already fulfilled:
The change with the /nullable to the model.mustach-template only has the be made once (in this PR), it basically creates our own python code generator. If a new api is written, only the script has to be rerun, no manual changes of any code have to be performed.
We do not want that many copied files as template. Ideally, we could use the generator without customers templates. To achieve this, I create the following PR for swagger-codegen: [Python] Nullable bugfix in model.mustache swagger-api/swagger-codegen#10906
The lightly pip package should not contain any code needed for generating the client. We could solve this by putting the files in the folder openapi_client_generator into the lightly-core package. What do you think about that?
There should be as little files and code in the repo as possible. Thus all generated files not needed should be deleted.
The new requirements added should be specified in the PIP package requirements.

philippmwirth · 2021-02-11T16:54:22Z

@philippmwirth:
Do I understand you correctly, that you basically wish the following changes?

1. If one generates a new python client from a new api.yml, no manual changes, to anything (neither .mustache files, nor generated files) should be necessary. This point is already fulfilled:
   The change with the /nullable to the model.mustach-template only has the be made once (in this PR), it basically creates our own python code generator. If a new api is written, only the script has to be rerun, no manual changes of any code have to be performed.

2. We do not want that many copied files as template. Ideally, we could use the generator without customers templates. To achieve this, I create the following PR for swagger-codegen: [swagger-api/swagger-codegen#10906](https://github.com/swagger-api/swagger-codegen/pull/10906)

3. The lightly pip package should not contain any code needed for generating the client. We could solve this by putting the files in the folder `openapi_client_generator` into the lightly-core package. What do you think about that?

4. There should be as little files and code in the repo as possible. Thus all generated files not needed should be deleted.

5. The new requirements added should be specified in the PIP package requirements.

@philippmwirth:
Do I understand you correctly, that you basically wish the following changes?

1. If one generates a new python client from a new api.yml, no manual changes, to anything (neither .mustache files, nor generated files) should be necessary. This point is already fulfilled:
   The change with the /nullable to the model.mustach-template only has the be made once (in this PR), it basically creates our own python code generator. If a new api is written, only the script has to be rerun, no manual changes of any code have to be performed.

2. We do not want that many copied files as template. Ideally, we could use the generator without customers templates. To achieve this, I create the following PR for swagger-codegen: [swagger-api/swagger-codegen#10906](https://github.com/swagger-api/swagger-codegen/pull/10906)

3. The lightly pip package should not contain any code needed for generating the client. We could solve this by putting the files in the folder `openapi_client_generator` into the lightly-core package. What do you think about that?

4. There should be as little files and code in the repo as possible. Thus all generated files not needed should be deleted.

5. The new requirements added should be specified in the PIP package requirements.

Yes, this is pretty accurate. Only one thing: I don't think it would be such a good idea to put the generator in the lightly-core repo. Can we not simply clone the code and apply the changes with a patch? My guess is we could write a script which retrieves the code, applies the patch, and then generates the openapi code.

copied from https://github.com/swagger-api/swagger-codegen/tree/master/modules/swagger-codegen/src/main/resources/python

…hon client

MalteEbner requested a review from philippmwirth February 11, 2021 13:29

MalteEbner self-assigned this Feb 11, 2021

MalteEbner added the enhancement label Feb 11, 2021

philippmwirth suggested changes Feb 11, 2021

View reviewed changes

MalteEbner added 9 commits February 15, 2021 09:18

openapi_genertion: add templates

a5d70f9

copied from https://github.com/swagger-api/swagger-codegen/tree/master/modules/swagger-codegen/src/main/resources/python

Create generate_openapi_client.sh

802c4e2

openapi_client: add generated files

e7ef18c

openapi_generator: allow nullable required

743eab7

deleted openapi_client_generator (now in lightly-core)

440194b

deleted client generated with old script

4a5c35e

include generated python client

dffd475

update requirements/base.txt to include the ones of the generated pyt…

f430ac1

…hon client

Bugfix in requirements/base.txt

bde4a85

MalteEbner force-pushed the openapi_generated_client branch from 24f6766 to bde4a85 Compare February 15, 2021 08:19

MalteEbner marked this pull request as draft February 15, 2021 08:55

MalteEbner closed this Feb 15, 2021

MalteEbner deleted the openapi_generated_client branch February 15, 2021 10:05

Navigation Menu

Uh oh!

Openapi generated client - #125

Openapi generated client#125
MalteEbner wants to merge 9 commits into
develop_active_learningfrom
openapi_generated_client

MalteEbner commented Feb 11, 2021 •

edited

Loading

Uh oh!

codecov Bot commented Feb 11, 2021 •

edited

Loading

Uh oh!

philippmwirth left a comment

Uh oh!

philippmwirth Feb 11, 2021

Uh oh!

philippmwirth Feb 11, 2021

Uh oh!

philippmwirth Feb 11, 2021

Uh oh!

philippmwirth Feb 11, 2021

Uh oh!

philippmwirth Feb 11, 2021

Uh oh!

philippmwirth Feb 11, 2021

Uh oh!

philippmwirth Feb 11, 2021

Uh oh!

MalteEbner commented Feb 11, 2021

Uh oh!

philippmwirth commented Feb 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		# install swagger-codegen following the instructions at https://github.com/swagger-api/swagger-codegen#compatibility
		swagger-codegen generate -l python -i $PATH_TO_SPEC -o lightly/openapi_generated --template-dir openapi_client_generator/python --template-engine mustache

Uh oh!

Conversation

MalteEbner commented Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

philippmwirth left a comment

Choose a reason for hiding this comment

Uh oh!

philippmwirth Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

philippmwirth Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

philippmwirth Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

philippmwirth Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

philippmwirth Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

philippmwirth Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

philippmwirth Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

MalteEbner commented Feb 11, 2021

Uh oh!

philippmwirth commented Feb 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MalteEbner commented Feb 11, 2021 •

edited

Loading

codecov Bot commented Feb 11, 2021 •

edited

Loading