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

Annotate pn.io.cache decorator #7079

Merged
merged 14 commits into from
Aug 8, 2024

Conversation

gandhis1
Copy link
Contributor

@gandhis1 gandhis1 commented Aug 5, 2024

Part of #7078

In the docs I see that minimum Python version is 3.9, however in the pyproject.toml it states requires-python = ">=3.10". Importing ParamSpec from typing instead of typing_extensions requires 3.10. This PR assumes the latter is correct and that typing_extensions is not required.

@MarcSkovMadsen

@gandhis1 gandhis1 force-pushed the cache_decorator_annotations branch from 4fdf060 to d0f1196 Compare August 5, 2024 18:14
@gandhis1 gandhis1 marked this pull request as ready for review August 5, 2024 18:18
@gandhis1 gandhis1 force-pushed the cache_decorator_annotations branch from d0f1196 to 76d7390 Compare August 5, 2024 18:34
Copy link

codecov bot commented Aug 5, 2024

Codecov Report

Attention: Patch coverage is 60.86957% with 9 lines in your changes missing coverage. Please review.

Project coverage is 82.23%. Comparing base (4c49251) to head (7d85ac6).

Files Patch % Lines
panel/io/cache.py 60.86% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7079      +/-   ##
==========================================
- Coverage   82.23%   82.23%   -0.01%     
==========================================
  Files         326      326              
  Lines       48586    48604      +18     
==========================================
+ Hits        39955    39969      +14     
- Misses       8631     8635       +4     

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

@MarcSkovMadsen MarcSkovMadsen self-requested a review August 6, 2024 05:25
@MarcSkovMadsen
Copy link
Collaborator

I'm wondering whats the best way to check this via mypy.

I tried mypy --follow-imports=skip panel/io/cache.py but it errors.

$ mypy --follow-imports=skip panel/io/cache.py
Traceback (most recent call last):
  File "/home/jovyan/repos/private/panel/.venv/bin/mypy", line 8, in <module>
    sys.exit(console_entry())
             ^^^^^^^^^^^^^^^
  File "/home/jovyan/repos/private/panel/.venv/lib/python3.11/site-packages/mypy/__main__.py", line 15, in console_entry
    main()
  File "mypy/main.py", line 103, in main
  File "mypy/main.py", line 187, in run_build
  File "mypy/build.py", line 193, in build
  File "mypy/build.py", line 268, in _build
  File "mypy/build.py", line 2950, in dispatch
  File "mypy/build.py", line 3348, in process_graph
  File "mypy/build.py", line 3475, in process_stale_scc
  File "mypy/build.py", line 2507, in write_cache
  File "mypy/build.py", line 1568, in write_cache
  File "mypy/nodes.py", line 390, in serialize
  File "mypy/nodes.py", line 4012, in serialize
  File "mypy/nodes.py", line 3949, in serialize
  File "mypy/nodes.py", line 3374, in serialize
  File "mypy/types.py", line 671, in serialize
  File "mypy/types.py", line 2428, in serialize
  File "mypy/types.py", line 1451, in serialize
  File "mypy/types.py", line 671, in serialize
  File "mypy/types.py", line 3093, in serialize
AssertionError: Internal error: unresolved placeholder type None

Then I tried mypy panel/io/cache.py which seems to work better.

$ mypy panel/io/cache.py
panel/io/cache.py:24: error: Skipping analyzing "param": module is installed, but missing library stubs or py.typed marker  [import-untyped]
panel/io/cache.py:26: error: Skipping analyzing "param.parameterized": module is installed, but missing library stubs or py.typed marker  [import-untyped]
panel/io/cache.py:26: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
panel/io/cache.py:38: error: Need type annotation for "_HASH_MAP" (hint: "_HASH_MAP: dict[<type>, <type>] = ...")  [var-annotated]
panel/io/cache.py:40: error: Need type annotation for "_HASH_STACKS"  [var-annotated]
panel/io/cache.py:110: error: Library stubs not installed for "pandas"  [import-untyped]
panel/io/cache.py:110: note: Hint: "python3 -m pip install pandas-stubs"
panel/io/cache.py:110: note: (or run "mypy --install-types" to install all missing stub packages)
panel/io/cache.py:177: error: Incompatible types in assignment (expression has type "bytes", target has type "Callable[[Any], Any]")  [assignment]
panel/io/cache.py:389: error: Incompatible return value type (got "Callable[[Any], Callable[[Callable[P, R]], Callable[P, R]]]", expected "Callable[P, R@cache] | Callable[[Callable[P, R@cache]], Callable[P, R@cache]]")  [return-value]
panel/io/cache.py:436: error: Cannot find implementation or library stub for module named "diskcache"  [import-not-found]
panel/io/cache.py:462: error: Incompatible types in "await" (actual type "R", expected type "Awaitable[Any]")  [misc]
panel/io/cache.py:493: error: "_Wrapped[P, R, P, Coroutine[Any, Any, R]]" has no attribute "clear"  [attr-defined]
panel/io/cache.py:505: error: Incompatible return value type (got "_Wrapped[P, R, P, Coroutine[Any, Any, R]]", expected "Callable[P, R] | Callable[[Callable[P, R]], Callable[P, R]]")  [return-value]
Found 11 errors in 1 file (checked 1 source file)

@MarcSkovMadsen
Copy link
Collaborator

MarcSkovMadsen commented Aug 6, 2024

I've tried to configure mypy in #7081. Could you do a review of that one @gandhis1? Thanks.

@MarcSkovMadsen
Copy link
Collaborator

I can see that @hoxbro updated to Python 3.10 in #6883. This PR has not yet been released. I.e. the upgrade to Python 3.10 for users will happen when 1.5.0 is released.

R = TypeVar("R")
T = TypeVar("T")

@overload
Copy link
Collaborator

Choose a reason for hiding this comment

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

After merging this PR with 7081 the errors I see for the panel/io/cache.py file are

panel/io/cache.py:38: error: Need type annotation for "_HASH_MAP" (hint: "_HASH_MAP: dict[<type>, <type>] = ...")  [var-annotated]
panel/io/cache.py:40: error: Need type annotation for "_HASH_STACKS"  [var-annotated]
panel/io/cache.py:177: error: Incompatible types in assignment (expression has type "bytes", target has type "Callable[[Any], Any]")  [assignment]
panel/io/cache.py:389: error: Incompatible return value type (got "Callable[[Any], Callable[[Callable[P, R]], Callable[P, R]]]", expected "Callable[P, R@cache] | Callable[[Callable[P, R@cache]], Callable[P, R@cache]]")  [return-value]
panel/io/cache.py:462: error: Incompatible types in "await" (actual type "R", expected type "Awaitable[Any]")  [misc]
panel/io/cache.py:493: error: "_Wrapped[P, R, P, Coroutine[Any, Any, R]]" has no attribute "clear"  [attr-defined]
panel/io/cache.py:505: error: Incompatible return value type (got "_Wrapped[P, R, P, Coroutine[Any, Any, R]]", expected "Callable[P, R] | Callable[[Callable[P, R]], Callable[P, R]]")  [return-value]

Thus I see some issue in line 389 after the cache type annoation is introduced. I don't see issues introduced in other files.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe a relevant youtube video: https://www.youtube.com/watch?v=_QXlbwRmqgI

Copy link
Contributor Author

@gandhis1 gandhis1 Aug 7, 2024

Choose a reason for hiding this comment

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

The line 389 issue I fixed by converting the lambda into a regular def, which supports annotations.

I took an initial look at fixing the others as well:

  • Added an annotation for _HASH_MAP
  • _HASH_STACKS does not even appear to be used anywhere, so I couldn't figure out what it was supposed to be annotated as. _STACK is likewise not used anywhere. Can these be deleted?
  • The line 177 error sure looks like a bug? How can a hash function be a bytes constant (see snippet below)? Those are not callable and hash_func is used as a callable.
for name in _FFI_TYPE_NAMES:
    _hash_funcs[name] = b'0'
  • The line 493 error I ignored for now, as it is assigned an arbitrary attribute clear on a function. We might be able to define a more precise return type that annotates the clear as well but that is probably overkill. I ended up implementing the overkill annotation because then user code can access clear() without any errors.
  • The 462/505 async / await errors I left alone, they are a bit tricky to address without over-complicating the annotations

panel/io/cache.py Outdated Show resolved Hide resolved
panel/io/cache.py Outdated Show resolved Hide resolved
panel/io/cache.py Outdated Show resolved Hide resolved
panel/io/cache.py Outdated Show resolved Hide resolved
@MarcSkovMadsen
Copy link
Collaborator

Note to my self. I should consider contributing an example to the docstring and maybe a link to reference docs to align with other docstrings.

panel/io/cache.py Outdated Show resolved Hide resolved
@hoxbro
Copy link
Member

hoxbro commented Aug 6, 2024

I can see that @hoxbro updated to Python 3.10 in #6883.

Yep, Bokeh 3.5 dropped Python 3.9 support. So we dropped it, too, as we are hard pinned to 3.5.

@gandhis1
Copy link
Contributor Author

gandhis1 commented Aug 6, 2024

I will make the requested changes and address the Mypy errors later today.

@gandhis1 gandhis1 force-pushed the cache_decorator_annotations branch from 8a73bd5 to 58af6df Compare August 7, 2024 00:37
@gandhis1 gandhis1 force-pushed the cache_decorator_annotations branch from 079f87c to 930acab Compare August 7, 2024 01:02
@gandhis1 gandhis1 force-pushed the cache_decorator_annotations branch 2 times, most recently from 65d1459 to 4f9a51e Compare August 7, 2024 02:03
@gandhis1 gandhis1 force-pushed the cache_decorator_annotations branch from a5498d9 to d466934 Compare August 7, 2024 02:24
Copy link
Collaborator

@MarcSkovMadsen MarcSkovMadsen left a comment

Choose a reason for hiding this comment

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

With the (limited) knowledge of typing I have, I can approve.

I believe the remaining questions can should be answered by Philipp.

@MarcSkovMadsen MarcSkovMadsen added this to the v1.5.0 milestone Aug 7, 2024
@gandhis1 gandhis1 changed the title Annotate the cache decorator Annotate pn.io.cache decorator Aug 7, 2024
@hoxbro
Copy link
Member

hoxbro commented Aug 7, 2024

Having variables in TYPE_CHECKING makes them private. I prefer having them there, as it also makes it easier to read, but I'm not sure what the "standard" is.

@gandhis1
Copy link
Contributor Author

gandhis1 commented Aug 7, 2024

Having variables in TYPE_CHECKING makes them private. I prefer having them there, as it also makes it easier to read, but I'm not sure what the "standard" is.

I restored this - the errors I previously encountered when doing this was because I was missing from __future__ import annotations.

@gandhis1
Copy link
Contributor Author

gandhis1 commented Aug 7, 2024

The tests keep on failing in places I am unfamiliar with. Do these test failures actually have anything to do with this PR?

@philippjfr
Copy link
Member

Some tests seem to be a little flakey will look at those again soon, but you're good to go here!

@philippjfr philippjfr merged commit 3a7f440 into holoviz:main Aug 8, 2024
16 checks passed
@gandhis1 gandhis1 deleted the cache_decorator_annotations branch August 8, 2024 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants