-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Conversation
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! |
This change made our use case possible! If it could be merged at some point (no urgency!), that would be fantastic. Thanks! |
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? |
Thanks @jmsmdy. Glad to learn that 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. |
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 |
@nicolaskruchten Just invited you as a collaborator on my branch, feel free to make whatever changes are necessary! |
@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:
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? |
Don't worry about the doc CI failures, I think it's just that we need to pin |
Re the |
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? |
Hmm let me take a look at those templates! Thanks for pushing through that failure! |
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 I'm not sure if this is the right long-term solution. |
OK so on |
…eate_conda_optional_env.sh to reflect change in dependencies. Reformated _orca.py with black.
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! |
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. |
Any remaining blockers for merging this PR ? Thanks! |
No blockers, but the next version of Plotly.py will be 5.0, in a few weeks' time :) |
Thanks! |
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