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

feat: make Series generic #1412

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

EdAbati
Copy link
Contributor

@EdAbati EdAbati commented Nov 20, 2024

What type of PR is this? (check all applicable)

  • πŸ’Ύ Refactor
  • ✨ Feature
  • πŸ› Bug Fix
  • πŸ”§ Optimization
  • πŸ“ Documentation
  • βœ… Test
  • 🐳 Other

Related issues

Checklist

  • Code follows style guide (ruff)
  • Tests added
  • Documented the changes

If you have comments or can explain your changes, please do so below

Last week, I was playing with the idea of having Series generic as DataFrames for better typing.

It may be useful since it is related to the current issues/PRs around documentation/typing.

@github-actions github-actions bot added the enhancement New feature or request label Nov 20, 2024
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

thanks for doing this!

i think in stable/v1 you may need to do something like

from narwhals.typing import Series as NwSeries
Series = NwSeries[Any]

to keep it stable for shiny?

@EdAbati EdAbati marked this pull request as ready for review November 20, 2024 23:00
@EdAbati
Copy link
Contributor Author

EdAbati commented Nov 20, 2024

Thanks @MarcoGorelli for the review ✌️

shiny is happy now but stable.v1.Series doesn't have the generic typing πŸ˜• If you know any smart way to renable some parts of it let me know.

(the CI error seems unrelated)

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

Thanks, nice one! just got a question

Looking over https://narwhals-dev.github.io/narwhals/backcompat/#exceptions, we did write

we may consider making some type hints more precise.

I feel like we should loop @schloerke into the conversation - would you be OK with us making this type hint more precise? It would only affect Shiny's internal typing and so wouldn't be user-facing, and we would also make the PR to Shiny to get your CI green again

If not, no worries, the stable.v1 mechanism does allow us to only make this change in the main namespace

narwhals/stable/v1/__init__.py Outdated Show resolved Hide resolved
@schloerke
Copy link
Contributor

@MarcoGorelli Correct. https://github.com/narwhals-dev/narwhals/actions/runs/11932852152/job/33258672137?pr=1412 does break py-shiny. But as you said, it's only seems to be internal and will hopefully be a quick fix.

When testing locally, I changed the pyproject.yaml narwhals dep to

    # "narwhals>=1.10.0",
    "narwhals@git+https://github.com/EdAbati/narwhals@series-generic",

Maybe we could wrap around https://github.com/posit-dev/py-shiny/blob/f6b92d8cf49a90f3b3dbb636cd6d7fdeee244cfd/shiny/render/_data_frame_utils/_types.py#L69 and check the narwhals version and return the appropriate Series or Series[Any](?) value. Hopefully this should fix most of the issues. (2x possible remaining locations: col type and return type for helper method.)

These can probably be done within Shiny before the PR is merged? Please tag me in the py-shiny PR when it is ready πŸ˜ƒ

I currently do not have a date as to when shiny would be released after the merge. 😞

@schloerke
Copy link
Contributor

Yay for stronger typing!!

@MarcoGorelli
Copy link
Member

i just tried this at it looks like it would break Altair's typing too, so I reverted the extra commit

maybe it's OK to just introduce this in the main namespace and leave stable.v1 as it is for now

@EdAbati
Copy link
Contributor Author

EdAbati commented Nov 26, 2024

Thanks both!
I agree that if it start breaking more than 1 downstream library test, we shouldn't change stable.v1 for now. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants