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

Specify version of plotly.js in CDN URL #2961

Merged
merged 8 commits into from
May 31, 2021
Merged

Specify version of plotly.js in CDN URL #2961

merged 8 commits into from
May 31, 2021

Conversation

adehad
Copy link
Contributor

@adehad adehad commented Dec 8, 2020

Summary of Changes

  1. To prevent version changes of plotly.js breaking old html exports, these exports now included a version URL that matches plotly.py's bundled plotly.js. This is the default behaviour of include_plotlyjs='cdn'.
  2. include_plotlyjs='cdn-latest' added to preserve original behaviour.
  3. Refactored tests and code that rely on the plotly.js URL to use a function in io/_utils.py

Code PR

  • I have read through the contributing notes and understand the structure of the package. In particular, if my PR modifies code of plotly.graph_objects, my modifications concern the codegen files and not generated files.
  • I have added tests (if submitting a new feature or correcting a bug) or
    modified existing tests.
  • For a new feature, I have added documentation examples in an existing or
    new tutorial notebook (please see the doc checklist as well).
    Not Applicable
  • I have added a CHANGELOG entry if fixing/changing/adding anything substantial.
  • For a new feature or a change in behaviour, I have updated the relevant docstrings in the code to describe the feature or behaviour (please see the doc checklist as well).

@adehad adehad marked this pull request as ready for review December 8, 2020 18:02
@nicolaskruchten
Copy link
Contributor

Thanks for this PR! I'll take a look very soon.

I'm curious: was there a recent breaking change that affected you? We work very hard to avoid those so we'd like to know if something cropped up!

@adehad
Copy link
Contributor Author

adehad commented Dec 8, 2020

I'm curious: was there a recent breaking change that affected you? We work very hard to avoid those so we'd like to know if something cropped up!

Indeed, last week had some trouble with some historical px.treemap() made figures. These treemaps were generated using a pandas DataFrame and then specifying a path.

Tracked it down to the v1.58.0 release from a console message suggesting something along the lines of "root could not be found". (Which confused me at the time as my path included a "root" too haha!)

Thanks for this PR! I'll take a look very soon.

Appreciate it ! Looks like I broke the docs, not too familiar with this set up, but happy to have a crack if you have any pointers.

Example failure for 3d-axes :
[NbConvertApp] Converting notebook build/ipynb/3d-axes.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

@nicolaskruchten
Copy link
Contributor

Ah, sorry for the breakage of the treemaps! if you're able to share with us an example, we'd love to fix it for other users!!

Re the docs in CI, don't worry about that for now, it's unrelated to your PR.

@nicolaskruchten
Copy link
Contributor

Ah, i've found an example of this breakage in our very own docs 🙈 and I've filed an upstream issue: plotly/plotly.js#5328

@nicolaskruchten
Copy link
Contributor

(your PR is still a good idea and we'll try to merge it soon!)

@adehad
Copy link
Contributor Author

adehad commented Dec 8, 2020

if you're able to share with us an example, we'd love to fix it for other users!!

Yep completely right, slipped my mind that this would affect all users of of plotly.py especially as 4.14.0 now uses plotly.js 1.58.1.

I've been able to reproduce it with an example from the docs:

import plotly.express as px
import pandas as pd
vendors = ["A", "B", "C", "D", None, "E", "F", "G", "H", None]
sectors = ["Tech", "Tech", "Finance", "Finance", "Other",
           "Tech", "Tech", "Finance", "Finance", "Other"]
regions = ["North", "North", "North", "North", "North",
           "South", "South", "South", "South", "South"]
sales = [1, 3, 2, 4, 1, 2, 2, 1, 4, 1]
df = pd.DataFrame(
    dict(vendors=vendors, sectors=sectors, regions=regions, sales=sales)
)
df["all"] = "all" # in order to have a single root node
print(df)
fig = px.treemap(df, path=['all', 'regions', 'sectors', 'vendors'], values='sales')
fig.show()

Console Error I see is:
Uncaught TypeError: can't access property "root", e.trace is undefined
(Edit looks like I was a bit slow and you got this already whoops !)

@nicolaskruchten
Copy link
Contributor

Thanks! We've fixed this particular bug in 1.58.2 BTW.

elif include_plotlyjs == "cdn" or include_plotlyjs == "cdn-latest":
cdn_url = plotly_cdn_url()
if include_plotlyjs == "cdn-latest":
cdn_url = plotly_cdn_url(cdn_ver="latest")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, if this is merged after #3030 - i.e. use j2 v2, this call should be changed to:

Suggested change
cdn_url = plotly_cdn_url(cdn_ver="latest")
cdn_url = plotly_cdn_url(cdn_ver="latest-v2")

See: plotly/plotly.js#5462

@nicolaskruchten
Copy link
Contributor

OK, going to merge this and then do a 5.0 beta release :)

I actually think I will drop the latest option, as ideally we won't be hosting a "latest" bundle on the CDN for v2 after all, to discourage folks baking it into their production systems.

@adehad
Copy link
Contributor Author

adehad commented May 29, 2021

No idea if you needed extra permissions (I think I allowed "Maintainers" to contribute to the branch) but I've also just "invited you to collaborate" on the fork, feel free to push directly to it!

@nicolaskruchten nicolaskruchten merged commit be0dbc5 into plotly:master May 31, 2021
@nicolaskruchten
Copy link
Contributor

Thanks! I think I pushed up a branch to your repo by mistake instead of appending to this one... sorry :)

Thanks again for the contribution! I'll remove the "latest" functionality in a second PR.

@adehad adehad deleted the lock_cdn_ver branch June 12, 2021 19:36
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

2 participants