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

Remove asyncio event loop patch on Windows #36

Merged

Conversation

jrbourbeau
Copy link
Member

@jrbourbeau jrbourbeau commented Feb 11, 2020

This PR proposes we remove the patch added in #32 which sets the event loop policy to asyncio.WindowsSelectorEventLoopPolicy() on Windows. This causes a difference in behavior depending on if tornado is installed from conda-forge, from PyPI, or from source.

The tornado developers decided to explicitly not set the event loop policy (xref tornadoweb/tornado#2608 (comment)) and added a notice to their documentation on how users may handle the situation (xref tornadoweb/tornado#2686).

cc @msarahan

Checklist

  • Used a fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@jakirkham
Copy link
Member

cc @mattpap

@jakirkham
Copy link
Member

While I understand the motivation here, patching packages is something downstream packager like Linux Distros and conda-forge have the option to do. It's used sparingly, but when needed it is a powerful tool that helps our users.

Usually we try to send patches upstream as well (or use patches already in upstream), which helps close the gap over time. Perhaps we are dropping the ball on that in the case of tornado? In which case, maybe the relevant parties concerned with this patch should discuss and come to some agreeable solution. Then that can be submitted upstream as a patch. Thoughts? 🙂

@msarahan
Copy link
Member

The correct fix for this appears to be that every consumer package needs add this call themselves. Some of them have. It would be really helpful if you could corral the others.

I'm fine reverting this here, but this behavior will stay in Anaconda until all of the consumer packages are fixed. As system integrators, our primary concern is making things work. Making things work by satisfying everyone's intent and ironing out inconsistencies with other sources of software is gravy, but time is always lacking.

@mattpap
Copy link
Contributor

mattpap commented Feb 11, 2020

I don't have a strong opinion on this. So far my concern was primarily with broken CI tests in bokeh by the original patch. We fixed the original issue with tornado in bokeh (bokeh/bokeh#9507) locally. The exact course of action wasn't trivial to figure out, so having this fixed by the package would have been convenient. Though that's just a user's perspective.

On a side note. On tornado's master, there's this:

https://github.com/tornadoweb/tornado/blob/f7d94d0e8a57f51ffc0aac5d55ed7bf9f9936b4d/tornado/platform/asyncio.py#L311-L316

policy, I suppose to simplify things, tough one still has to use set_event_loop_policy().

@jrbourbeau
Copy link
Member Author

Thanks for the additional context and feedback @jakirkham @msarahan @mattpap

The particular package I work on uses a custom event loop policy. It actually uses the AnyThreadEventLoopPolicy policy in tornado that @mattpap pointed out. The patch here ends up overriding the event loop policy to asyncio.WindowsSelectorEventLoopPolicy, instead of the AnyThreadEventLoopPolicy we set internally, and leads to test failures (e.g. https://github.com/dask/distributed/pull/3249/checks?check_run_id=438844309#step:6:5729). I think the use of custom policies is one reason why the tornado developers decided not to set the event loop policy upstream.

I personally am in favor of removing the patch altogether. Alternatively, we could add an additional check to the patch which only sets the event loop policy if the current policy is set to asyncio.WindowsProactorEventLoopPolicy. That's the route that's taken in Jupyter:

https://github.com/jupyter/notebook/blob/43df5af2b614088b4b297fae90a70b6505b9bf84/notebook/notebookapp.py#L1752-L1755

This should (I think) avoid overriding any custom policy loops that have been set, while also still avoiding the asyncio.WindowsProactorEventLoopPolicy policy which is incompatible with tornado.

@jrbourbeau
Copy link
Member Author

Any thoughts on how to proceed? There doesn't seem to be a strong opinion on removing the patch. I'm also happy to update this PR to only set the event loop policy when the current policy is set to asyncio.WindowsProactorEventLoopPolicy

@jakirkham
Copy link
Member

jakirkham commented Feb 13, 2020

FWICT there are no objections to dropping the patch (please let me know if I misunderstood).

Anaconda will still carry the patch either way. Though I'm guessing if this causes issues for Dask + Distributed, they will need to handle that somehow.

Bokeh has already handled this another way. So don't need this any more.

This causes problems for Dask + Distributed. So really needs to be dropped.

Given the state of things and the low likelihood of getting this patch upstreamed, my inclination is drop the patch. In other words merge this PR.

Thoughts @msarahan @mattpap? 🙂

@msarahan msarahan merged commit 43e5b42 into conda-forge:master Feb 13, 2020
@jrbourbeau jrbourbeau deleted the remove-asyncio-windows-patch branch February 13, 2020 16:40
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.

5 participants