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

Type annotations in dash (+ add mypy tests in CI) #1748

Closed
wants to merge 1 commit into from

Conversation

anders-kiaer
Copy link
Contributor

@anders-kiaer anders-kiaer commented Sep 7, 2021

With Python 2 dropped, we would love to see Dash type hinted. Benefits:

  • Wrong usage of the dash package APIs can be captured using mypy when writing Dash apps, even directly in editors.
  • Easier to help with community PRs (easier to read code that is type hinted).
  • It is a prerequisite to later take a look at a killer feature: Type hint/editor help on props in the TypeScript/ReactPropmypy/Python hint border: Allow extracting meta data from typescript components #719 (comment).

Type hinted code base will also make it easier for Plotly to onboard new developers to plotly/dash. And it well help you detect type flow issues/bugs in the Python code not already covered (so far I found two type issues).

Happy to take this further if interest. What do you think @alexcjohnson et al.?

@@ -35,17 +35,17 @@ def __nonzero__(self):

# pylint: disable=no-init
class CallbackContext:
@property
@property # type: ignore[misc]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably due to python/mypy#1362.

GLOBAL_CALLBACK_MAP = {}
GLOBAL_INLINE_SCRIPTS = []
GLOBAL_CALLBACK_LIST: List[dict] = []
GLOBAL_CALLBACK_MAP: Dict[str, dict] = {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If taken further, I'll try replace dict with Dict with more information about the underlying structure.

ignore_errors=True

[mypy-dash.long_callback.managers.*]
ignore_errors=True
Copy link
Contributor Author

@anders-kiaer anders-kiaer Sep 7, 2021

Choose a reason for hiding this comment

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

The list of ignore errors will be shortened if PR is taken further. If it is too much to efficiently (merge conflicts etc) do in one PR, the mypy.ini file could be used to split out mypy / type hint work over several PRs.

@chriddyp
Copy link
Member

This is really awesome @anders-kiaer ! I've been in 2.7 backwards compatibility for so long that I don't have much 1st hand experience with Python-typing. Any downsides to doing this?

@anders-kiaer
Copy link
Contributor Author

anders-kiaer commented Sep 12, 2021

This is really awesome @anders-kiaer ! I've been in 2.7 backwards compatibility for so long that I don't have much 1st hand experience with Python-typing. Any downsides to doing this?

Thanks @chriddyp. I would say there are no real downsides (at least not compared to what you get back from type hinting). If I should try to come up with something, it would perhaps be slightly increased startup time of apps, and more code to write.

The first is to a large degree solved in Python 3.7+ (https://docs.python.org/3/whatsnew/3.7.html#pep-563-postponed-evaluation-of-annotations), and I would say that even in 3.6 you don't see this in practice (e.g. import plotly takes a long time typically in Python 3.6, so the CPython/Python std.lib type hint startup time increase discussion isn't something I would consider here - especially since Python 3.6 has EOL this year, including security fixes).

For the latter the extra code to write is in our experience more than justified by the value type hints give (automatic type testing, readability, help editors give the Dash app programmer even better live feedback on autocompletion/code issues etc.). It also reduces the need of writing docstrings/comments (which easily could get out of sync with the code). Sphinx can use type hints/annotations instead of rst-written type hints in docstrings (such that docstrings are only used for text explaining e.g. what the functions do). And for projects that don't use Sphinx and compile documentation themselves (or need type annotations during runtime for some other reason) can extract the type hints with https://docs.python.org/3.6/library/inspect.html#inspect.getfullargspec from std.lib:

annotations is a dictionary mapping parameter names to annotations. The special key "return" is used to report the function return value annotation (if any).

🙂

@anders-kiaer anders-kiaer changed the title First try type hints and mypy compatibility Type annotations in dash (+ add mypy tests in CI) Sep 13, 2021
@tuchandra
Copy link

tuchandra commented Dec 11, 2021

Hi @anders-kiaer - are you still actively working on this? I'd love to contribute if I can. What do you see as the minimal amount necessary to get something landed, assuming that doing it all at once will be challenging?

@anders-kiaer
Copy link
Contributor Author

Hi @tuchandra 👋

are you still actively working on this?

No, after the upside/"downside" input comment above, I didn't dare spend more time on it before clear indication from maintainers that "this will likely be merged when ready" 🙂 It is quite time consuming to in detail type hint when not knowing the internal code base in detail from before (and by nature a type hint PR touches a lot of code ⇾ easy to get merge conflicts if not quickly reviewed/merged).

What do you see as the minimal amount necessary to get something landed, assuming that doing it all at once will be challenging?

All at once, at least for a single person, would be challenging (and probably extra time consuming in total due to the likely merge conflicts). I guess a minimal amount could be:

  • Type hint ≥ 1 modules in dash.
  • Make sure mypy is green on dash (i.e. that type hinted modules passes, and other modules are configured to be ignored).
  • We add the mypy test to dash's CI workflow.

Future PRs would then type hint ≥ 1 new modules and correspondingly shorten the mypy ignore list.

I'd love to contribute if I can.

Please feel free to copy my commit as starting point. 🙂 (I can probably also grant you write access to my fork/branch and we do it as a joint effort splitting some modules between us. Let me know if prefer this instead.)

@tuchandra
Copy link

Got it, sure. Looking over this PR, it looks like you were focusing on the core dash module; I'm thinking starting with the public API might be easier, though? It could possibly be added to typeshed instead, if the maintainers here are not able to spend time on this.

@tuchandra
Copy link

@chriddyp, would adding the type stubs to typeshed be something you (or the other maintainers) are supportive of?

@gothicVI
Copy link
Contributor

Is this still considered? I'm highly in favor of adding type hints as dash is currently the only dependency we have that breaks full mypy coverage but at the same time is often the sole source of type errors.

@gvwilson gvwilson added feature something new P2 considered for next cycle labels Aug 13, 2024
@gvwilson
Copy link
Contributor

thank you again for this @anders-kiaer - our next major release is going to include some work by @T4rk1n that addresses a lot of these issues.

@gvwilson gvwilson closed this Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new P2 considered for next cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants