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] asyncio support #6245

Merged
merged 7 commits into from
Aug 14, 2017
Merged

[python] asyncio support #6245

merged 7 commits into from
Aug 14, 2017

Conversation

toumorokoshi
Copy link
Contributor

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master for non-breaking changes and 3.0.0 branch for breaking (non-backward compatible) changes.

Description of the PR

This is a pr for #5939. It introduces library variants for the Python client, and adds the 'asyncio' client.

The asyncio client is strictly Python3.5+ only.

@wing328 @frol @masterandrey

@toumorokoshi toumorokoshi changed the title Asyncio [python] asyncio support Aug 5, 2017
.gitignore Outdated
@@ -164,3 +164,4 @@ samples/client/petstore/typescript-node/npm/npm-debug.log
# aspnetcore
samples/server/petstore/aspnetcore/.vs/
effective.pom
\?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

when running run-in-docker.sh, this directory continued to get populated with jars.

@@ -202,6 +209,16 @@ public void processOpts() {
supportingFiles.add(new SupportingFile("git_push.sh.mustache", "", "git_push.sh"));
supportingFiles.add(new SupportingFile("gitignore.mustache", "", ".gitignore"));
supportingFiles.add(new SupportingFile("travis.mustache", "", ".travis.yml"));

if ("asyncio".equals(getLibrary())) {
Copy link
Contributor Author

@toumorokoshi toumorokoshi Aug 5, 2017

Choose a reason for hiding this comment

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

I was trying to replicate behavior I saw in the java client, where you can choose the library and the subdirectory seemingly populated from the resource directory with the same name.

If there's a better pattern here, I'd love to know.

# prerequisite: setuptools
# http://pypi.python.org/pypi/setuptools

REQUIRES = ["aiohttp", "urllib3", "six >= 1.10", "certifi", "python-dateutil"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

aside from the rest.py, the other changes were fairly minor to the files themselves. I'm happy to switch this over to a template conditional for the other cases.

@toumorokoshi
Copy link
Contributor Author

Some notes:

  1. as requested, coded against the 2.3.0 branch.
  2. I was able to keep the surgery pretty minor: the rest api client had to be modified significantly, but the changes to the other files were minimal. 3 files modified total.
  3. the asyncio client uses (aiohttp)[http://aiohttp.readthedocs.org/] as a core, and enables a common session / connection pool to be used per api class.

@wing328 wing328 changed the base branch from 2.3.0 to master August 5, 2017 16:33
@wing328 wing328 added this to the v2.3.0 milestone Aug 5, 2017
@wing328
Copy link
Contributor

wing328 commented Aug 5, 2017

We've released 2.2.3 about 2 weeks ago and latest master is now 2.3.0 (I've updated the PR to target master)

@wing328
Copy link
Contributor

wing328 commented Aug 5, 2017

cc @taxpon @scottrw93

@wing328
Copy link
Contributor

wing328 commented Aug 5, 2017

Travis CI failed with the following errors:

checkOptionsHelp(io.swagger.codegen.python.PythonClientOptionsTest)  Time elapsed: 0.015 sec  <<< FAILURE!

java.lang.AssertionError: These options weren't checked: library.

checkOptionsProcessing(io.swagger.codegen.python.PythonClientOptionsTest)  Time elapsed: 0.24 sec  <<< FAILURE!
mockit.internal.MissingInvocation: 
Missing 1 invocation to:
io.swagger.codegen.DefaultCodegen#setLibrary(String)
   with arguments: "urllib3"
   on mock instance: io.swagger.codegen.languages.PythonClientCodegen@261b6c8c
	at io.swagger.codegen.python.PythonClientOptionsTest.setExpectations(PythonClientOptionsTest.java:28)
Results :
Failed tests: 
  PythonClientOptionsTest>AbstractOptionsTest.checkOptionsHelp:44 These options weren't checked: library.
  PythonClientOptionsTest.checkOptionsProcessing » MissingInvocation Missing 1 i...

Please update the following files to fix it:
https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/test/java/io/swagger/codegen/python/PythonClientOptionsTest.java
https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/test/java/io/swagger/codegen/options/PythonClientOptionsProvider.java

@frol
Copy link
Contributor

frol commented Aug 6, 2017

@toumorokoshi I thought the main point of using asyncio would be an async implementation, so users can work with the API client asynchronously, but the implementation ends up to be synchronous due to awaits here and there. How much sense does it make?

Also, the asyncio/api_client.mustache differs from the original template only in two lines adding async/await prefixes. I think it might be easier to maintain one template with conditional async/await prefixes instead of two copies.

@toumorokoshi
Copy link
Contributor Author

toumorokoshi commented Aug 6, 2017

@frol the modification I did enables asynchronous programming with asyncio.

The pattern I implemented was the coroutine-based pattern, rather than callback. For asyncio, it's the more common programming abstraction.

https://docs.python.org/3/library/asyncio-task.html

async/await functions are still asynchronous, in the sense that I/O tasks do not block the thread of execution. Instead, they return control back to the main event loop. In the context of Asyncio, asynchronous I/O is the expectation, rather than a completely asynchronous processing pipeline.

coroutines return futures, and are able to still provide the parallelism one would expect for any callback-based functionality, such as chaining callbacks:

https://docs.python.org/3/library/asyncio-task.html#asyncio.Future.add_done_callback

or executing tasks in parallel:

https://docs.python.org/3/library/asyncio-task.html#example-parallel-execution-of-tasks

re the template: sounds good, I can make that change.

@frol
Copy link
Contributor

frol commented Aug 6, 2017

Well, everything you say is true, but it sounds like you think about the use cases for the non-async code base while I think about async usage:

async def my_async_func():
    pets = pets_api.get_pets()  # Async non-blocking call

    # Do something else here while the request is processed asynchronously.

    for pet in await pets:
        print(pet)

@toumorokoshi
Copy link
Contributor Author

@frol that pattern you're referring to is possible with coroutines: it doesn't really change the behavior.

Here's working example code, showing that the pet coroutine starts before the work is done:

import asyncio
from petstore_api.apis.pet_api import PetApi


pet_api = PetApi()

async def start_pet_coroutine():
    """
    wrap this, to show the I/O starts before the work is
    performed.
    """
    print("coroutine started!")
    result = await pet_api.get_pet_by_id(3)
    return result

async def test_api():
    # ensure_future schedules the future.
    pet = asyncio.ensure_future(start_pet_coroutine())

    # regardless of how work is scheduled into your event loop,
    # you will need to await to start that Task first.
    #
    # If you start doing your cpu work without awaiting first,
    # The event loop never has a chance to start those tasks.
    # as such, you need at least a asyncio.sleep to start new tasks.

    # this call enables the api work to be scheduled.
    await asyncio.sleep(0.1)

    # insert CPU-intensive work here.
    print("do work")

    # do whatever with the result, when it's available
    pet_result = await pet
    print(pet_result)

loop = asyncio.get_event_loop()
loop.run_until_complete(test_api())

prints:

coroutine started!
do work
{'category': {'id': 0, 'name': 'string'},
 'id': 3,
 'name': 'doggie',
 'photo_urls': ['string'],
 'status': 'available',
 'tags': [{'id': 0, 'name': 'string'}]}

Please note the comments: it explains some nuances around asyncio that you'll run into, regardless of your choice of abstraction.

@frol
Copy link
Contributor

frol commented Aug 8, 2017

@toumorokoshi I haven't worked with asyncio code-bases, but the proposed snippet looks overcomplicated to me. We end up with asyncio implementation which is neither native for "classic" Python nor for asyncio. Is there any benefit in it comparing to the AbstractEventLoop.run_in_executor (this will work even with "classic" requests):

import asyncio
from petstore_api.apis.pet_api import PetApi


pet_api = PetApi()

async def test_api():
    loop = asyncio.get_event_loop()
    pet = loop.run_in_executor(None, pet_api.get_pet_by_id, 3)

    await asyncio.sleep(0.1)

    # insert CPU-intensive work here.
    print("do work")

    pet_result = await pet
    print(pet_result)

loop = asyncio.get_event_loop()
loop.run_until_complete(test_api())

P.S. As I mentioned, I don't have much experience with asyncio, so my judgment can be irrelevant. I know that run_in_executor ends up running the code in a separate thread, which is just a convenient workaround for the use-cases when you need to run synchronous code inside asynchronous, but it has quite an overhead, and I don't know how "ensure_future" operates.

@toumorokoshi
Copy link
Contributor Author

@wing328 your example is uncommon for asyncio usage.

Applications that utilize asyncio tend to not run anything in a threadpool of any kind. This is because in Python, unlike some languages like Java, threading does not result in taking advantage of multiple CPUs. This is because Python has a Global Interpreter Lock. Thus, multithreading only improves the performance of native c functions bound to Python, and for performing I/O in parallel.

asyncio enables non-blocking I/O within a single thread. As such, most asyncio applications utilize no threading whatsoever, and use multiprocessing to consume multiple CPUs.

If someone did implement that code as written, it's an anti-pattern for performant asyncio. threads consume a significantly larger amount of memory than asyncio's lightweight threads. In addition, the context switches of threads is much more expensive than asyncio.

ensure_future effectively accomplishes the asyncio equivalent to run_in_executor: begins the future, which starts the I/O, and returns control back to the event loop to continue execution of the code at hand.

I appreciate the feedback! But as you mentioned, I think you're thinking of use cases for asyncio as someone who doesn't have a lot of experience with it, and is coming from a parallelism-by-threading background. I'm pretty confident in my understand of asyncio's best practices (I'm pretty invested in it), so I hope you'll defer to my expertise in this case.

@wing328
Copy link
Contributor

wing328 commented Aug 9, 2017

@toumorokoshi I think you meant to tag @frol in your reply.

@toumorokoshi
Copy link
Contributor Author

woops! My apologies. I must have been looking at the wrong place. Thank you for correcting!

@wing328
Copy link
Contributor

wing328 commented Aug 9, 2017

@toumorokoshi that's ok. Btw, very good discussion 👍

clientCodegen.setPackageUrl(PythonClientOptionsProvider.PACKAGE_URL_VALUE);
// clientCodegen.setLibrary(PythonClientCodegen.DEFAULT_LIBRARY);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this line stay here?

@frol
Copy link
Contributor

frol commented Aug 9, 2017

OK, I don't have any other comments, let's merge asyncio/api_client.mustache into ./api_client.mustache and the PR should be good to be merged into upstream.

@frol
Copy link
Contributor

frol commented Aug 14, 2017

Looks good to me!

@wing328
Copy link
Contributor

wing328 commented Aug 14, 2017

@frol thanks for reviewing the change.

@toumorokoshi thanks for the enhancement.

@wing328 wing328 merged commit d39b1ff into swagger-api:master Aug 14, 2017
@wing328
Copy link
Contributor

wing328 commented Aug 17, 2017

FYI. I've sent the following tweet to promote the new feature:

https://twitter.com/wing328/status/898103379896881152

@prathik457
Copy link

When is this PR coming to a released version of codegen?

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.

4 participants