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: Infer parameters types for __init__(default...), __get__ and __set__ #985

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

hoxbro
Copy link
Member

@hoxbro hoxbro commented Dec 13, 2024

Adding type hints to __set__, __get__, and __init__.

Haven't tested on old versions, so for now this should be seen as a POC.

Code
from __future__ import annotations

from typing import reveal_type
from datetime import date

import param


class MyParams(param.Parameterized):
    a = param.Integer()
    b = param.Integer(default=10)
    c = param.Boolean(default=True)
    d = param.Number()
    e = param.Number(default=10.0)

    f = param.Date()
    g = param.Date(default=date(2020, 1, 1))

    h = param.ClassSelector(default=1, class_=int)


params = MyParams()
reveal_type(params.a)
reveal_type(params.b)
reveal_type(params.c)
reveal_type(params.d)
reveal_type(params.e)
reveal_type(params.f)
reveal_type(params.g)
reveal_type(params.h)

for s in "abcdefgh":
    print(f"params.{s} = {getattr(params, s)}")

With this PR:
image

Before:

 Screenshot 2024-12-13 12 29 33

Copy link

codecov bot commented Dec 13, 2024

Codecov Report

Attention: Patch coverage is 98.55072% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.25%. Comparing base (57064e6) to head (a2bd29b).

Files with missing lines Patch % Lines
param/parameters.py 98.38% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #985   +/-   ##
=======================================
  Coverage   87.25%   87.25%           
=======================================
  Files           9        9           
  Lines        4928     4936    +8     
=======================================
+ Hits         4300     4307    +7     
- Misses        628      629    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hoxbro
Copy link
Member Author

hoxbro commented Dec 13, 2024

Pretty cool with list:

image

@hoxbro
Copy link
Member Author

hoxbro commented Dec 13, 2024

Not sure how type checkers like Mypy and Pyright like this, but the LSP seems to love it 🙂

@jbednar
Copy link
Member

jbednar commented Dec 13, 2024

Cool!

@hoxbro hoxbro changed the title playing around with types for parameters type: Infer parameters types for __init__(default...), __get__ and __set__ Dec 16, 2024
@philippjfr
Copy link
Member

This seems like a good start but I also worry that's it's actually dead end. The problem being that the typing is inherited entirely from the default but in many cases the default could be None or in fact only cover a subset of what is actually supported.

@hoxbro
Copy link
Member Author

hoxbro commented Dec 16, 2024

The main point of this PR was to add type information to the parameter, not type validation. I'm unsure if we can add type validation with type checkers like MyPy, Pyright, or BasedPyright

I have pushed some changes to the ClassSelector, which does some validation with a type checker.

image

from __future__ import annotations


import param
from typing import reveal_type


class MyParams(param.Parameterized):
    a = param.ClassSelector(default=1, class_=(list, int, str))
    b = param.ClassSelector(default=1, class_=int)


params = MyParams()

reveal_type(params.a)
reveal_type(params.b)

params.a = [1]
params.a = "a"
params.b = "b"

@maximlt
Copy link
Member

maximlt commented Dec 16, 2024

The main point of this PR was to add type information to the parameter, not type validation.

What is the difference between type information and type validation?

Given how big of a topic typing Param is, writing down somewhere (e.g. issue, design doc) the direction we are taking would be worthwhile. I will struggle merging a PR without having the big picture in mind.

@hoxbro
Copy link
Member Author

hoxbro commented Dec 16, 2024

What is the difference between type information and type validation?

Type information shows what the argument is, and type validation verifies that it matches what is expected. This PR currently adds information about the parameter.

This is useful when running an LSP, as it currently always writes something like _Undefined | Any | object; it will now infer it from the __init__(default=...), __set__, or __get__.

This means an LSP will understand that a = param.String() is a string and can autocomplete its method.

image

This does not do any validation. E.g., param.Integer(default="1") will not raise any warnings in the code. Validation of this is handled at runtime.

image

Given how big of a topic typing Param is, writing down somewhere (e.g. issue, design doc) the direction we are taking would be worthwhile. I will struggle merging a PR without having the big picture in mind.

Understandable. The technology itself currently limits the big picture. I'm also afraid this will be postponed because we don't have a "perfect" solution.

@hoxbro
Copy link
Member Author

hoxbro commented Dec 16, 2024

We likely could add some validation, this will, as far as I can see, come at the cost of type information (which could be ok). Below is an example of Integer.

image
image

diff --git a/param/parameters.py b/param/parameters.py
index 508159a..b70745f 100644
--- a/param/parameters.py
+++ b/param/parameters.py
@@ -839,7 +839,7 @@ class Number(Dynamic[T]):
 
 
 
-class Integer(Number[T]):
+class Integer(Number[int]):
     """Numeric Parameter required to be an Integer"""
 
     _slot_defaults = dict(Number._slot_defaults, default=0)
@@ -847,14 +847,14 @@ class Integer(Number[T]):
     @typing.overload
     def __init__(
         self,
-        default: T = 0, *, bounds=None, softbounds=None, inclusive_bounds=(True,True), step=None, set_hook=None,
+        default: int = 0, *, bounds=None, softbounds=None, inclusive_bounds=(True,True), step=None, set_hook=None,
         allow_None=False, doc=None, label=None, precedence=None, instantiate=False,
         constant=False, readonly=False, pickle_default_value=True, per_instance=True,
         allow_refs=False, nested_refs=False
     ):
         ...
 
-    def __init__(self, default: T = Undefined, *args, **kwargs):
+    def __init__(self, default: int = Undefined, *args, **kwargs):
         super().__init__(default=default, *args, **kwargs)
 
     def _validate_value(self, val, allow_None):

@hoxbro
Copy link
Member Author

hoxbro commented Dec 16, 2024

This is also not supported, and I am wondering if it can be supported.

image

@maximlt
Copy link
Member

maximlt commented Dec 16, 2024

Ok thanks I get what you mean with type information (inference) vs. validation (run-time checked).

We likely could add some validation, this will, as far as I can see, come at the cost of type information (which could be ok).

Why?

This is also not supported, and I am wondering if it can be supported.

I'm not sure I follow, what is not supported?

Understandable. The technology itself currently limits the big picture. I'm also afraid this will be postponed because we don't have a "perfect" solution.

Which technology? How Param is implemented? What Python typing offers? Or both?

Don't be afraid :) You've already mentioned a few interesting things in this PR (type information, LSP, type validation, dataclass_transform), it'd be valuable to collect them in a single document that can be augmented over time with our findings. Typing in Param is a big enough project that it requires this kind of approach imo. Perhaps this will even demonstrate that it is not possible to provide a robust typing experience with Param due its too-dynamic nature, making us think about some changes we could make to accommodate that (if typing is judged more important).

At the very least, I'd love to read more explanation about the approach showcased in this PR and its tradeoffs. It's okay if it's not "perfect", but if it's wrong (e.g. how is None as default handled?) then we need to know how exactly to decide whether it's acceptable or not.

@hoxbro
Copy link
Member Author

hoxbro commented Dec 17, 2024

Ok thanks I get what you mean with type information (inference) vs. validation (run-time checked).

We can get type validation. I'm using basedpyright as my LSP here. Setting the type-checking mode to "basic" (lowest type setting, see here) with the diff in #985 (comment), give the following result (note I have only activated this mode in this comment):

image

We likely could add some validation, this will, as far as I can see, come at the cost of type information (which could be ok).

Why?

As you can see, every reveal_type outputs int, even though d is actually a string. This is what I meant by trade-off: we get some type-validation at the cost of wrong type-information. This will error out at runtime, so this is OK. Ideally the type-validation and runtime-validation would match up, but there could be edge cases. Also, currently, we will lose some of the type information when doing it with a bit more complex parameter like param.List.

This is also not supported, and I am wondering if it can be supported.

I'm not sure I follow, what is not supported?

See c in the screenshot above, e.g., c: int. Sorry for not being more specific.

Which technology? How Param is implemented? What Python typing offers? Or both?

Mostly, Python typing is pretty rough. Some changes could likely also be done in Param.

Don't be afraid :) You've already mentioned a few interesting things in this PR (type information, LSP, type validation, dataclass_transform),

I haven't found a use for dataclass_transform yet, but it could come into play later.

it'd be valuable to collect them in a single document that can be augmented over time with our findings.

It would be ideal, but currently, I'm just trying things out and seeing what sticks. I'm not even sure this works with other people's setups.

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.

4 participants