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

[python-experimental] Quicken package loading #6437

Merged
merged 15 commits into from
Jun 1, 2020

Conversation

spacether
Copy link
Contributor

@spacether spacether commented May 26, 2020

This PR was written to resolve #6402
In this PR we have:

  • created apis and models python modules which contains all apis and models
  • individual apis and models are in the api and model packages
  • stopped loading in models when loading the package {{packageName}} for example petstore_api
  • updated all our tests to selectively only load the models that they need, so the models package is not used in samples testing
  • adds the exception ApiAttributeError into the root package namespace because it was missing
  • readme and api docs samples updated to use the .model and .api imports

@sebastien-rosset

The load time change in loading petstore_api in the v3 samples directory is:

  • master branch time to load petstore_api in python3.7.0, load_time=0.759689
  • this PR's time to load petstore_api in python3.7.0, load_time=0.40

Load time should scale directly with memory usage.
When loading the root package we should no longer see the RecursionError

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

Python technical committee:
@taxpon (2017/07) @frol (2017/07) @mbohlool (2017/07) @cbornet (2017/09) @kenjones-cisco (2017/11) @tomplus (2018/10) @Jyhess (2019/01) @arun-nalla (2019/11) @spacether (2019/11)

@auto-labeler
Copy link

auto-labeler bot commented May 26, 2020

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@spacether spacether added this to the 5.0.0 milestone May 26, 2020
@spacether
Copy link
Contributor Author

@sebastien-rosset how does this look to you?
If you use the all_models module do you still see the RecursionError?

  • If yes, do you want me to add a configuration parameter recursion_limit?

@sebastien-rosset
Copy link
Contributor

@sebastien-rosset how does this look to you?
If you use the all_models module do you still see the RecursionError?

  • If yes, do you want me to add a configuration parameter recursion_limit?

Let me try and let you know, thank you for the PR!

@sebastien-rosset
Copy link
Contributor

@sebastien-rosset how does this look to you?
If you use the all_models module do you still see the RecursionError?

If I import mypackage, the recursion depth currently reaches 983.

import mypackage

If I import mypackage.all_models, the recursion depth currently reaches 987.

import mypackage.all_models

Does this answer your question?

  • If yes, do you want me to add a configuration parameter recursion_limit?

Do you mean a codegen CLI option or a configuration parameter in the generated Configuration class? Either way, I'm not sure it would be useful.

First, the client may be part of a larger application, the part invoking the generated Python SDK may possibly be just a small piece of the entire application. This would mean potentially the generated OpenAPITools python code would change the value in the middle of the application, but isn't this better to set this global parameter at the beginning of the application? BTW, I'm not sure what setrecursionlimit() do exactly, especially in a multi-threaded application, the multi-threading behavior does not seem to be documented. What does this call do for existing threads vs new threads? Everything seems to point that code should be rewritten such that high level of recursion are not needed.

Second, the application may already have some code (or may need) to set the recursion limit. So now potentially we would have two separate lines of code setting the recursion limit, not a good idea.

If the configuration option is part of the Configuration class, isn't there going to be a chicken-and-egg problem, since the import will fail even before you can access the Configuration class? You could ask users to specifically import the Configuration class then set the recursion limit, but I feel this is very unpythonic and cumbersome. We might as well just document that users should set the recursion limit before the import; or better, somehow prevent the recursion to occur.

@spacether
Copy link
Contributor Author

spacether commented May 26, 2020

@sebastien-rosset how does this look to you?
If you use the all_models module do you still see the RecursionError?

If I import mypackage, the recursion depth currently reaches 983.

import mypackage

If I import mypackage.all_models, the recursion depth currently reaches 987.

Can you email or slack me an oas document (json or yaml) that demonstrates these recursion depths? It is very strange to me that the package import is triggering such high recursive counts.
I should be able to improve it if I have an oas document to work with.

Suspected reason that we still have high recursive counts:

  • I think that it is because apis are also imported at the package root level like from petstore_api.api.pet_api import PetApi
  • those api files include model imports

So the root import is still loading models that the apis use.
If I also remove the api imports from the root of the package level this may fix it.
Having the asked sample oas document would help here.

@spacether
Copy link
Contributor Author

spacether commented May 26, 2020

The load time change in loading petstore_api in the v3 samples directory is now:

  • master branch time to load petstore_api in python3.7.0, load_time=0.759689
  • this PR's time to load petstore_api in python3.7.0, load_time=0.403240 (updated, apis not loaded in when the package is imported)

Next up: checking it with the larger sample spec.

@spacether
Copy link
Contributor Author

@sebastien-rosset when I load in the library created with your sample spec, and my improvement to remove the apis from the root of the package I find:

  • load_time is ~ 0.4 - 0.5 seconds when inside a python3 virtual environment
  • I do not get the RecursionError
  • The recursion depth that see when loading the package is now 126

@spacether
Copy link
Contributor Author

If the configuration option is part of the Configuration class, isn't there going to be a chicken-and-egg problem, since the import will fail even before you can access the Configuration class? You could ask users to specifically import the Configuration class then set the recursion limit, but I feel this is very unpythonic and cumbersome. We might as well just document that users should set the recursion limit before the import; or better, somehow prevent the recursion to occur.

Yes, I meant a parameter inside our python Configuration class. Good point, setting that parameter would impact all imports in the program so we probably shouldn't do that.
I have a comment in all_models describing what the user can do to fix the recursion depth error.
Does that meet your needs or do you think that there is a better way to convey this to the user?

@spacether
Copy link
Contributor Author

TODO:

  • create all_apis like all_models
  • remove the api imports into the root of the apis folder
  • this will let users load in just one api and its related models without having to load in all apis and related models

@sebastien-rosset
Copy link
Contributor

If the configuration option is part of the Configuration class, isn't there going to be a chicken-and-egg problem, since the import will fail even before you can access the Configuration class? You could ask users to specifically import the Configuration class then set the recursion limit, but I feel this is very unpythonic and cumbersome. We might as well just document that users should set the recursion limit before the import; or better, somehow prevent the recursion to occur.

Yes, I meant a parameter inside our python Configuration class. Good point, setting that parameter would impact all imports in the program so we probably shouldn't do that.
I have a comment in all_models describing what the user can do to fix the recursion depth error.
Does that meet your needs or do you think that there is a better way to convey this to the user?

How about editing the `README_common.mustache file? I think users go to the README file first. If the script fails on first import, most likely they would go to README for help, I doubt many users will bother looking elsewhere.

If the OpenAPI model is large, the import may fail with an error indicating the maximum recursion limit has been exceeded.
In that case, you may need to adjust the maximum recursion limit as shown below:

import sys
sys.setrecursionlimit(1500)
import {{{packageName}}}

@spacether
Copy link
Contributor Author

Note to self:

  • move all models into the model folder
  • change all_models into models
  • do the same for api and apis

@spacether spacether force-pushed the create_allmodels branch 2 times, most recently from 3f25a51 to 37b5ff9 Compare May 29, 2020 04:59
@spacether
Copy link
Contributor Author

spacether commented May 29, 2020

Note to self: why are the model imports missing from the operations?
Update: this has been fixed

@spacether
Copy link
Contributor Author

TODO: update v3 sample model tests to include the sys import

@spacether spacether closed this May 30, 2020
@spacether spacether reopened this May 30, 2020
@spacether spacether closed this May 30, 2020
@spacether spacether reopened this May 30, 2020
@spacether spacether closed this May 31, 2020
@spacether spacether reopened this May 31, 2020
@spacether spacether closed this Jun 1, 2020
@spacether spacether reopened this Jun 1, 2020
@spacether
Copy link
Contributor Author

The drone CI error is unrelated to this change, merging it in

@spacether spacether merged commit d8c4223 into OpenAPITools:master Jun 1, 2020
@spacether spacether deleted the create_allmodels branch June 1, 2020 07:25
jimschubert added a commit that referenced this pull request Jun 2, 2020
* master:
  Update Generate.java (#6515)
  Undo PR #6451 (#6514)
  Minor enhancement to Python client generator's code format (#6510)
  [python-experimental] Quicken package loading (#6437)
  [Python][Client] Fix delimiter collision issue #5981 (#6451)
  [Java][Jersey2] add petstore integration tests (#6508)
  UE4 client generator fixes (#6438)
  Fix docs typos (#6478)
  [php-laravel] Show required PHP version in docs (#6502)
  [php-lumen] Show required PHP version in docs (#6501)
  [Java][Jersey2] Fix typo and script, Log enhancements, HTTP signature, deserialization (#6476)
  Remove deprecations 5.0 (#6060)
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.

[python-experimental] loading package cause maximum recursion depth error
2 participants