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 ThemeConfig (TypedDict) #3536

Merged
merged 165 commits into from
Sep 22, 2024
Merged

Conversation

dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Aug 12, 2024

Resolves one item in #3519

Description

This PR aims to improve the UX when authoring a theme.
By utilising TypedDict(s) we can provide a very similar experience to that of writing in Vega Editor, with rich autocompletion and static validation feedback.

Main Additions

Interactions with public API

These classes are entirely isolated from the main alt.___ namespace, but can still be used anywhere a dict|Map annotation is present in existing code.
Currently:

  • The top-level object ThemeConfig is exported to altair.typing
  • All others are accessible via altair.typing.theme.

Using the classes themselves is entirely optional, as can be seen in test_theme.py.
Simply annotating ThemeConfig (and using a static type checker) allows the user to specify a theme using native python types.

Examples

Autocomplete w/ native python types

Code block

import altair as alt
from altair.typing import ThemeConfig # noqa: TCH001


@alt.register_theme("custom theme", enable=True)
def my_theme() -> ThemeConfig:
    return {""}

image

Type checking w/ mixed python|altair types

Code block

import altair as alt
from altair.typing import ThemeConfig # noqa: TCH001
from altair.typing.theme import ConfigKwds

@alt.register_theme("custom theme", enable=True)
def my_theme() -> ThemeConfig:
    return {
        "autosize": {"contains": "padding"},
        "config": ConfigKwds(font="Segoe UI", axis=9999999),
        "width": "container",
        "height": [500],
    }

image

Options for imports & runtime typing

Code block

import altair as alt
from altair.typing import ThemeConfig, theme  # noqa: F811
from altair.typing.theme import AxisConfigKwds
from vega_datasets import data

custom_theme = ThemeConfig(
    config=theme.ConfigKwds(
        axis=AxisConfigKwds(grid=True, labelFont="system-ui"),
        axisX=AxisConfigKwds(labelOpacity=0.5),
        circle=theme.MarkConfigKwds(fill="purple"),
    )
)

print(
    f"{custom_theme!r}\n\n"
    f"The above theme was defined using {ThemeConfig.__name__!r}, "
    f"but at runtime the type is {type(custom_theme).__name__!r}"
)

# Manual registration
alt.themes.register("demo", lambda: custom_theme)
alt.themes.enable("demo")

alt.Chart(data.cars()).mark_circle().encode(x="Horsepower", y="Miles_per_Gallon")

image

@register_theme doc

Default

Code block

import altair as alt
from altair.typing import ThemeConfig
from altair.typing.theme import ConfigKwds
from vega_datasets import data


source = data.stocks()
lines = (
    alt.Chart(source, title=alt.Title("Stocks"))
    .mark_line()
    .encode(x="date:T", y="price:Q", color="symbol:N")
    .interactive(bind_y=False)
)
lines

image

With a registered theme

Code block

@alt.register_theme("param_font_size", enable=True)
def custom_theme_2() -> ThemeConfig:
    sizes = 12, 14, 16, 18, 20
    return {
        "autosize": {"contains": "content", "resize": True},
        "background": "#F3F2F1",
        "config": ConfigKwds(
            axisX={"labelFontSize": sizes[1], "titleFontSize": sizes[1]},
            axisY={"labelFontSize": sizes[1], "titleFontSize": sizes[1]},
            font="'Lato', 'Segoe UI', Tahoma, Verdana, sans-serif",
            headerColumn={"labelFontSize": sizes[1]},
            headerFacet={"labelFontSize": sizes[1]},
            headerRow={"labelFontSize": sizes[1]},
            legend={"labelFontSize": sizes[0], "titleFontSize": sizes[1]},
            text={"fontSize": sizes[0]},
            title={"fontSize": sizes[-1]},
        ),
        "height": {"step": 28},
        "width": 350,
    }


lines

image

Early POC Demo

These screenshots were taken mid-development

Providing fully nested auto-complete

image

Type checking feedback

image

Tasks

Mostly avoiding use `list` unless properties of it are needed.
Also using `set` comprehensions
…tion`

Provides all existing functionality.
Adds `use_concrete`, for upcoming `TypedDict` changes
- Adapted to new `get_python_type_representation`
- Avoid nested listcomp
- Avoid repeated sorting
- Moved the generic parts to `_generate_sig_args`
Will be adding a docstring with more detail
Handled by `jsonschema_to_python_types`
@dangotbanned
Copy link
Member Author

dangotbanned commented Sep 18, 2024

@mattijn @joelostblom

FYI the conversation marked as unresolved is actually resolved.

I haven't updated the status of it yet, to block merging until you've both seen the updated description

Otherwise this should be ready to merge 👍

@joelostblom
Copy link
Contributor

I don't have much to comment on the changes to the import, so will refer to @mattijn for that.

On first look, the class based syntax looks maybe a bit verbose:

image

So I would probably opt for the combination with dicts myself:

image

But maybe I will change my mind when I try them; it's great that both exist and support autocompletion!

@dangotbanned
Copy link
Member Author

@mattijn I'm planning to merge this tomorrow if you don't have any objections.

In #3600 I've been trying to work around not having the refactored tools/... and have made a lot of progress; but I need to start making changes in tools.generate_schema_wrapper.py and tools.schemapi.utils.py soon for generating alt.expr.__init__.py

@mattijn
Copy link
Contributor

mattijn commented Sep 22, 2024

Thanks for asking! I still cannot understand why this ThemeConfig feature is available within the altair.types. I thought we declared that altair.types is the interface of public types within altair. But now we are placing features such as ThemeConfig in there. Is it a type? Therefore it does not help me with discoverability either.

Also, as a user, I really prefer a feature explanation that I can copy and paste. I don't see that in this PR. I see a few screenshots, but that is really not the advocated route for a Minimal, Reproducible Example. For me it is still not clear how I can use this from the PR description.

I found the following in the code:

import altair as alt
from altair.typing import ThemeConfig
from vega_datasets import data


@alt.register_theme("param_font_size", enable=True)
def custom_theme() -> ThemeConfig:
    sizes = 12, 14, 16, 18, 20
    return {
        "autosize": {"contains": "content", "resize": True},
        "background": "#F3F2F1",
        "config": {
            "axisX": {"labelFontSize": sizes[1], "titleFontSize": sizes[1]},
            "axisY": {"labelFontSize": sizes[1], "titleFontSize": sizes[1]},
            "font": "'Lato', 'Segoe UI', Tahoma, Verdana, sans-serif",
            "headerColumn": {"labelFontSize": sizes[1]},
            "headerFacet": {"labelFontSize": sizes[1]},
            "headerRow": {"labelFontSize": sizes[1]},
            "legend": {"labelFontSize": sizes[0], "titleFontSize": sizes[1]},
            "text": {"fontSize": sizes[0]},
            "title": {"fontSize": sizes[-1]},
        },
        "height": {"step": 28},
        "width": 350,
    }


source = data.stocks()
lines = (
    alt.Chart(source, title=alt.Title("Stocks"))
    .mark_line()
    .encode(x="date:T", y="price:Q", color="symbol:N")
)
lines.interactive(bind_y=False)

That works! But I also see another approach. Here you define a custom_theme from the ThemeConfig

from altair.typing import ThemeConfig, theme

custom_theme = ThemeConfig(
    config=theme.ConfigKwds(
        axis=theme.AxisConfigKwds(grid=True, labelFont="system-ui"),
        circle=theme.MarkConfigKwds(fill="purple"),
    )
)

custom_theme

How do I register this as theme?

I searched for register_theme within the altair documentation page, but nothing was returned https://altair-viz.github.io/search.html?q=register_theme. A brief description to the documentation page would help the average user a lot here.

Last, after I enabled the custom theme, how can I de-register this custom theme and go back to the default settings?

Thank you for your patience!

@dangotbanned
Copy link
Member Author

Thanks for your response @mattijn!

I'm going to try my best to address your concerns in a few follow-up comments.

Just finished writing up the first (of 2 or 3), so please know there is more to come

@dangotbanned
Copy link
Member Author

dangotbanned commented Sep 22, 2024

Why altair.typing

@mattijn

I still cannot understand why this ThemeConfig feature is available within the altair.types. I thought we declared that altair.types is the interface of public types within altair. But now we are placing features such as ThemeConfig in there. Is it a type? Therefore it does not help me with discoverability either.

So in the description I provided a link that can clear this up:

This PR aims to improve the UX when authoring a theme.
By utilising TypedDict(s) ...

I've picked out some key quotes, but I would really recommend reading the section Using TypedDict Types as this illustrates how TypedDict(s) differ from regular classes:

  • The created TypedDict type object is not a real class object.
  • When called, the TypedDict type object returns an ordinary dictionary object at runtime
  • In particular, TypedDict type objects cannot be used in isinstance() tests such as isinstance(d, Movie).

Back to your question:

Is it a type?

I'm afraid this is complex question to answer, but I will do my best to summarize.

  • TypedDict is defined in typing - not as a class - but as a function
    • Screenshot
  • There is currently an open proposal PEP 747 which would define (among other symbols) that a TypedDict is a TypeForm

The short (but seemingly a non)answer is that it is not a real class.

I believe that if we were to add these TypedDict(s) to the top-level, that it could cause confusion for users that (understandably) expect them to work the same as SchemaBase classes.

Alternatives

In #3536 (comment) I mentioned some related issues, but can now see I forgot to elaborate on what I would prefer for v6.

I would like to have used altair.theme as the namespace instead of altair.typing.theme.
However, this would be very easy to mistake for altair.themes - which is a variable storing a registry of themes.

For v6 I would propose the following api changes - and if there is any support I will open a new issue:

  • altair.typing.theme -> altair.theme
    • Including dropping altair.typing.ThemeConfig, which is already included in the above
  • @altair.register_theme -> @altair.theme.register
  • New function: altair.theme.enable
  • Remove altair.themes top-level export

This would consolidate all theme related functionality behind a single namespace:

from altair import theme

@theme.register("custom 1", enable=False)
def custom_theme() -> theme.ThemeConfig:
    return {
        "autosize": {"contains": "content", "resize": True}, 
        "config": theme.ConfigKwds(
            circle=theme.MarkConfigKwds(fill="purple")
        ),
    }

...

theme.enable("custom 1")
...
theme.enable("default")

...

@dangotbanned
Copy link
Member Author

dangotbanned commented Sep 22, 2024

Screenshots vs Code blocks

@mattijn

Also, as a user, I really prefer a feature explanation that I can copy and paste. I don't see that in this PR. I see a few screenshots, but that is really not the advocated route for a Minimal, Reproducible Example. For me it is still not clear how I can use this from the PR description.

I have now updated the PR description with collapsible code blocks to accompany each screenshot.

I would absolutely agree that providing a Minimal, Reproducible Example is helpful, although I thought that related more to bugs/issues?

There were a few reasons leaned towards screenshots:

  1. (Most importantly) the key benefits to this PR are seen in an IDE - not at runtime:
    • Autocompletion for dict keys
    • Static type checking feedback
    • Documentation
  2. I had already provided a comprehensive test suite containing code for 13 theme definitions when asked to update the description
    • I seem to have misinterpreted your request as wanting something more than copy/pasting these tests
    • I did mention them and provide a link before presenting the screenshots

      Using the classes themselves is entirely optional, as can be seen in test_theme.py.

  3. Most other typing-focused PRs have also used screenshots to communicate the impact

@dangotbanned
Copy link
Member Author

dangotbanned commented Sep 22, 2024

@alt.register_theme

@mattijn

How do I register this as theme?

I've updated the PR description, this is the example under the heading Options for imports & runtime typing

I searched for register_theme within the altair documentation page, but nothing was returned altair-viz.github.io/search.html?q=register_theme. A brief description to the documentation page would help the average user a lot here.

Well spotted!
I wasn't aware this hadn't been included in the API reference yet, but will open a new issue/PR to address it - since that is orthogonal to this PR.

I was waiting until this PR was finished before introducing @register_theme to the User Guide, since they complement eachother well.

There are a few other changes I've noted for the User Guide in #3519, including ThemeConfig as mentioned in #3536 (comment)


Last, after I enabled the custom theme, how can I de-register this custom theme and go back to the default settings?

Exactly the same way as documented in https://altair-viz.github.io/user_guide/customization.html#defining-a-custom-theme

In #3526 I've described this as a convenience for the common case where you use a single theme.
That isn't the only possible use-case, but it is the motivating one

@dangotbanned dangotbanned merged commit 712680b into vega:main Sep 22, 2024
13 checks passed
@dangotbanned
Copy link
Member Author

dangotbanned commented Sep 22, 2024

I didn't mean to trigger a merge to main with 712680b

I was trying to fix the conflicts but now I see the comment I'd mentioned was unresolved in #3536 (comment) has been resolved

@binste can we revert this so that @mattijn has time to respond please?

Edit

See #3536 (comment)

mattijn added a commit that referenced this pull request Sep 22, 2024
@dangotbanned
Copy link
Member Author

Thanks @mattijn for 3d97fae

I'm not sure what would be the best way to proceed here, should I open a new PR?

@mattijn
Copy link
Contributor

mattijn commented Sep 22, 2024

Thanks for the added explanations! Appreciated. I like the suggestion you provide in #3536 (comment) to consolidate everything under a single namespace, including the proposed ThemeConfig. I’m fine with doing that now, rather than releasing a version first and requiring deprecation immediately after.

However, instead of phasing out the plural altair.themes in favour of the singleton altair.theme can we consolidate this PR's work into the existing altair.themes? For example, can we have altair.themes.ThemeConfig (and altair.themes.AxisConfigKwds, etc.) aligning with the existing altair.themes.register() and altair.themes.enable().

Aesthetically I like the singleton altair.theme, but I think sticking with altair.themes is more practical, if that is also an option. Can you reflect on this?

Thank you for the added explanations regarding the type of ThemeConfig! I currently don't think it needs to be registered as part of interface of altair its public types. The current _TypedDictMeta type seems fine and even if it later becomes something like TypeForm, it's essentially a dictionary (great!) at runtime if I understand correctly.

Also, much appreciated for including both code blocks and screenshots! Great combination as such!

@mattijn
Copy link
Contributor

mattijn commented Sep 22, 2024

I don't think it is possible to re-open a merged PR, even when the changes are reverted with the latest commit, probably the best option is to open a new PR😔

@dangotbanned
Copy link
Member Author

Thanks for the added explanations! Appreciated. I like the suggestion you provide in #3536 (comment) to consolidate everything under a single namespace, including the proposed ThemeConfig. I’m fine with doing that now, rather than releasing a version first and requiring deprecation immediately after.

Thank you for the added explanations regarding the type of ThemeConfig! I currently don't think it needs to be registered as part of interface of altair its public types. The current _TypedDictMeta type seems fine and even if it later becomes something like TypeForm, it's essentially a dictionary (great!) at runtime if I understand correctly.

Also, much appreciated for including both code blocks and screenshots! Great combination as such!

Thanks @mattijn


However, instead of phasing out the plural altair.themes in favour of the singleton altair.theme can we consolidate this PR's work into the existing altair.themes? For example, can we have altair.themes.ThemeConfig (and altair.themes.AxisConfigKwds, etc.) aligning with the existing altair.themes.register() and altair.themes.enable().

Aesthetically I like the singleton altair.theme, but I think sticking with altair.themes is more practical, if that is also an option. Can you reflect on this?

Okay so rather than fully revert this PR, I think the changes we want are strictly concerning a small refactor of the public interface.
#3526 was merged after the last release, so renaming + moving @register_theme is all good

I propose:

  • We continue this discussion in a new issue
    • On the condition that resolving it is blocking for the next release
  • Whatever solution we end up on becomes a small refactor PR to implement this interface
  • (If necessary) update the title(s), description(s) for feat: Adds @register_theme decorator #3526, feat: Adds ThemeConfig (TypedDict) #3536
    • Either noting the interface changed prior to release
    • Or simply replacing references to the new symbols/import locations

How does that sound to you?

@mattijn
Copy link
Contributor

mattijn commented Sep 23, 2024

The changes requested were only concerning a small refactor of the public interface. Proposal sounds good!

@dangotbanned dangotbanned deleted the config-typed-dict branch October 6, 2024 17:36
dangotbanned added a commit that referenced this pull request Oct 13, 2024
I commented this out in #3536 to identify remaining typing issues.

All of `"./tools/**/*.py"` is now typed, so re-enabling in the future is very unlikely
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