Skip to content

fix: type issues with Chart mark methods.#3936

Merged
mattijn merged 6 commits intovega:mainfrom
alec-bike:api-type-fix
Jan 18, 2026
Merged

fix: type issues with Chart mark methods.#3936
mattijn merged 6 commits intovega:mainfrom
alec-bike:api-type-fix

Conversation

@alec-bike
Copy link
Contributor

Chart mark_line and similar methods trigger a basedpyright type check warning for unknown argument. This PR adds type annotations to remove the warning. It also fixes some minor type warnings raised by ty and basedpyright.

Resolves #3870.

@alec-bike alec-bike changed the title fix: type issues with Chart mark_* methods. fix: type issues with Chart mark_* methods. Jan 8, 2026
@alec-bike alec-bike force-pushed the api-type-fix branch 5 times, most recently from 9140413 to 51c0ddb Compare January 12, 2026 02:33
@alec-bike
Copy link
Contributor Author

@mattijn the builds are failing with my latest commit, likely due to changes I've made in the schemapi code and generated schema python files (the builds fail on "Test that schema generation has no effect"). The tests pass on my system -- are there additional steps I need to take to pass the pull-request build process when the generated files change?

@mattijn
Copy link
Contributor

mattijn commented Jan 12, 2026

Great that you are making progress! No problem! I might have a clue how to approach this. It's in the contributing docs:

A large part of Altair's code base is automatically generated. After you have made your manual changes, make sure to run the following [locally] to see if there are any changes to the automatically generated files:

uv run task generate-schema-wrapper

Once you have done that you also shall commit these changed files.

The Github CI is doing this too, and errors when there are changes after generation.

@alec-bike
Copy link
Contributor Author

alec-bike commented Jan 12, 2026

I've run generate_schema_wrapper and checked in mixins.py (no other files are modified) but the CI still fails immediately. I also tried running update-init-file but this did not modify __init__.py.

Interestingly, the CI error message indicates ruff has a problem with the __init__.py file. I was able to reproduce the error locally (macOS) if I use ruff version 0.14.11 (latest) instead of 0.12.3 (altair version).

Here is the CI output:

RUF067 `__init__` module should only contain docstrings and re-exports
  --> altair/vegalite/v6/schema/__init__.py:9:1
   |
 7 | from altair.vegalite.v6.schema.core import *
 8 |
 9 | SCHEMA_VERSION = 'v6.1.0'
   | ^^^^^^^^^^^^^^^^^^^^^^^^^
10 |
11 | SCHEMA_URL = 'https://vega.github.io/schema/vega-lite/v6.1.0.json'
   |

RUF067 `__init__` module should only contain docstrings and re-exports
  --> altair/vegalite/v6/schema/__init__.py:11:1
   |
 9 | SCHEMA_VERSION = 'v6.1.0'
10 |
11 | SCHEMA_URL = 'https://vega.github.io/schema/vega-lite/v6.1.0.json'
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
12 |
13 | __all__ = ['SCHEMA_URL', 'SCHEMA_VERSION', 'channels', 'core']
   |
...
subprocess.CalledProcessError: 
  Command '('ruff', 'check',
    PosixPath('/home/runner/work/altair/altair/altair/vegalite/v6/schema/__init__.py'), '--fix')' 
  returned non-zero exit status 1.
Error: Process completed with exit code 1.

I checked the "Install dependencies" CI step and indeed the latest ruff is installed:

Run uv pip install -e ".[dev, all]"
...
 + ruff==0.14.11

On my system version 0.12.3 is installed:

uv sync --all-extras
uv tree --outdated -d1
...
── ruff v0.12.3 (extra: dev) (latest: v0.14.11)

@alec-bike
Copy link
Contributor Author

alec-bike commented Jan 13, 2026

I believe the regression is from ruff 0.14.10 to 0.14.11. My previous PR built fine with ruff 0.14.10 but this one fails with ruff 0.14.11.

When I manually set these versions locally, I can reproduce this:

❯ uv sync --all-extras -P ruff==0.14.10
❯ uv run ruff check altair/vegalite/v6/schema/__init__.py
All checks passed!
❯ uv sync --all-extras -P ruff==0.14.11
❯ uv run ruff check altair/vegalite/v6/schema/__init__.py
RUF067 `__init__` module should only contain docstrings and re-exports
  --> altair/vegalite/v6/schema/__init__.py:9:1
   |
 7 | from altair.vegalite.v6.schema.core import *
 8 |
 9 | SCHEMA_VERSION = "v6.1.0"
   | ^^^^^^^^^^^^^^^^^^^^^^^^^
10 |
11 | SCHEMA_URL = "https://vega.github.io/schema/vega-lite/v6.1.0.json"
   |

RUF067 `__init__` module should only contain docstrings and re-exports
  --> altair/vegalite/v6/schema/__init__.py:11:1
   |
 9 | SCHEMA_VERSION = "v6.1.0"
10 |
11 | SCHEMA_URL = "https://vega.github.io/schema/vega-lite/v6.1.0.json"
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
12 |
13 | __all__ = [
   |

Found 2 errors.

It looks like the CI build grabs the latest ruff, while my local build pegs to 0.12.3.

Looking into ruff RUF067 I see this was just added in 0.14.11 under preview. Interestingly, altair runs with preview = true for ruff.

@alec-bike
Copy link
Contributor Author

alec-bike commented Jan 13, 2026

Note:

  • "lint" uses uv sync --all-extras which installs 0.12.3 ruff and passes.
  • "build" uses uv pip install -e ".[dev, all]" which installs latest ruff and fails.

Probably both CI steps should use uv sync for consistency?

@alec-bike
Copy link
Contributor Author

alec-bike commented Jan 13, 2026

The latest build works when I peg the max ruff version to 0.12.3 in pyproject.toml.

This fixes the build error, but does not address the dependency inconsistency between "build" and "lint" steps.

@alec-bike alec-bike marked this pull request as ready for review January 13, 2026 00:43
@alec-bike alec-bike force-pushed the api-type-fix branch 2 times, most recently from 86016a4 to f3fec33 Compare January 13, 2026 18:19
@alec-bike alec-bike changed the title fix: type issues with Chart mark_* methods. fix: type issues with Chart mark methods. Jan 14, 2026
@mattijn
Copy link
Contributor

mattijn commented Jan 14, 2026

Great research! Good idea to align lint and build process using uv sync👍.
If we set preview for ruff to False and this gives not many other errors than I prefer that over setting an upper limiting of the ruff version.

One question regarding current code diff, why is this annotate_var_kwds introduced? How is that related to types? Or is it something that enables a new feature?

@alec-bike
Copy link
Contributor Author

alec-bike commented Jan 14, 2026

The annotate_var_kwds1 is used to select only mixins for the type annotation (not channels or core). I added some comments to explain this.

Setting preview = false for ruff avoids the RUF067 issue and CI passes 🎉

Existing preview options do trigger a warning during uv run task generate_schema_wrapper, but no errors:

warning: Selection `PLC2801` has no effect because preview is not enabled.
warning: Selection `PLR6201` has no effect because preview is not enabled.
warning: Selection `PLW1514` has no effect because preview is not enabled.

These ruff options are commented out in pyproject.toml to remove the warnings.

Footnotes

  1. Renamed to annotate_kwds_flag in latest commit.

@alec-bike
Copy link
Contributor Author

CI has been stable since disabling ruff preview. PR is ready for review/merge.

@mattijn
Copy link
Contributor

mattijn commented Jan 18, 2026

Looks great! One last question:

Why is this change in msg on line 5315? Before this PR the msg was in line with line 5305 just above there, does that line now also require changing?

@mattijn mattijn enabled auto-merge (squash) January 18, 2026 16:58
@mattijn mattijn merged commit a2afad3 into vega:main Jan 18, 2026
15 checks passed
@mattijn
Copy link
Contributor

mattijn commented Jan 18, 2026

Good eyes👍 Merged and thanks again for working on this

@alec-bike alec-bike deleted the api-type-fix branch January 19, 2026 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Partially unknown type for some Chart methods.

2 participants