Skip to content

Add support for manually generated stub files (*.pyi)#13900

Merged
mattpap merged 4 commits into
branch-3.7from
mattpap/13870_pyi
Jan 30, 2025
Merged

Add support for manually generated stub files (*.pyi)#13900
mattpap merged 4 commits into
branch-3.7from
mattpap/13870_pyi

Conversation

@mattpap

@mattpap mattpap commented May 17, 2024

Copy link
Copy Markdown
Contributor

This PR is an experiment, based on discussion #13870 and other, to bring static types to models and properties. The main goal of this PR is to establish the correct patterns, what works for us and what doesn't. For example I marked all models as data classes, to solve the problem of repetition of properties and their types in __init__ and class members (and also from base classes in __init__).

Ultimately most of this will be automatically generated, but before that we need to figure out the correct types, structure, etc. We also can't simply generate the structure without types, because then we would lose all type information. This is because mypy doesn't support declaration merging, like TypeScript does, so it's an all or nothing type of situation.

Why this didn't happen earlier? It would be infinitely better to have types together with the source code in one place, as we attempted to do and what we achieved in bokehjs. However, due limitations of Python typing, mypy, etc., at this point having separate type declarations at least for models and properties, seems to be the only reasonable approach, short of redesigning our model/property system to better fit the typing ecosystem (which may not be the worst idea for bokeh 4.0 or beyond).

For the sake of convenience and code readability, this PR uses Python 3.12 syntax:

  • type TypeName = This | That for defining type aliases
  • type TypeName[A, B] = This[A] | That[B] for defining generic type aliases
  • class SomeClass[T]: ... for defining generic classes

This only relates to *.pyi files and affects type-checking. It doesn't require run-time support. I suppose we having mypy configured for 3.12 and ruff for 3.10, will be sufficient to use modern typing syntax and make sure it doesn't bleed into *.py files.

fixes #13292
addresses #13870

@mattpap mattpap added this to the 3.x milestone May 17, 2024
@bryevdv

bryevdv commented May 17, 2024

Copy link
Copy Markdown
Member

We also can't simply generate the structure without types, because then we would lose all type information.

FWIW I was planning to do exactly that [1], although my initial thought was to auto generate explicit __init__ for everything, rather than relying on @dataclass to do that automagically. I think your idea here is a very clever approach, but I have some questions:

  • We have some custom __init__ to handle. I don't think we can get rid of them all, e.g. Range1d(start, end). How would to propose to incorporate cases with custom __init__?
  • I am just extremely skeptical that we would be able to actually define proper actual Python types corresponding to all of the Bokeh types. Assuming that is the case, is there a way to make things work that is not "all or nothing"?

[1] Remember, the actual ask is to support auto-completion in IDEs, and that does not require types to be present in order to function

@mattpap
mattpap force-pushed the mattpap/13870_pyi branch from 2aaf2c5 to d88364f Compare May 21, 2024 13:11
@mattpap
mattpap changed the base branch from branch-3.5 to branch-3.6 July 4, 2024 10:42
@mattpap
mattpap changed the base branch from branch-3.6 to branch-3.7 September 26, 2024 16:36
@bryevdv

bryevdv commented Jan 21, 2025

Copy link
Copy Markdown
Member

@mattpap I would like to proceed more quickly with an initial update that adds the stub files without types. I don't think there is any reasonable way to conclude that "no types" is worse than "wrong types" (i.e. the current situation) but at least IDE auto-completion would start to work. Should I start a separate PR or do you want to prioritize this?

@mattpap
mattpap force-pushed the mattpap/13870_pyi branch 3 times, most recently from 6e1784a to 58369d0 Compare January 22, 2025 13:56
@mattpap

mattpap commented Jan 22, 2025

Copy link
Copy Markdown
Contributor Author

At this rate I should be done with the initial pass over all models by the end of the week.

@mattpap
mattpap force-pushed the mattpap/13870_pyi branch 2 times, most recently from dcf4834 to c284c55 Compare January 23, 2025 17:12
@mattpap

mattpap commented Jan 23, 2025

Copy link
Copy Markdown
Contributor Author

This is how model constructor signature looks like in VSCode:

image

Comment thread src/bokeh/core/property/any.py Outdated
Comment thread src/bokeh/.ruff.toml Outdated
Comment thread src/bokeh/_types.pyi
@mattpap
mattpap force-pushed the mattpap/13870_pyi branch 4 times, most recently from 69a18d9 to 203e00c Compare January 24, 2025 06:05
@mattpap mattpap modified the milestones: 3.x, 3.7 Jan 24, 2025
@mattpap

mattpap commented Jan 24, 2025

Copy link
Copy Markdown
Contributor Author

This is tentatively ready though there are two areas that need work:

  1. Type data specs.
  2. Fill in property mixins (Include).

@mattpap
mattpap force-pushed the mattpap/13870_pyi branch 5 times, most recently from a2c2e49 to b128cc3 Compare January 24, 2025 19:29
@mattpap

mattpap commented Jan 24, 2025

Copy link
Copy Markdown
Contributor Author

This PR is now fully complete.

Comment thread docs/bokeh/source/docs/dev_guide/python.rst Outdated
@bryevdv

bryevdv commented Jan 27, 2025

Copy link
Copy Markdown
Member

I'll try to take a look at this tonight

@mattpap

mattpap commented Jan 27, 2025

Copy link
Copy Markdown
Contributor Author

I started issue #14254 for statically typing glyph methods.

@bryevdv bryevdv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Generally LGTM at a quick glance. I have not done a fine-toothed comb review of the .pyi files. Did you write a script to partially automate their generation? If so please check it into scripts.

Comment thread src/bokeh/core/enums.py
Comment thread pyproject.toml
Comment thread src/bokeh/models/tools.py
def GlyphRendererOf(*types: type[Model]):
""" Constraints ``GlyphRenderer.glyph`` to the given type or types. """
return TypeOfAttr(Instance(GlyphRenderer), "glyph", Either(*(Instance(tp) for tp in types)))
return TypeOfAttr(Instance(GlyphRenderer), "glyph", Either(*(Instance(type) for type in types)))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am surprised a linter did not complain but regardless, best not to shadow builtins. I usually use typ

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't see why it would. type is a built-in function, not e.g. a keyword, so shadowing it is perfectly fine. In fact modern language servers in editors like vscode recognize this and change highlight color of type to indicate it's not a built-in anymore.

@bryevdv bryevdv Jan 29, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I didn't mean to imply that it's not possible, merely that it's usually not advised as it can cause confusion. This applies to any built-in function, e.g. you can set range = 10 or all = None but probably shouldn't. Arguably here the risk is minimized greatly since it is contained inside a list comprehension, so I'm mostly arguing to advocate for consistency.

@bryevdv bryevdv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But please add any helper scripts even if they are speculative

@mattpap

mattpap commented Jan 29, 2025

Copy link
Copy Markdown
Contributor Author

But please add any helper scripts even if they are speculative

I don't have any, I used vim macros and quite a bit of labor to do the conversion.

@mattpap
mattpap merged commit e460f3f into branch-3.7 Jan 30, 2025
@mattpap
mattpap deleted the mattpap/13870_pyi branch January 30, 2025 00:14
@bryevdv

bryevdv commented Jan 30, 2025

Copy link
Copy Markdown
Member

@mattpap thoughts on backporting this? It only has one small conflict for Segment

bryevdv pushed a commit that referenced this pull request Jan 30, 2025
* Add support for manually generated stub files (*.pyi)

* Properly declare visual properties

* Add release notes

* Add copyright header to *.pyi files
@bryevdv bryevdv mentioned this pull request Jan 30, 2025
21 tasks
@github-actions

Copy link
Copy Markdown

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators May 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow to statically type models and properties in bokeh

2 participants