-
Notifications
You must be signed in to change notification settings - Fork 170
feat(typing): Make Implementation less opaque
#3016
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
Conversation
- Based on (python/mypy#9937) - Issue raised privately by @FBruzzesi
|
I didn't take a look at this yet, but I want to add some context if someone else is reviewing it and wonders what the hell is happening. In #2983 I am creating a new series via a snippet such as: x: SeriesT
impl = x.implementation
new_series(..., backend=impl)Since in #3002 we too good, now the type checker complains since I then started a series of adjustments which led to nowhere, so I asked for help and here is @dangotbanned making magic |
the `pyspark` parts are going to be needed next
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.
Thanks @dangotbanned - I left a few comments/questions, the main one being: I would already be happy to have the distinction between eager and lazy, so that we know that a DataFrame and a Series always returns a eager implementation (although a lazyframe might return any implementation in practice, since class EagerDataFrame(CompliantDataFrame[...], CompliantLazyFrame[...], ...)
Avoids the need for runtime branching in the descriptor
will work on simplifying later
Check added in #2957
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 am quite happy with this! Thanks @dangotbanned
@MarcoGorelli do you have any objection and/or strong opinion against it?
| """Return True if `impl` allows eager operations.""" | ||
| return impl in { | ||
| Implementation.CUDF, | ||
| Implementation.MODIN, |
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.
👌🏼
Thanks @FBruzzesi!
I cannot stress this enough ... This PR is easily the most complex bit of typing I've ever worked on 😳 But I had no idea this was possible a week ago when I started it 😂 |
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.
just one comment, else looks good, thanks!
Maybe solution for #3016 (comment)
`v1` is different for the frame cases #3016 (comment)

What type of PR is this? (check all applicable)
Related issues
IntoBackendgeneric #3002type: ignoreworkaround python/mypy#9937)Implementationless opaque #3016 (comment))Checklist
Examples
This PR gives us static typing for
Implementationat the narwhals-level:Adapting (https://github.com/narwhals-dev/narwhals#example), the fun stuff happens when we use aliases/type variables.
IntoFrameTexpands to all validImplementation's:However, if we tweak that to use
IntoDataFrameTit can be narrowed to all ofEagerAllowed:The inverse also works, so when we use
IntoLazyFrameTit can be narrowed to all ofLazyAllowed:Note
See tests/implementation_test.py for more exhaustive examples 😅