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: Adds ChartType type and type guard is_chart_type. Change PurePath to Path type hints #3420

Merged
merged 17 commits into from
May 23, 2024

Conversation

dangotbanned
Copy link
Member

This PR mostly relates to type hints and associated tools, from the perspective of a downstream library author.

Note

Below was automatically added, but is only from the first commit.
The others also have descriptions, not sure if you'd like them all collected into the PR as well.

However, it was the motivation behind the contributions generally so I've left it in.

First commit description

I'm working on a library that uses altair, and looking to make it an optional dependency. During this process, I thought ChartType and is_chart_type might be a good fit in altair itself.

I don't believe any existing altair code would directly benefit from these additions - but you could likely reduce some isinstance checks with type narrowing functions like is_chart_type.
For example altair/vegalite/v5/api.py has 63 occurrences of isinstance.


per CONTRIBUTING.md

In particular, we welcome companion efforts from other visualization libraries to render the Vega-Lite specifications output by Altair. We see this portion of the effort as much bigger than Altair itself

Towards this aim, it could be beneficial to provide typing constructs/tools for downstream use.

Regarding my specfic use case, the below Protocol - I believe - describes the concept of a _/Chart, for the purposes of writing to file:

from typing import Any, Protocol, runtime_checkable

@runtime_checkable
class AltairChart(Protocol):
    data: Any

    def copy(self, *args: Any, **kwargs: Any) -> Any: ...
    def save(self, *args: Any, **kwargs: Any) -> None: ...
    def to_dict(self, *args: Any, **kwargs: Any) -> dict[Any, Any]: ...
    def to_html(self, *args: Any, **kwargs: Any) -> str: ...

Due to the number of symbols in altair and heavy use of mixins, coming to this conclusion can be a stumbling block.

However, exporting things like ChartType, is_chart_type, AltairChart could be a way to expose these shared behaviours - without requiring any changes to the core API.


I'd likely have written this differently if altair was already using PEP 563, but imagine introducing that could impact the autogenerated code.
Did want to note it though, as it might help with some circular references.

I'm working on a library that uses `altair`, and looking to make it an optional dependency. During this process, I thought `ChartType` and `is_chart_type` might be a good fit in `altair` itself.

I don't believe any existing `altair` code would directly benefit from these additions - but you could likely reduce some `isinstance` checks with [type narrowing](https://typing.readthedocs.io/en/latest/spec/narrowing.html) functions like `is_chart_type`.
For example `altair/vegalite/v5/api.py` has **63** occurrences of `isinstance`.

---

per [CONTRIBUTING.md](https://github.com/vega/altair/blob/main/CONTRIBUTING.md)
> In particular, we welcome companion efforts from other visualization libraries to render the Vega-Lite specifications output by Altair. We see this portion of the effort as much bigger than Altair itself

Towards this aim, it could be beneficial to provide `typing` constructs/tools for downstream use.

Regarding my specfic use case, the below [Protocol](https://typing.readthedocs.io/en/latest/spec/protocol.html#runtime-checkable-decorator-and-narrowing-types-by-isinstance) - I believe - describes the concept of a `_/Chart`, for the purposes of writing to file:

```py
from typing import Any, Protocol, runtime_checkable

@runtime_checkable
class AltairChart(Protocol):
    data: Any

    def copy(self, *args: Any, **kwargs: Any) -> Any: ...
    def save(self, *args: Any, **kwargs: Any) -> None: ...
    def to_dict(self, *args: Any, **kwargs: Any) -> dict[Any, Any]: ...
    def to_html(self, *args: Any, **kwargs: Any) -> str: ...
```

Due to the number of symbols in `altair` and heavy use of mixins, coming to this conclusion can be a stumbling block.

However, exporting things like `ChartType`, `is_chart_type`, `AltairChart` could be a way to expose these shared behaviours - without requiring any changes to the core API.
`TopLevelMixin.save` calls a function accepting `pathlib` objects, but the two didn't share the same `Union` for `fp`.
When using `pyright`, the following warning is presented:

> Argument of type "Path" cannot be assigned to parameter "fp" of type "str | IO[Unknown]" in function "save"
> Type "Path" is incompatible with type "str | IO[Unknown]"
> "Path" is incompatible with "str"
> "Path" is incompatible with "IO[Unknown]"

This fix avoids the need to use type ignores on the public facing API, as a user.
By normalizing `fp` in `save`, it simplifies the signatures of two other functions and removes the need for `str` specific suffix handling.
…sion

Although there are branches it isn't needed, I think this is easier to read and maintain.

Very low priority change, just something I noticed while reading.
@dangotbanned
Copy link
Member Author

I'm unsure if I've followed the contributing guide correctly.

I expected running this locally, without errors, would be safe to push:

hatch run test

I'll hold of on any more commits for now, please let me know if I've missed something

@mattijn
Copy link
Contributor

mattijn commented May 13, 2024

Thanks for the PR! Without giving a detailed review I can say that the CI errors originate from the auto-generated code.
You will likely need to run the following to have these generated:

hatch run python tools/generate_schema_wrapper.py

See also: https://github.com/vega/altair/blob/main/NOTES_FOR_MAINTAINERS.md#auto-generating-the-python-code.

@dangotbanned
Copy link
Member Author

Thanks for the PR! Without giving a detailed review I can say that the CI errors originate from the auto-generated code. You will likely need to run the following to have these generated:

hatch run python tools/generate_schema_wrapper.py

See also: main/NOTES_FOR_MAINTAINERS.md#auto-generating-the-python-code.

Thanks, will give this a try.

Believe these should be public, but it seems like the issue at build-time is that I'd be extending the public API.
@dangotbanned
Copy link
Member Author

Looks like the same as error as #3418

@dangotbanned
Copy link
Member Author

@mattijn still getting the same error, not anywidget as in #3419 (comment)

Is there anything I can do to help here?

@mattijn
Copy link
Contributor

mattijn commented May 16, 2024

Cleaning the logs and re-running all jobs make all tests pass. This PR is ready for review. @binste has been working hard in the last year to make Altair a typed library, so I hope he can find some time in coming days/week.
I'm not sure if it is related to this _is_chart_type, maybe we could introduce something along the line of ChartLike like we have DataFrameLike.

altair/utils/save.py Outdated Show resolved Hide resolved
altair/utils/save.py Outdated Show resolved Hide resolved
@binste
Copy link
Contributor

binste commented May 18, 2024

Thanks @dangotbanned! I'll try to have a detailed look at this soon.

Copy link
Contributor

@binste binste left a comment

Choose a reason for hiding this comment

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

Thanks again for the PR! Agree that this is a helpful utility to have in Altair.

I've made ChartType and is_chart_type public and bumped typing_extensions so it includes TypeIs. I've added some other smaller comments after which I think this is good to go.

altair/utils/save.py Outdated Show resolved Hide resolved
altair/utils/save.py Outdated Show resolved Hide resolved
altair/utils/save.py Outdated Show resolved Hide resolved
altair/utils/save.py Outdated Show resolved Hide resolved
altair/utils/save.py Show resolved Hide resolved
altair/utils/save.py Outdated Show resolved Hide resolved
altair/utils/save.py Outdated Show resolved Hide resolved
@dangotbanned dangotbanned requested a review from binste May 23, 2024 09:49
@binste binste changed the title feat: Type hints: Adds ChartType and guard is_chart_type feat: Adds ChartType type and type guard is_chart_type May 23, 2024
@binste binste changed the title feat: Adds ChartType type and type guard is_chart_type feat: Adds ChartType type and type guard is_chart_type. Change PurePath to Path type hints May 23, 2024
Copy link
Contributor

@binste binste left a comment

Choose a reason for hiding this comment

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

Thanks again! Looks good to me.

@binste binste merged commit 18a2c3c into vega:main May 23, 2024
13 checks passed
@dangotbanned dangotbanned deleted the ecosystem-utils branch May 25, 2024 08:20
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.

4 participants