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

Updated functools.wraps and functools.update_wrapper to use ParamSpec… #6670

Merged
merged 23 commits into from
Feb 26, 2023

Conversation

erictraut
Copy link
Contributor

… to preserve signatures of wrapped and wrapper functions.

@github-actions

This comment has been minimized.

@erictraut
Copy link
Contributor Author

erictraut commented Dec 23, 2021

I'm not sure how to make the pytype CI script happy in this case. In fact, it's not obvious to me from the output what it's complaining about. Any suggestions? @JelleZijlstra? @srittau?

@srittau
Copy link
Collaborator

srittau commented Dec 23, 2021

This is a pytype issue. Maybe @rchen152 has some insight.

stdlib/functools.pyi Outdated Show resolved Hide resolved
@rchen152
Copy link
Collaborator

This is a pytype issue. Maybe @rchen152 has some insight.

Huh, that's a strange error. It usually occurs when some name is used twice in a stub (e.g.,

x: int
x: str

) but that's clearly not the case here. I'll take a look.

FYI we're not doing any more pytype releases until after the New Year, since most of the team is OOO, so it'll take a little longer than usually to ship a fix 😅

@srittau srittau added the status: deferred Issue or PR deferred until some precondition is fixed label Dec 24, 2021
@srittau
Copy link
Collaborator

srittau commented Dec 25, 2021

One thing to try: As we are effectively "throwing away" the wrapper function's signature (except for the __wrapped__ field) and replacing it by the wrapped function's, we could use a simple _Wrapper = TypeVar("_Wrapper", bound=Callable[..., Any]) type var. _P2 and _R2 are always used together, and _P2.args or _P2.kwargs are never accessed. This could at least help us get a clearer picture of what is going wrong.

@AlexWaygood
Copy link
Member

I wonder if inheriting from Protocol rather than Generic might reduce the primer diff somewhat?

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@srittau
Copy link
Collaborator

srittau commented Dec 25, 2021

The PR certainly seems worthwhile and correct to me, but we still need pytype and mypy fixes for it to be viable. I suggest we keep it open as "deferred" for now.

rchen152 added a commit to google/pytype that referenced this pull request Jan 4, 2022
* Don't create multiple TypeVars for the same ParamSpec. We were creating a new
  TypeVar every time we saw a ParamSpec used in a class definition, leading to
  "Duplicate top-level identifier" errors.
* Show a more helpful error message when P.args or P.kwargs is misspelled.
  Misspelling a ParamSpec attribute previously produced a mysterious "Can't
  find pyi for <ParamSpec name>" error at the dependency loading phase.

See python/typeshed#6670 and
python/typeshed#6676.

PiperOrigin-RevId: 419457152
@rchen152
Copy link
Collaborator

rchen152 commented Jan 6, 2022

The pytype issue in this PR has been fixed in the latest release (pytype-2022.1.5).

@erictraut
Copy link
Contributor Author

Thanks @rchen152! Looks like we'll need to update the pytype CI tests in typeshed to use the new version.

@srittau, do you have any remaining concerns about the PR?

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

The spark entry in the primer diff concerns me. It's going to be far more common to use @wraps with functions than with any other kind of callable, and it seems that, with this patch, mypy no longer recognises that a function will still have a __name__ attribute even if it's decorated with @wraps. I'm guessing that also applies for __defaults__, __kwdefaults__, and all the other special attributes that exist for functions but not for other callables.

@srittau
Copy link
Collaborator

srittau commented Jan 6, 2022

The primer output is still a bit concerning, but I think mostly unavoidable with this PR. Any other opinions on that? I am removing the "deferred" label for now.

@srittau srittau removed the status: deferred Issue or PR deferred until some precondition is fixed label Jan 6, 2022
@JelleZijlstra
Copy link
Member

Good news is that I can reproduce the pyspark crash with mypy 0.950, but not with current mypy master, so I think it's been fixed.

@github-actions

This comment was marked as outdated.

@AlexWaygood
Copy link
Member

The crashes do indeed seem to be fixed on mypy 0.960! Still a lot of primer output elsewhere, though :/

@srittau
Copy link
Collaborator

srittau commented May 27, 2022

Looking at the primer output, one of the main problems seems to be that mypy has a problem with constructs like the following:

def foo(func: Callable[..., Any]):
    @functools.wraps(func)  # Argument 1 to "wraps" has incompatible type "Callable[..., Any]"; expected "Callable[[VarArg(<nothing>), KwArg(<nothing>)], <nothing>]"  [arg-type]
    def inner(*args, **kwargs):
        ...

@erictraut
Copy link
Contributor Author

What is the way forward on this issue? It continues to cause pain for pyright users. Mypy now has full support for ParamSpec. Is there some other mypy change or bug fix that needs to be implemented?

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

bokeh (https://github.com/bokeh/bokeh)
+ src/bokeh/document/callbacks.py: note: In member "add_session_callback" of class "DocumentCallbackManager":
+ src/bokeh/document/callbacks.py:182:31: error: Incompatible types in assignment (expression has type "Callable[[], None]", variable has type "_Wrapped[[], None, [], None]")  [assignment]

jax (https://github.com/google/jax)
+ jax/_src/maps.py:623: error: Incompatible return value type (got "_Wrapped[[VarArg(Any), KwArg(Any)], Any, [VarArg(Any), KwArg(Any)], Any]", expected "Wrapped")  [return-value]
+ jax/_src/maps.py:623: note: "_Wrapped" is missing following "Wrapped" protocol member:
+ jax/_src/maps.py:623: note:     lower

pandas (https://github.com/pandas-dev/pandas)
+ pandas/core/strings/accessor.py:130: error: "_Wrapped[<nothing>, <nothing>, [Any, VarArg(Any), KwArg(Any)], Any]" has no attribute "__name__"  [attr-defined]

nox (https://github.com/wntrblm/nox)
+ nox/_decorators.py:34: error: Incompatible return value type (got "_Wrapped[[VarArg(Any), KwArg(Any)], Any, <nothing>, <nothing>]", expected "FunctionDecorator")  [return-value]
+ nox/_decorators.py:34: error: Argument 1 to "__call__" of "_Wrapper" has incompatible type "FunctionDecorator"; expected "Callable[[VarArg(<nothing>), KwArg(<nothing>)], <nothing>]"  [arg-type]
+ nox/_decorators.py:49: error: Incompatible types in assignment (expression has type "_Wrapped[<nothing>, <nothing>, [VarArg(Any), KwArg(Any)], Any]", variable has type "FunctionType")  [assignment]

pytest (https://github.com/pytest-dev/pytest)
+ src/_pytest/fixtures.py:1173: error: Argument 1 to "wraps" has incompatible type "FixtureFunction"; expected "Callable[[VarArg(<nothing>), KwArg(<nothing>)], <nothing>]"  [arg-type]

hydra-zen (https://github.com/mit-ll-responsible-ai/hydra-zen)
- src/hydra_zen/wrapper/_implementations.py:439: error: Incompatible types in assignment (expression has type "Zen[P, R]", variable has type "Callable[[Any], Any]")  [assignment]
+ src/hydra_zen/wrapper/_implementations.py:439: error: Incompatible types in assignment (expression has type "Zen[P, R]", variable has type "_Wrapped[P, R, [Any], Any]")  [assignment]
- src/hydra_zen/wrapper/_implementations.py:439: note: "Zen[P, R].__call__" has type "Callable[[Union[DataClass_, Type[DataClass_], Dict[Any, Any], DictConfig, str]], R]"

kopf (https://github.com/nolar/kopf)
+ kopf/_kits/webhacks.py:80: error: Argument 1 to "wraps" has incompatible type "_ServerFn"; expected "Callable[[VarArg(<nothing>), KwArg(<nothing>)], <nothing>]"  [arg-type]

spark (https://github.com/apache/spark)
+ python/pyspark/sql/udf.py:428: error: "_Wrapped[[VarArg(Any), KwArg(Any)], Any, [VarArg(Union[Column, str])], Column]" has no attribute "__name__"  [attr-defined]
+ python/pyspark/sql/connect/udf.py:155: error: "_Wrapped[[VarArg(Any), KwArg(Any)], Any, [VarArg(Union[Column, str])], Column]" has no attribute "__name__"  [attr-defined]

werkzeug (https://github.com/pallets/werkzeug)
+ src/werkzeug/formparser.py:153: error: Incompatible return value type (got "_Wrapped[<nothing>, <nothing>, <nothing>, <nothing>]", expected "F")  [return-value]

streamlit (https://github.com/streamlit/streamlit)
+ lib/streamlit/deprecation_util.py: note: In function "deprecate_func_name":
+ lib/streamlit/deprecation_util.py:81:5: error: "_Wrapped[<nothing>, <nothing>, [VarArg(Any), KwArg(Any)], Any]" has no attribute "__name__"  [attr-defined]

prefect (https://github.com/PrefectHQ/prefect)
- src/prefect/utilities/asyncutils.py:218: error: Argument 1 to "wraps" has incompatible type "T"; expected "Callable[..., object]"  [arg-type]
+ src/prefect/utilities/asyncutils.py:218: error: Argument 1 to "wraps" has incompatible type "T"; expected "Callable[[VarArg(<nothing>), KwArg(<nothing>)], <nothing>]"  [arg-type]
+ src/prefect/utilities/asyncutils.py:235: error: Need type annotation for "wrapper"  [var-annotated]
- src/prefect/utilities/asyncutils.py:241: error: "Callable[[VarArg(Any), KwArg(Any)], Any]" has no attribute "aio"  [attr-defined]
+ src/prefect/utilities/asyncutils.py:241: error: "_Wrapped[Any, Any, [VarArg(Any), KwArg(Any)], Any]" has no attribute "aio"  [attr-defined]
- src/prefect/utilities/asyncutils.py:242: error: Incompatible return value type (got "Callable[[VarArg(Any), KwArg(Any)], Any]", expected "T")  [return-value]
+ src/prefect/utilities/asyncutils.py:242: error: Incompatible return value type (got "_Wrapped[Any, Any, [VarArg(Any), KwArg(Any)], Any]", expected "T")  [return-value]

jinja (https://github.com/pallets/jinja)
+ src/jinja2/compiler.py:56: error: Incompatible return value type (got "_Wrapped[<nothing>, <nothing>, <nothing>, <nothing>]", expected "F")  [return-value]

@kkirsche
Copy link
Contributor

What is the way forward on this issue? It continues to cause pain for pyright users. Mypy now has full support for ParamSpec. Is there some other mypy change or bug fix that needs to be implemented?

Following up on this, there has been commit activity since this was asked, but this question still needs to be answered. Alex added the "needs decision" label on Aug 20, 2022, so I was wondering if any concrete steps can move this forward toward a resolution one way or another? Thanks all

@srittau
Copy link
Collaborator

srittau commented Feb 22, 2023

I'm fine with merging this. The primer output doesn't look great, but not too bad. A few type ignores are okay, until the remaining mypy problems are sorted out.

Unless someone objects, I will do so in the next few days.

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.

9 participants