Skip to content

Fix run and rpc type annotations and run with function pointer #223

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

Merged
merged 1 commit into from
May 2, 2025

Conversation

dfarr
Copy link
Member

@dfarr dfarr commented May 2, 2025

The type annotations of run and rpc had regressed to return type Handle[Generator[Any, Any, T] | T as opposed to just type T.

This PR fixes this and adds more robust "tests" (implemented as unit tests, checked by pyright) to help mitigate regressions in the future. In addition we are now wrapping the underlying function using functools.update_wrapper and binding the identity of the Function instance to the function itself.

Note:

Ideally, run and rpc would not require special type annotations to work with Function (since this class implements the __call__ method and is therefore callable). As best I can tell this is required because the python type system lacks conditional types, therefore the __call__ method is implemented as:

def __call__(self, ctx: Context, *args: P.args, **kwargs: P.kwargs) -> Generator[Any, Any, R] | R:
    return self._func(ctx, *args, **kwargs)

Which is not equivalent to Generator[Any, Any, R] and R respectively and likely the reason for the regression. If ever python adds conditional types, or we figure out a better way to implement Function we can likely remove these annotations (and switch Function run/rpc to use self._func which preserves type information) in the future.

@dfarr dfarr requested review from Tomperez98 and avillega May 2, 2025 08:04
@dfarr dfarr merged commit 87c9b93 into main May 2, 2025
8 checks passed
@dfarr dfarr deleted the resonate-type-annotations branch May 2, 2025 16:37
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.

2 participants