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

Webagg backend: get rid of tornado #20591

Merged
merged 6 commits into from
Oct 21, 2021
Merged

Conversation

martinRenou
Copy link
Member

@martinRenou martinRenou commented Jul 7, 2021

PR Summary

This allows using the webagg backend in JupyterLite, by replacing tornado with asyncio. See matplotlib/ipympl#334 and https://github.com/jtpio/jupyterlite/pull/219

cc. @davidbrochart

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • [N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@jklymak jklymak added this to the v3.5.0 milestone Jul 7, 2021
@tacaswell
Copy link
Member

tacaswell commented Jul 8, 2021

This seems like a major API change? We either need replacements/backcompat shims or a very clear API change note for the classes that no longer exist.

Does this put a floor on our tornado support to versions that know howto play nice with asyncio?

@martinRenou
Copy link
Member Author

martinRenou commented Jul 9, 2021

I guess we could keep the timertornado class around? And do a lazy import of tornado if one uses it.

This way the PR would just be about adding a new timer class that doesn't need tornado, and that ipympl could use.

@anntzer
Copy link
Contributor

anntzer commented Jul 9, 2021

I don't know enough about the ecosystem (whether the jupyter side or the async side :-)) to have an answer here, but do you think anyone is actually relying on the tornado timer? Could it be reasonably deprecated? Or perhaps more mildly, moved to backend_webagg, and make backend_webagg_core a "you need to provide your own timer" abstract backend?

@martinRenou
Copy link
Member Author

Given the comments and concerns about backward compatibility, I've put back the TimerTornado.

Having the TimerTornado around is harmless in a "pyodide" context like JupyterLite as long as tornado is imported lazily, so we can definitely keep it.

@timhoffm timhoffm added this to Wishlist in v3.6 Milestone Jul 11, 2021
@jklymak jklymak marked this pull request as draft July 21, 2021 01:41
@QuLogic QuLogic modified the milestones: v3.5.0, v3.6.0 Aug 21, 2021
@stonebig
Copy link
Contributor

is there still hope on this for 3.5.0 ?

@tacaswell
Copy link
Member

@stonebig It looks like there are still a few back-compatibility changes that need to be addressed.

We have not "feature frozen" for 3.5 yet, can you give me a sense of how important / impactful getting this in would be?

@stonebig
Copy link
Contributor

For a windows user, Tornado is non-performant.
Martin writes this is also a stone to remove to improve wasm / web flight envelop of python Jupyter stack.

So my interest to push this, as I believe it will have positive effects for python on web and python on windows. Nothing more.

@tacaswell
Copy link
Member

This PR is affected by a re-writing of our history to remove a large number of accidentally committed files see discourse for details.

To recover this PR it will need be rebased onto the new default branch (main). There are several ways to accomplish this, but we recommend (assuming that you call the matplotlib/matplotlib remote "upstream"

git remote update
git checkout main
git merge --ff-only upstream/main
git checkout YOUR_BRANCH
git rebase --onto=main upstream/old_master
# git rebase -i main # if you prefer
git push --force-with-lease   # assuming you are tracking your branch

If you do not feel comfortable doing this or need any help please reach out to any of the Matplotlib developers. We can either help you with the process or do it for you.

Thank you for your contributions to Matplotlib and sorry for the inconvenience.

@tacaswell tacaswell marked this pull request as ready for review October 20, 2021 21:47
@tacaswell tacaswell modified the milestones: v3.6.0, v3.5.0 Oct 20, 2021
tornado 5 is from ~2018 hence is consistent with our support window policy.
@tacaswell tacaswell merged commit cced93b into matplotlib:main Oct 21, 2021
v3.6 Milestone automation moved this from Wishlist to Done Oct 21, 2021
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Oct 21, 2021
tacaswell added a commit that referenced this pull request Oct 21, 2021
…591-on-v3.5.x

Backport PR #20591 on branch v3.5.x (Webagg backend: get rid of tornado)
@ArchangeGabriel
Copy link
Contributor

I’m packaging matplotlib for ArchLinux and I have some questions regarding this changes:

  • the documentation (nor tests env, etc.) wasn’t updated and still says tornado is required for the webagg backend, is that correct?
  • if it’s not, in what cases is tornado still required?

@martinRenou martinRenou deleted the asyncio_timer branch November 16, 2021 18:07
@martinRenou
Copy link
Member Author

It is still needed, because the webagg backend provides a TornadoTimer class that might be used by dependencies (and by other places in Matplotlib?)

But in the long term the dependency might be removed?

Please someone correct me if I am wrong.

@tacaswell
Copy link
Member

The important change here is that backend_webagg_core no longer requires tornado. This is the module that ipympl imports and hence hiding the tornado import at that level removes an implicit tornado dependency from ipympl.

However backend_webagg.py still very much depends on tornado, the core application class is

class WebAggApplication(tornado.web.Application):
initialized = False
started = False
!

We may want to rename this as backend_webagg_tornado and add a backend_webagg_fastapi, backend_webagg_aiohttp, (or maybe make those one ones sibling projects in mpl and also eject the tornado one?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants