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

Improve Type Checking Support #3425

Merged
merged 1 commit into from
May 2, 2022
Merged

Improve Type Checking Support #3425

merged 1 commit into from
May 2, 2022

Conversation

JP-Ellis
Copy link
Contributor

@JP-Ellis JP-Ellis commented Oct 15, 2021

Code PR Checklist

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).
  • 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).

Prevent lazy import when type checking

This checks whether type checking is being done, and if so, disables the lazy import as this is generally not supported by type checkers. This should not have any impact on the runtime performance as the lazy import should still be used at runtime.

This is achieved by inspecting the TYPE_CHECKING variable.

This should resolve issue #1682 and provide a temporary workaround for issue #1103. In the latter case, the addition of stubs would probably still be an improvement as I believe this would be faster.

@JP-Ellis
Copy link
Contributor Author

As this changes part of the codegen code, I have re-ran that and included the subsequent changes in the commit. I wasn't sure whether I was meant to do that or not. Let me know if you want me to remove the generated code from the pull request.

@JP-Ellis
Copy link
Contributor Author

I've rebased the pull request on the master branch as this has included fixes for the previously failing tests.

@JP-Ellis
Copy link
Contributor Author

If there's any interest in this pull request, I'm happy to rebase so that it can merge with the most recent changes.

@maresb
Copy link
Contributor

maresb commented Jan 3, 2022

I'm just a Plotly user/contributor (not a maintainer) but I'm very interested in having this merged.

@niderhoff
Copy link

Still interested :)

@ldorigo
Copy link

ldorigo commented Apr 28, 2022

Could this get merged? Type hints are a huge help and this PR goes a long way to making them work... :-)

@nicolaskruchten
Copy link
Contributor

Sorry for the long radio-silence here folks. This PR seems fine in principle although I'm a bit worried about it breaking something I'm not aware of. Does anyone know of any major dangers to this approach?

If it could be rebased to the latest commit, I will kick the tires on it before the next release to see if I can break it.

@JP-Ellis
Copy link
Contributor Author

Thanks @nicolaskruchten. The only way I can see it causing an issue would be if typing.TYPE_CHECKING was defined at runtime (i.e. not during type checking), and that in principle should only result in a slowdown of Plotly as modules are no longer lazily loaded.

@JP-Ellis
Copy link
Contributor Author

JP-Ellis commented May 1, 2022

Commit has been rebased, and code has been regenerated.

For some reason, the regnerated code has the following change, though I'm not sure whether that is expected or not.

                     Sets the tick label formatting rule using d3
                     formatting mini-languages which are very
                     similar to those in Python. For numbers, see: h
-                    ttps://github.com/d3/d3-format/tree/v1.4.5#d3-f
-                    ormat. And for dates see:
+                    ttps://github.com/d3/d3-format/tree/v1.4.5#d3-
+                    format. And for dates see:
                     https://github.com/d3/d3-time-
                     format/tree/v2.2.3#locale_format. We add two
                     items to d3's date formatter: "%h" for half of

@maresb
Copy link
Contributor

maresb commented May 1, 2022

I was sad to see the CI failing, but it's failing for an unrelated issue.

I opened issue #3702 and proposed some fixes, including #3703 and #3704. Hopefully everything will be green soon!

@nicolaskruchten
Copy link
Contributor

CI should be fixed now, if you can merge master into your branch please :) (with thanks to @maresb)

This checks whether type checking is being done, and if so, disables the
lazy import as this is generally not supported by type checkers.  This
should not have any impact on the runtime performance as the lazy import
should still be used at runtime.

This is achieved by inspecting the
[`TYPE_CHECKING`](https://docs.python.org/3/library/typing.html#typing.TYPE_CHECKING)
variable.

This should resolve issue #1682 and provide a temporary workaround for
issue #1103.  In the latter case, the addition of stubs would probably
still be an improvement as I believe this would be faster.

Signed-off-by: JP-Ellis <josh@jpellis.me>
@JP-Ellis
Copy link
Contributor Author

JP-Ellis commented May 2, 2022

CI is now passing 🎉

@nicolaskruchten
Copy link
Contributor

Thanks! I'll kick the tires on this this week.

Just so I'm clear on what the expected impact here is, could you describe what you see with/without this PR? Which tools should display a difference: VSCode, Jupyter Notebook, JupyterLab? I'm not a PyCharm user so I can't test there... It would be great to have before/after screenshots for various platforms if anyone can submit one :)

@nicolaskruchten
Copy link
Contributor

OK the above message was a bit lazy: I'm downloading PyCharm now :)

@nicolaskruchten nicolaskruchten merged commit 022a525 into plotly:master May 2, 2022
@nicolaskruchten
Copy link
Contributor

OK, I'm satisfied that this is safe :)

I'd still like some clear examples about specific important interactions that folks are doing in certain IDEs which are improved by this PR.

@nicolaskruchten
Copy link
Contributor

Thanks @maresb and @JP-Ellis !

@maresb
Copy link
Contributor

maresb commented May 2, 2022

@nicolaskruchten, I use VS Code with Pylance for type checking.

I was using the following ugly hack.py:

from typing import TYPE_CHECKING

from plotly.graph_objs import FigureWidget

if TYPE_CHECKING:
    # Plotly does some conditional import stuff for the case that ipywidgets is missing.
    # This confuses Pylance, so we help it by directly importing the real FigureWidget.
    try:
        from plotly.graph_objs._figurewidget import FigureWidget  # noqa: F811, F401
    except ImportError:
        pass

and then

from hack import FigureWidget

This achieves the desired type inference:

image

otherwise, due to the dynamic import stuff, I end up with

image

I haven't tested it myself, but I expect this PR to supersede my ugly hack, so I'm happy about that. Thanks so much!!!

@maresb
Copy link
Contributor

maresb commented May 2, 2022

Since I can do it easily for VS Code, I just cloned master and installed it, and I can verify that my hack is no longer needed. Thanks again everyone! 😄

@JP-Ellis JP-Ellis deleted the type_checking branch May 2, 2022 21:03
@JP-Ellis
Copy link
Contributor Author

JP-Ellis commented May 2, 2022

@nicolaskruchten Type checking allows the editor to have some insight into the code and assist the user. There are many ways this can help:

  • The editor can suggest go.Scatter when the user has types go.S, or allowing the user to browse all the members of a module after typing go..
  • The editor can know the arguments of functions, for example, knowing that XAxis has autorange as an argument but not data.
  • Similarly, the editor can be aware of various class attributes and methods. So it will know that an XAxis object has a title.
  • In most cases, the documentation can be displayed as shown by @maresb

Note that while this PR allows the editor to do some introspection, as there are no type annotations in Plotly's code, the editor will still be unaware of the types of most things. In particular:

  • If I create explicitly an XAxis instance, the editor will know to suggest x_axis.title if I type x_axis.t; however, if I access the XAXis from a Layout instance (layout.xaxis), the editor will not know that I have an XAxis instance because the relevant code does not indicate what the function returns and thus layout.xaxis.t will not suggest layout.xaxis.title.
  • When creating an XAxis, the editor will know that autorange=... is a possible argument, but it will not know whether a string, Boolean, list, etc. is expected.

I am sure that the type annotations can be generated since most of the documentation includes type annotations, but this PR for now just allows the type checker / editor to see the code (as it was hidden behind lazy imports before).

@nicolaskruchten
Copy link
Contributor

Thanks @JP-Ellis! I'm pretty familiar with what type checking does, but I want to be able to explain specifically what IDE interactions didn't work before but do now... So for example it seems like typing go.S<tab> before did nothing in VSCode but now it autocompletes Scatter. Is that about right? FWIW, typing go.F<tab> still doesn't autocomplete Figure and I'm not sure why.

I'm working on getting some minimal type annotations for the chaining methods on Figure so that px.scatter(...).update_layout(...).add_hline(...).<tab> will propose all the Figure methods but the class hierarchy is making this annoying :)

@JP-Ellis
Copy link
Contributor Author

JP-Ellis commented May 2, 2022

Sorry, I wasn't sure what you were asking for, so my answer was quite broad.

It should be the case now that go.S<tab> and go.F<tab> autocomplete correctly. When I made the PR initially, I checked and it seemed to work fine. It should also allow access to further submodule. For example, go.layout.xaxis.<tab> should list the content of the module, namely:

  • Rangebreak
  • Rangeselector
  • Rangeslider
  • Tickfont
  • Tickformatstop
  • Title
  • rangeselector
  • rangeslider
  • title

The type annotations would be a massive improvement to the usability of Plotly in the future 👍

@nicolaskruchten
Copy link
Contributor

Actually, sorry go.S autocompletes to go.scatter (lowercase, which is a module) rather than go.Scatter in VSCode. Jupyter-LSP seems to do the right thing.

@nicolaskruchten
Copy link
Contributor

It should be the case now that go.S and go.F autocomplete correctly.

Can you validate this in your local editor? Does it work in VSCode? PyCharm?

@JP-Ellis
Copy link
Contributor Author

JP-Ellis commented May 3, 2022

I can confirm that it all works as expect.

  • plotly.graph_obs. allows me to scroll through all classes / submodules.
  • plotly.graph_obs.S gives me autocompletions starting with S: Sankey, Scatter, Scatter3d, ...
  • plotly.graph_objs.s gives me autocompletitions starting with s: sankey, scatter, scatter3d, ...
  • Creating an XAxis instance and typing xaxis. allows me to see all the expected attributes, and xaxis.tit completes with xaxis.title and xaxis.titlefont (and then other less exact fits).

I am using VSCode with Pylance as the language server. Note that the order in which suggestions are offered varies based on usage.

@nicolaskruchten
Copy link
Contributor

Ok thanks! Maybe I don't have Pylance or it's disabled for some reason...

@nicolaskruchten
Copy link
Contributor

nicolaskruchten commented May 3, 2022

Hmm no, Pylance is installed and enabled etc. Here's how I'm testing it, maybe you're doing something different... I'm opening a new file, selecting Python as the language and then typing import plotly.graph_objects as go; go.S and seeing the following (and scrolling down doesn't give me Scatter or any other class):

image

@JP-Ellis
Copy link
Contributor Author

JP-Ellis commented May 3, 2022

Yeah, not sure why this is happening. Doing the same thing for me gives go.Sankey first and then go.Scatter.

At this stage, I don't think it is an issue with the PR though, I think this is a difference due to configurations. Can you check that go.Scatter is in the list though? And if you type go.Scatter( does it start suggesting correct arguments?

@nicolaskruchten
Copy link
Contributor

Oh the PR is great, I'm just making sure I know how to explain it correctly in the upcoming announcement post on the forum :)

Indeed, I found the settings in my VSCode which were preventing this from working perfectly, sorry for the noise :)

@nicolaskruchten
Copy link
Contributor

And this works in JupyterLab without the language-server extensions too. Nice work @JP-Ellis !

@nicolaskruchten
Copy link
Contributor

Final note on this PR: as a result of finally merging it, I was able to do the work in this PR #3708 and get some nice easy wins! Thanks again :)

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