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(typing): Adds public altair.typing module #3515

Merged
merged 36 commits into from
Aug 8, 2024

Conversation

altair/typing.py Outdated Show resolved Hide resolved
@mattijn
Copy link
Contributor

mattijn commented Aug 3, 2024

Again, just FYI @dangotbanned: https://github.com/vega/.github?tab=readme-ov-file#adding-maintainers 😉

@dangotbanned
Copy link
Member Author

Again, just FYI @dangotbanned: https://github.com/vega/.github?tab=readme-ov-file#adding-maintainers 😉

Alright I'm interested @mattijn, if you wanna hold a vote.

No hard feelings if I remain a contributor - I'm still very keen to continue working on altair regardless

@binste
Copy link
Contributor

binste commented Aug 3, 2024

Good idea, thanks @dangotbanned for opening it! Yeah the reverse works. I'll commit to this branch tomorrow. Of course if you want to take a stab at it today, feel free to go for it! As long as we're not working on it at the same time :)

Again, just FYI @dangotbanned: https://github.com/vega/.github?tab=readme-ov-file#adding-maintainers 😉

Alright I'm interested @mattijn, if you wanna hold a vote.

No hard feelings if I remain a contributor - I'm still very keen to continue working on altair regardless

Awesome! Fully agree with Mattijn. It would make it easier for you to collaborate with us and also as a recognition of the significant contributions you made to Altair recently. I've created a vote in the maintainers Slack channel. It's up for 7 days, I'll let you know afterwards.

@dangotbanned
Copy link
Member Author

dangotbanned commented Aug 3, 2024

Awesome! Fully agree with Mattijn. It would make it easier for you to collaborate with us and also as a recognition of the significant contributions you made to Altair recently. I've created a vote in the maintainers Slack channel. It's up for 7 days, I'll let you know afterwards.

Thanks @binste @mattijn really appreciate it

Good idea, thanks @dangotbanned for opening it! Yeah the reverse works. I'll commit to this branch tomorrow. Of course if you want to take a stab at it today, feel free to go for it! As long as we're not working on it at the same time :)

All yours as of 51a84a5 (#3515)

I got more of it done than I was expecting, not fully there yet.

Will add some notes tomorrow, but feel free to shoot over any questions if I havent got to it yet.

altair/typing.py Outdated
else:
from typing_extensions import TypeAlias

ChannelType: TypeAlias = Union[str, Mapping[str, Any], Any]
Copy link
Member Author

@dangotbanned dangotbanned Aug 4, 2024

Choose a reason for hiding this comment

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

@binste I know I removed ChannelType here, but altair.typing could define the precise types for each of the positional-args.

pola-rs/polars#17995 (comment)

image

E.g. ChannelX, ChannelY, ..., or similar?

These did vary between each method, but I think this would be 6 unique channel type aliases total:

Unsure if there would be more needed for the yet-unwritten methods

Copy link
Contributor

Choose a reason for hiding this comment

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

You already got very far! Great to see that we're almost there 😄

I'm going through it now but I'm also wondering if we maybe just want to specify, for each channel, the types separately. ChannelX, ChannelY, etc. works for me regarding naming. So basically what's there now for EncodingKwgs but as individual type aliases.

I don't know if we then even need a TypedDict, what do you think? Unpack supports the usage suggested in pola-rs/polars#17995 (comment) only for Python >= 3.12. And I think the plot methods should then only accept **kwargs: Unpack[EncodeKwgs] as else the arguments which are explicitly defined are defined twice. See typing docs.

Downside would be that in Polars, users only get autocomplete and type checking for the channels which are explicitly defined but for simple exploratory charts, this could be enough?

Copy link
Member Author

@dangotbanned dangotbanned Aug 4, 2024

Choose a reason for hiding this comment

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

You already got very far! Great to see that we're almost there 😄

Thanks @binste for such a thorough response!

And I think the plot methods should then only accept **kwargs: Unpack[EncodeKwds] as else the arguments which are explicitly defined are defined twice. See typing docs.

I was trying to demonstrate the correct way to use Unpack there, to avoid that issue (the second one):

def foo(name, **kwargs: Unpack[Movie]) -> None: ...     # WRONG! "name" will always bind to the first parameter.

def foo(name, /, **kwargs: Unpack[Movie]) -> None: ...  # OK! "name" is a positional-only parameter,
           # ^^^                                        # so **kwargs can contain a "name" keyword.
Long screenshot

image

I don't know if we then even need a TypedDict, what do you think? Unpack supports the usage suggested in pola-rs/polars#17995 (comment) only for Python >= 3.12.

I'm not sure there's an issue with typing_extensions.Unpack which I used unconditionally there?
Usually ruff or pylance will warn me when I do something that isn't allowed in py3.8

image


@binste

Downside would be that in Polars, users only get autocomplete and type checking for the channels which are explicitly defined but for simple exploratory charts, this could be enough?

@dangotbanned comment

You could then repeat the /, **kwargs: Unpack[EncodeKwds] for the other methods - maybe changing the positional-only ones if needed

This was essentially the compromise of that, if that makes sense?
You have your common ones that can be called without kwargs, but more options with kwargs

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you're absolutely right! Thanks for the detailed explanation. Lost a bit the overview there but this brought me back on track :)

Just tested /, **kwargs: Unpack[EncodeKwds] with typing_extensions.Unpack on Python 3.11 and works as expected.

Reviewing the rest now.

Copy link
Member Author

Choose a reason for hiding this comment

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

@MarcoGorelli when you see this

The need for this solution is that alt.Chart.encode has alphabetically ordered parameters, whereas pl.DataFrame.plot has:

  • pos-only (in a different order)
  • *args
  • **kwargs

Copy link
Member Author

Choose a reason for hiding this comment

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

Marking as unresolved purely so this is easier to find during review

It is actually resolved unless #3515 (comment) turns up anything new

binste and others added 8 commits August 6, 2024 06:33
…o API references in docs. Remove is_chart_type from 'API' docs section as it's now in typing. Rename '_EncodeKwds' to 'EncodeKwds'
Would have only been needed before adding an `__all__`
Updates all references, docs, `ruff` rule
If this is needed for every call, it belongs in the function itself
Copy link
Member Author

@dangotbanned dangotbanned left a comment

Choose a reason for hiding this comment

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

Delivering as promised in #3515 (comment)

This is a mostly visual review @binste but I think it is really important to make sure the result of this is as helpful as possible.

@@ -72,6 +72,17 @@
:nosignatures:

{api_classes}

Type Hints
Copy link
Member Author

Choose a reason for hiding this comment

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

@binste definitely agree with having a section for this in the API reference!

I did some testing though and my suggestion would be holding off on this until after 5.4.0.
We'll probably need to experiment a bit to get everything looking consistent, and I wouldn't want that blocking @MarcoGorelli getting their hands altair.typing for polars.

Type Hints section

image

TypeAlias

image

TypeAliasType

image

TypeAlias (w/ dangling docstring)

This "doc" is missing, but sphinx can support this

image

TypedDict

For this I think the Methods & Attributes sections aren't helpful enough for how much length they add?

image

We could still keep the relevant code, but just leave it out of the toctree for now?


I've had my eye on switching to sphinx-autoapi to more easily customise when-then-otherwise. The jsonschema docs are built with it instead of sphinx-autodoc.

I think these could be handled together in another issue focused on the API reference.
What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

As we now define our public API via the API section (#3488) I think we should offer this guarantee to polars that there are no breaking changes in altair.typing within minor versions. As 5.4.0 will likely be the minimum required version, it would be great to already have it.

I fully agree with you that the docs can be made more user-friendly for these objects. However, I do not (yet) see it as a blocking issue. When I generate the docs, all type hint aliases are correctly identified by sphinx as such, nothing is identified as TypeAliasType as in your screenshot:

image

image

Did you modify anything to produce this on purpose or do we have a difference? Could you maybe try again with hatch run doc:clean-generated && hatch run doc:build-html?

Copy link
Member Author

@dangotbanned dangotbanned Aug 7, 2024

Choose a reason for hiding this comment

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

As we now define our public API via the API section (#3488) I think we should offer this guarantee to polars that there are no breaking changes in altair.typing within minor versions. As 5.4.0 will likely be the minimum required version, it would be great to already have it.

Yeah you do make a good point @binste, great to see (#3488) mentioned!

I fully agree with you that the docs can be made more user-friendly for these objects. However, I do not (yet) see it as a blocking issue. When I generate the docs, all type hint aliases are correctly identified by sphinx as such, nothing is identified as TypeAliasType as in your screenshot:

So I don't think any of them were identified with the labels (TypeAlias, TypeAliasType, ..., TypedDict) I gave in #3515 (comment).
I used those in the comment to explain why sphinx had treated them differently.

Did you modify anything to produce this on purpose or do we have a difference? Could you maybe try again with hatch run doc:clean-generated && hatch run doc:build-html?

Didn't modify anything to produce that intentionally.
I run on a clean build, here is another screenshot w/ another build - in case this is sphinx version related

Screenshot take 2

image

Maybe the issue is hatch isn't updating your sphinx, since the dependencies don't state a minimum version?

altair/pyproject.toml

Lines 84 to 85 in 95f654f

doc = [
"sphinx",

Otherwise, maybe it is the python version we're using. I'm using 3.12, which would be the first one to introduce TypeAliasType.
Is it possible that sphinx would only use the symbol if built using sys.version_info >= (3, 12)?
That seems odd to me, as this was using typing_extensions.

Copy link
Member Author

Choose a reason for hiding this comment

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

@binste hopefully we'll get the same sphinx output now I've merged this #3527

Copy link
Contributor

Choose a reason for hiding this comment

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

Weird, even with #3527, I don't get this. I'm using Python 3.11 so maybe it's something like this...

I can build the docs with this setup when making the release. I'd suggest that if I merge this on the weekend and then make the release as-is. This makes it part of the public API and we can figure out doc issues at a later stage. Let me know if I'm missing anything, thanks! :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Weird, even with #3527, I don't get this. I'm using Python 3.11 so maybe it's something like this...

I can build the docs with this setup when making the release. I'd suggest that if I merge this on the weekend and then make the release as-is. This makes it part of the public API and we can figure out doc issues at a later stage. Let me know if I'm missing anything, thanks! :)

I agree @binste, if you have consistency with 3.11 then go with that for now 👍

Also, and this is non-blocking for me, but I'd suggest renaming the section from Type Hints -> Typing

Has been fun working with you on this

Copy link
Contributor

Choose a reason for hiding this comment

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

Done 👍 And same, was fun :)

tools/generate_schema_wrapper.py Show resolved Hide resolved
dangotbanned added a commit to dangotbanned/altair that referenced this pull request Aug 7, 2024
jonmmease pushed a commit that referenced this pull request Aug 7, 2024
* ci: Bump `vl-convert-python>=1.6.0`

Likely the source of #3523 (comment)

* ci: Bump `sphinx>=8.0.0`

Possibly resolves issue #3515 (comment)
@binste
Copy link
Contributor

binste commented Aug 8, 2024

@mattijn Just pinging you in case you want to have a look at this. Good to go from my side. I'll wait for @dangotbanned's final confirmation as well. I'd like to merge this on the weekend and cut the release so that they can continue with the work in Polars.

@mattijn
Copy link
Contributor

mattijn commented Aug 8, 2024

I'm on holiday coming period, trust you here👍

@binste binste merged commit ee7fd19 into vega:main Aug 8, 2024
13 checks passed
@dangotbanned dangotbanned deleted the public-typing branch August 8, 2024 20:49
dangotbanned added a commit to dangotbanned/altair that referenced this pull request Aug 16, 2024
Didn't fix the issue vega#3515 (comment)
Caused a regression when (locally) running `hatch test --all`

Partially reverts vega#3527
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.

polars.DataFrame.plot support tracking
5 participants