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

Replaced 'retrying' dependency with 'tenacity' in plotly package #2911

Merged
merged 10 commits into from
Apr 22, 2021

Conversation

jmsmdy
Copy link
Contributor

@jmsmdy jmsmdy commented Nov 17, 2020

The plotly dependency 'retrying' is no longer being maintained. The currently-maintained fork, 'tenacity', is compatible with both Python 2 and 3, and is feature-compatible (although not API compatible). This pull request changes the few instances where the retrying package is used in plotly to use tenacity instead.

One motivation for this is to get plotly working in Pyodide (a means developed by Firefox to run Python in the browser, which currently supports numpy, pandas, matplotlib, and a number of other datascience packages). 'Retrying' seems to be incompatible with Pyodide.

closes #2907

@jmsmdy
Copy link
Contributor Author

jmsmdy commented Nov 17, 2020

It looks like the orca tests are failing with "ModuleNotFoundError: No module named 'tenacity'". I assume this is only because tenacity is not available on the CI server. Not sure what to do about this!

@Alex-Monahan
Copy link

This change made our use case possible! If it could be merged at some point (no urgency!), that would be fantastic.

Thanks!
-Alex

@nicolaskruchten
Copy link
Contributor

Thanks @jmsmdy! Pyodide is so cool, I'm glad someone is excited about getting Plotly working there <3

@jonmmease this seems like a reasonable PR to merge once we get CI running green, right?

@jonmmease
Copy link
Contributor

Thanks @jmsmdy. Glad to learn that retrying lives on in tenacity.

Looks like tenacity is available on PyPI, the anaconda main channel, and conda-forge so I wouldn't consider this to be a breaking change. Happy to see this merged when tests are worked out, looks like we need to track down the dependencies used in the orca CI tests and add tenacity there as well.

@nicolaskruchten
Copy link
Contributor

OK, let's get this thing merged then! @jmsmdy can you please allow me to push to your branch? If not, can you please run black on _orca.py and replace retrying with tenacity in create_conda_optional_env.sh and push?

@jmsmdy
Copy link
Contributor Author

jmsmdy commented Nov 25, 2020

OK, let's get this thing merged then! @jmsmdy can you please allow me to push to your branch? If not, can you please run black on _orca.py and replace retrying with tenacity in create_conda_optional_env.sh and push?

@nicolaskruchten Just invited you as a collaborator on my branch, feel free to make whatever changes are necessary!

@jmsmdy
Copy link
Contributor Author

jmsmdy commented Dec 7, 2020

@nicolaskruchten I made the changes you suggested, but now the docs don't seem to be building (none of the commits I made changed the docs). Looking at the CircleCI output for build-doc, the error for an example doc build (which is the same as the error for many docs which failed), is as follows:

[NbConvertApp] Converting notebook build/ipynb/plotly-express.ipynb to html [NbConvertApp] Executing notebook with kernel: python3 Traceback (most recent call last): File "/home/circleci/project/doc/venv/bin/jupyter-nbconvert", line 8, in <module> sys.exit(main()) File "/home/circleci/project/doc/venv/lib/python3.7/site-packages/jupyter_core/application.py", line 254, in launch_instance return super(JupyterApp, cls).launch_instance(argv=argv, **kwargs) File "/home/circleci/project/doc/venv/lib/python3.7/site-packages/traitlets/config/application.py", line 845, in launch_instance app.start() File "/home/circleci/project/doc/venv/lib/python3.7/site-packages/nbconvert/nbconvertapp.py", line 350, in start self.convert_notebooks() File "/home/circleci/project/doc/venv/lib/python3.7/site-packages/nbconvert/nbconvertapp.py", line 524, in convert_notebooks self.convert_single_notebook(notebook_filename) File "/home/circleci/project/doc/venv/lib/python3.7/site-packages/nbconvert/nbconvertapp.py", line 489, in convert_single_notebook output, resources = self.export_single_notebook(notebook_filename, resources, input_buffer=input_buffer) File "/home/circleci/project/doc/venv/lib/python3.7/site-packages/nbconvert/nbconvertapp.py", line 418, in export_single_notebook output, resources = self.exporter.from_filename(notebook_filename, resources=resources) File "/home/circleci/project/doc/venv/lib/python3.7/site-packages/nbconvert/exporters/exporter.py", line 181, in from_filename return self.from_file(f, resources=resources, **kw) File "/home/circleci/project/doc/venv/lib/python3.7/site-packages/nbconvert/exporters/exporter.py", line 199, in from_file return self.from_notebook_node(nbformat.read(file_stream, as_version=4), resources=resources, **kw) File "/home/circleci/project/doc/venv/lib/python3.7/site-packages/nbconvert/exporters/html.py", line 119, in from_notebook_node return super().from_notebook_node(nb, resources, **kw) File "/home/circleci/project/doc/venv/lib/python3.7/site-packages/nbconvert/exporters/templateexporter.py", line 384, in from_notebook_node output = self.template.render(nb=nb_copy, resources=resources) File "/home/circleci/project/doc/venv/lib/python3.7/site-packages/jinja2/environment.py", line 1090, in render self.environment.handle_exception() File "/home/circleci/project/doc/venv/lib/python3.7/site-packages/jinja2/environment.py", line 832, in handle_exception reraise(*rewrite_traceback_stack(source=source)) File "/home/circleci/project/doc/venv/lib/python3.7/site-packages/jinja2/_compat.py", line 28, in reraise raise value.with_traceback(tb) File "/home/circleci/project/doc/nb.tpl", line 1, in top-level template code {%- extends 'basic.tpl' -%} jinja2.exceptions.TemplateNotFound: basic.tpl

Searching through the plotly.py repository, I can't seem to find the file basic.tpl anywhere. Could you provide some guidance for how to fix this?

@nicolaskruchten
Copy link
Contributor

I can't seem to find the file basic.tpl anywhere. Could you provide some guidance for how to fix this?

Don't worry about the doc CI failures, I think it's just that we need to pin nbconvert to 5.x on master.

@nicolaskruchten
Copy link
Contributor

Re the chart-studio package, I would indeed recommend not changing anything in there at all if possible :)

@jmsmdy
Copy link
Contributor Author

jmsmdy commented Dec 8, 2020

OK, so after rolling back changes to chart-studio, and adding some jinja template files from nbconvert to the docs folder, all tests are passing. Should I add a new commit removing the jinja template files I added to get the docs test to pass?

@nicolaskruchten
Copy link
Contributor

Hmm let me take a look at those templates! Thanks for pushing through that failure!

@jmsmdy
Copy link
Contributor Author

jmsmdy commented Dec 9, 2020

The templates were obtained from the 5.6.1 version of nbconvert, here: https://github.com/jupyter/nbconvert/tree/5d2eaed5cbba1f8dff14126c015f4fe28f5f3311/nbconvert/templates

I just searched for basic.tpl, then traced back the template imports. The templates I added are located at doc/basic.tpl, doc/display_priority.tpl, and doc/null.tpl.

I'm not sure if this is the right long-term solution.

@nicolaskruchten
Copy link
Contributor

OK so on master I've pinned nbconvert down to the version that has basic.tpl built-in, so in principle you should be able to remove these templates and rebase onto master (or merge master in) and things should still be green.

@jmsmdy
Copy link
Contributor Author

jmsmdy commented Dec 11, 2020

OK, just rebased to master and removed the (now unneeded) jinja templates that I previously added. As far as I can tell, this is ready to marge!

@nicolaskruchten
Copy link
Contributor

This looks good to merge to me! @jonmmease any objections?

@jmsmdy thanks so much for this, I'll likely merge it right before the next minor release (i.e. 4.15) early in the new year.

@rth
Copy link

rth commented Apr 6, 2021

I'll likely merge it right before the next minor release (i.e. 4.15) early in the new year.

Any remaining blockers for merging this PR ? Thanks!

@nicolaskruchten
Copy link
Contributor

No blockers, but the next version of Plotly.py will be 5.0, in a few weeks' time :)

@jonmmease jonmmease merged commit 6bf5849 into plotly:master Apr 22, 2021
@jonmmease
Copy link
Contributor

Thanks!

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.

[Feature Request] Replace dependency 'retrying' with 'tenacity' to allow possible Pyodide compatibility.
5 participants