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

Port greenio to Trollius #5

Closed
wants to merge 14 commits into from
Closed

Port greenio to Trollius #5

wants to merge 14 commits into from

Conversation

vstinner
Copy link
Collaborator

@vstinner vstinner commented Jul 3, 2014

Hi,

I would like to use greenio in OpenStack to be able to use the asyncio API be execute tasks in greenlet. OpenStack projects are heavily based on eventlet and it's not possible to switch from eventlet to asyncio in one step, it should be done incrementally.

asyncio and Python 3 cannot be requirements on OpenStack because OpenStack is already deployed in production with Python 2, and projects don't support Python 3 yet (most clients are Python 3 compatibility, but not servers yet).

My plan is to use Trollius with greenio, replace incrementally eventlet with Trollius in OpenStack:
http://haypo-notes.readthedocs.org/openstack.html#roadmap

Victor

@vstinner
Copy link
Collaborator Author

vstinner commented Jul 3, 2014

To test greenio on Trollius, you need to use the branch trollius_greenio which as a tasks.task_factory variable. I just sent an email to ask to add a new BaseEventLoop.task_factory attribute to python-tulip mailing list.

@greenio.task
def test():
return (yield from foo())
if trollius is not None:
Copy link

Choose a reason for hiding this comment

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

I understand the need to do this, but it still feels very wrong :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, can you elaborate? What's "wrong"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the need: "yield from" syntax raises a SyntaxError on Python 2, you just cannot import the module.

It may be possible to use different files, but I prefer to keep all code in a single code base.

Copy link

Choose a reason for hiding this comment

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

I understand that, that's the part that feels wrong, big #ifdef trollius around blocks of code.

But I guess this is how it goes....

@1st1
Copy link
Owner

1st1 commented Jul 4, 2014

After talking with Mark McLoughin, I see an issue by importing asyncio first. On Python 3.4, asyncio will always be used. In this case, an application cannot use Trollius coroutines with greenio, because supporting Trollius and asyncio coroutines is only supported by Trollius Task class. asyncio Task class only supports asyncio coroutines.

So would you be ok to import Trollius first in greenio for these reasons?

@Haypo I see the reason. How about a rather obscure, but still working solution: before trying to import asyncio we check if trollius is in sys.modules?

@vstinner
Copy link
Collaborator Author

vstinner commented Jul 4, 2014

@Haypo I see the reason. How about a rather obscure, but still working solution: before trying to import asyncio we check if trollius is in sys.modules?

My asyncio patch adding task_factory is under review. I started with asyncio.tasks.task_factory, which is the option chosen in this pull request. I then moved the factory to BaseEventLoop. But Guido van Rossum proposed to move the factory to the event loop policy.

If the task factory is in the event loop policy, we can offer two event loop policies:

  • GreenAsyncioEventLoopPolicy: available if the asyncio module is available
  • GreenTrolliusEventLoopPolicy: available if the trollius module is available

So it puts less pressure on the import part :-) I didn't check which parts should be specific to trollius or asyncio. There is a least the GreenTask class which should inherit from asyncio.Task or from trollius.Task.

I will update this pull request when the API for task_factory will be decided. FYI the review:
https://codereview.appspot.com/110820049/

@vstinner
Copy link
Collaborator Author

vstinner commented Jul 8, 2014

The task factory has been implemented in Tulip: it's a new nice BaseEventLoop.create_task() method, instead of an ugly global task_factory variable. I merged Tulip into Trollius and updated the pull request.

@1st1
Copy link
Owner

1st1 commented Jul 8, 2014

@Haypo Thanks Victor. I'll review your PR in detail shortly.

@vstinner
Copy link
Collaborator Author

vstinner commented Jul 9, 2014

@1st1 : Oh, I forgot to fix the "import order" issue: provide 2 event loop policies (asyncio & trollius), not only one.

@vstinner
Copy link
Collaborator Author

vstinner commented Jul 9, 2014

Ok, I think that the code is now ready for a review. Let me explain the whole change:

  • the goal is to support Trollius
  • in the new version of the patch, if the trollius module is missing: the code for asyncio does not change
  • yield_from() now also accepts coroutine objects. To support asyncio and trollius using the same code base, you must not create directly Task or GreenTask object, but use the new loop.create_task() function (I just added this method to Tulip and Trollius for greenio!). yield_from() now creates the task for you using the good class (GreenTask or GreenTrolliusTask depending on the event loop policy).
  • in a few places in unit tests, I had to use exec("...") to support Python 2 and Python 3 in the same code base: Python 2 raises a SyntaxError on "yield from".
  • I added test_trollius.py to ensure that greenio supports running asyncio coroutines in an trollius event loop. This test may be enhanced to test more cases. Executing a trollius coroutine in an asyncio event loop does not work because asyncio Task does not support "yield" (yield From(...)), "yield from ..." must be used instead.
  • To support Trollius, you must set the event loop policy to an instance of GreenTrolliusEventLoopPolicy ( I dislike such long name :-( ).
  • It's possible to support asyncio and trollius if you also set the asyncio and trollius event loop policies to an instance of GreenTrolliusEventLoopPolicy.

@vstinner
Copy link
Collaborator Author

vstinner commented Jul 9, 2014

I reviewed my code on Github and I found some issues. They should be fixed now.

@vstinner
Copy link
Collaborator Author

I moved Python2-specific changes in a new pull request to make this pull request simpler: #7

@vstinner
Copy link
Collaborator Author

I also created the pull request #8 for the greenio.yield_from() change to accept also coroutine objects. I will rebase the trollius branch when #7 and #8 will be merged.

@1st1
Copy link
Owner

1st1 commented Jul 15, 2014

All other PRs are merged. Please update this one.

@vstinner
Copy link
Collaborator Author

I abandon this pull request in favor of my updated pull request #9.

@vstinner vstinner closed this Jul 15, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants