-
-
Notifications
You must be signed in to change notification settings - Fork 80
Add static typing #1065
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
base: main
Are you sure you want to change the base?
Add static typing #1065
Conversation
|
Woah, this looks extremely promising. |
| from typing import Any, Optional, overload | ||
|
|
||
| class String: | ||
| default: Optional[str] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you handle allow_None=False?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't so far. This just explores the principles via real code. If allow_None can be changed dynamically it would be the right type annotation anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I'd be interested in learning how we might need to adapt Param to make it work better with typing. For instance, yes I think you can modify allow_None dynamically? (you can do pretty much you want in Python dynamically anyway). In practice though, I don't think users change it dynamically. But allow_None is used quite a lot so it's important to support it, otherwise the hints will be wrong in a lot of times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I understand is that the problem is that we are using descriptors (classes) and they cannot be overloaded - thus Optional[str] is the best type annotation we can provide.
In experiment.py I also shown how you can make overloaded param_string functions instead - might solve some issues. But I don't think we would like to go down that route?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But allow_None is used quite a lot so it's important to support it, otherwise the hints will be wrong in a lot of times.
Though not ideal I think it's fine if the type is more permissive than the actual validation, so I'd expect we have to make all types Optional to account for allow_None.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can see there is no way around it unless we want to make breaking changes to param and leave the "descriptor" pattern behind.
We could split String into String and OptionalString parameters to follow Pythons type system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If made more permissive than in reality I expect a type checker to complain at some point or another, see the code below. How would we let people know that the library provides inexact type hints? I haven't worked so much with type hints so far but know that when type checking is enabled in a code base dealing with wrong types gets quickly pretty frustrating.
def algo(input: str): pass
class P(param.Parameterized):
value = param.String(allow_None=False)
def process(self):
# Wouldn't a type checker be complaining there
# if the type of `self.value` is `str | None`? Given
# that `algo` only accepts `str`, I'd expect it to complain.
return algo(self.value)We could split String into String and OptionalString parameters to follow Pythons type system.
That's one approach. You can integrate it in your proposal if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One solution to make a type checker happy here is to add assert isinstance(self.value, str) before calling algo. Not sure that's a very satisfying solution but that is the sort of thing I'd expect us to have to document if we release a version of Param that doesn't always provide correct type hints.
| @@ -0,0 +1,66 @@ | |||
| from typing import Any | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where the value is :-) Showing how dataclass_transform can be applied etc.
|
@MarcSkovMadsen I'm not really sure what I'm supposed to review here and what the actual proposal is. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so I spent some time reviewing the Markdown files and exploring experiment.py. Some points I wanted to raise in addition to the comments:
- I found some inaccuracies in the problem statement in
problem.md - I'm not sure what's the difference between
analysis.mdandexperiment.md, they contain somewhat similar content - The
dataclass_transformis used but I haven't found it to work? From the documents, I don't see a description of why it doesn't work, and what it would take for it to work. - Only
param.Stringandparam.Numberare used as examples. I would have expected an analysis of this kind to experiment with a wider variety of Parameters (selectors, mappings, sequences, callable, class selector, etc.), to find potential limitations and suggest approaches how to deal with them. - Have you looked into how SQLAlchemy implemented their typing support in v2? Some of their requirements look similar to ours https://docs.sqlalchemy.org/en/20/changelog/whatsnew_20.html#orm-declarative-models.
My intuition with making Param work better with type hints is that:
- it would need quite some work
- to make it a good experience some changes in Param would be required (and personally I would be in favor of whatever changes needed, even if it means losing some of the runtime capabilities of Param).
This analysis and the changes made both look too shallow. I also don't see what changes we would need to make to Param so it works well with type hints. Overall, I struggle to see from this analysis what are our options? Surely, there are many ways to improve the current situation.
Maybe it is because the goal is to get some more type hints in an IDE? If so, is that a goal we can reach without bringing some frustrations because the type hints we'd provide are not correct?
Would you accept a PR like that? If yes then I will start implementing.
I don't think I would accept a PR like that as this analysis didn't give me the confidence this is a valid approach. Which shouldn't prevent you from exploring further that approach or another one! Also, I see myself as a release manager of Param; @jbednar, @philippjfr and @jlstevens are its official maintainers and can have a different opinion than mine; Philipp seems to be pretty interested in how this work started.
| @@ -0,0 +1,52 @@ | |||
| # Param - Static Typing - Problem | |||
|
|
|||
| [HoloViz Param](https://param.holoviz.org/) is a powerful library for defining parameters on `param.Parameterized` classes: | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a strange definition of Param?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But does it matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You wrote on Discord that you used AI to work on this PR. I wouldn't have pointed this out otherwise but maybe in that case it matters.
| **Param does not play well with Python's static typing ecosystem.** | ||
|
|
||
| Modern Python development expects type annotations to: | ||
| - Enable code completion and inline documentation in editors (like VS Code, PyCharm) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type annotations enable inline documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Tooltips with init signatures and in general more information.
|
|
||
| Modern Python development expects type annotations to: | ||
| - Enable code completion and inline documentation in editors (like VS Code, PyCharm) | ||
| - Allow static analysis and error checking with tools like mypy and pylint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean pylance? I haven't used pylint in a while but don't really expect it to use type annotations much, at least not as much as mypy/pylance/ty and others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mypy and pylance use it more. But for example pylint/ ruff have rules for working with type annotations - for example enforcing them.
| But with Param, these benefits are lost: | ||
|
|
||
| - **No Automatic Type Inference:** When you declare a parameter (e.g. `my_parameter = param.String(...)`), neither the class nor its instances have a type annotation for `my_parameter`. Editors and type checkers cannot know its type. | ||
| - **Poor Editor Experience:** You don't get tab completion, hover help, or docstrings for parameter attributes on instances. For example, `instance.my_parameter` is invisible to your IDE. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| data: pd.DataFrame = param.DataFrame(precedence=0.1) | ||
| ``` | ||
|
|
||
| Even with a type annotation, editors and type checkers do not recognize `data` as a `pd.DataFrame` on the instance. This leads to poor auto-completion and false warnings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its when data = param.DataFrame(precedence=0.1). Then its not recognized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then this example and the text need to be updated as it clearly states "even with a type annotation, ...":
Even with a type annotation, editors and type checkers do not recognize
dataas apd.DataFrameon the instance. This leads to poor auto-completion and false warnings.
|
|
||
| obj = MyClass(my_parameter="hello", my_number=3.14) | ||
|
|
||
| reveal_type(obj.my_parameter) # Should be "builtins.str" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VS Code shows str | None.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It cannot infer more. But ideally it should be str. Cannot be achieved without
- type annotation or
- overloaded function or
- plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as #1065 (comment)
| from typing import Any, Optional, overload | ||
|
|
||
| class String: | ||
| default: Optional[str] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If made more permissive than in reality I expect a type checker to complain at some point or another, see the code below. How would we let people know that the library provides inexact type hints? I haven't worked so much with type hints so far but know that when type checking is enabled in a code base dealing with wrong types gets quickly pretty frustrating.
def algo(input: str): pass
class P(param.Parameterized):
value = param.String(allow_None=False)
def process(self):
# Wouldn't a type checker be complaining there
# if the type of `self.value` is `str | None`? Given
# that `algo` only accepts `str`, I'd expect it to complain.
return algo(self.value)We could split String into String and OptionalString parameters to follow Pythons type system.
That's one approach. You can integrate it in your proposal if you want.
| default: float | ||
| def __get__(self, instance: Any, owner: Any) -> float: ... | ||
| def __set__(self, instance: Any, value: float) -> None: ... | ||
| def __init__(self, default: Optional[float] = ..., doc: Optional[str] = ...) -> None: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding first question. Yes static type checkers would complain that you would have to check for None. But now you only have two possibilities str | None. To some that is a big help. The ones that do no appreciate types can just let them be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that having type annotations would be very useful. But I also think that if we have a library that provides type annotations that are not always correct, then we should provide a solution for people to deal with that. It might be it would just be a documentation thing, stating that these annotations should only be used in an IDE and in a special mode. I don't really know to be honest but I don't find that answer to be complete and clear enough:
The ones that do no appreciate types can just let them be.
| from typing import Any, Optional | ||
|
|
||
| class Number: | ||
| default: float |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is default required here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I thought it defined the return type of some_number = param.Number(...). But probably its just a type annotation of the default attribute should be float | None to be correct.
| ### Param Descriptors and Typing | ||
| - Param descriptors (e.g., param.String, param.Number) can provide good attribute type inference in mypy if their stubs define correct __get__ return types (e.g., str, Optional[str], float). | ||
| - Type annotations on descriptor assignments cause mypy errors unless suppressed with # type: ignore[assignment]. | ||
| - Wrapper functions using @overload (e.g., param_string) can help mypy infer the type of the value returned by the function, but the attribute is still a descriptor, so __init__ signatures are not synthesized. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the __init__ signature not correct when the attribute is a selector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry. Don't understand the question?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I used the wrong word and meant descriptor instead of selector.
Why is the __init__ signature not correct when the attribute is a descriptor?
|
Thanks again for starting this @MarcSkovMadsen. After reviewing what's here I'm wavering a little bit. Effectively based on my reading, and until Python adopts type-aware descriptors we don't have many good options. With the list effectively being:
from typing import Optional
class Integer:
def __get__(self, instance: object, owner: type) -> Optional[int]:
...
def __set__(self, instance: object, value: Optional[int]) -> None:
...
class Model(param.Parameterized):
a: int = Integer()
b: Optional[int] = Integer(allow_None=True)
class Model(param.Parameterized):
a = Integer[int]()
b = Integer[Optional[int]](allow_None=True)with some extra effort in defining class Model(param.Parameterized):
a = Integer[int]()
b = Integer(allow_None=True)
On balance I think the approach using generics is most promising, even if it is somewhat verbose. So my feeling is that @hoxbro's PR https://github.com/holoviz/param/pull/985/files is the correct place to start from. |
|
Traitlets seems to have figured out something interesting, you can see that with # t.py
from traitlets import HasTraits, Int
class P(HasTraits):
x = Int(default=2)
y = Int(allow_none=True)
p = P(x=3, y=None)
reveal_type(p.x)
reveal_type(p.y) |






Closing #376
This PR adds static typing to param in a way that will not fundamentally change the current implementation and keep the api.
A more radical approach would be to leave descriptor pattern and engage with static typing stakeholders (including mypy) and get param supported in same way as dataclasses, attrs and pydantic.
Recommendation (from analysis.md)
Update - Plugin
Ahh. I tried experimenting with a plugin to generate the
__init__signature - but this was too complicated for me and AI. See https://docs.pydantic.dev/latest/integrations/mypy/#enabling-the-plugin and https://github.com/pydantic/pydantic/blob/main/pydantic/mypy.py.Todo
mypy static-typing/experiment.py --no-incremental.)