fix(typing): Ignore `[arg-type]` error in `_deduplicate_enum_errors` #3475

dangotbanned · 2024-07-12T09:38:42Z

Fixes mypy error that appeared during a merge https://github.com/vega/altair/actions/runs/9905108163/job/27364368641

altair/utils/schemapi.py:423: error: Argument 1 to "join" of "str" has incompatible type "Any | Unset"; expected "Iterable[str]"  [arg-type]
Found 1 error in 1 file (checked 367 source files)
Error: Process completed with exit code 1.

Edit

This is ready to merge and is blocking for all other PRs

…rors` Fixes `mypy` error that appeared during a merge vega#3467 (comment)

mattijn · 2024-07-12T09:45:18Z

For my understanding, this is required because of a new version release of jsonschema (v4.23.0) or mypy (changelog)?

dangotbanned · 2024-07-12T09:51:12Z

For my understanding, this is required because of a new version release of jsonschema (v4.23.0) or mypy (changelog)?

@mattijn

I'm fairly certain it is jsonschema, but I don't understand why the un-annotated validator_value=_unset in https://github.com/python-jsonschema/jsonschema/pull/1019/files would trigger this

mattijn · 2024-07-12T09:54:20Z

@Julian, quick question: is this something to expect to introduce in packages that are dependent on jsonschema?

dangotbanned · 2024-07-12T09:57:09Z

@Julian, quick question: is this something to expect to introduce in packages that are dependent on jsonschema?

From a glance at the discussion, I think Unset appearing was something they were trying to avoid.

If mypy inferred this type to be Any, there wouldn't be a need to check it is not Unset

dangotbanned · 2024-07-12T11:31:09Z

Okay so I'm more confident this was caused by the stubs being updated (11 hours ago)

This does have the annotation:

validator_value: Any | Unset

Which mypy is referencing in:

error: Argument 1 to "join" of "str" has incompatible type "Any | Unset"; expected "Iterable[str]" [arg-type]

Whereas jsonschema updated 4 days ago and doesn't have the same annotations

@mattijn I think the solution to avoiding regressions like this in the future, would be getting altair added to https://github.com/hauntsaninja/mypy_primer
The PR that introduced this change would have flagged it here python/typeshed#12315 (comment)

Looking through the projects they currently test in https://github.com/hauntsaninja/mypy_primer/blob/57a892bb6903a06dc7f5042018c0659caf760898/mypy_primer/projects.py#L61 - there are many smaller projects already included.

The issue is not related to any runtime changes python-jsonschema/jsonschema#1019 (comment)

dangotbanned · 2024-07-12T15:38:53Z

@mattijn really appreciate you questioning this in #3475 (comment)

There were no runtime changes introduced by jsonschema, so I'm ignoring this "error" as it is simply a correctness vs library intention issue.

If the typeshed stubs are updated in the future, as a result of python-jsonschema/jsonschema#1019 (comment) - we'll get an [unused-ignore] warning and be able to revert back to the original code.

mattijn · 2024-07-13T12:30:36Z

I'm a bit concerned that this means that the latest released altair version will not pass mypy checks anymore when using the latest released version of jsonschema. Or I'm wrong in thinking this? Does this need to be raised at the typeshed repository? @binste, do you have some insights here, also on mypy_primer. I don't feel technically skilled in regards to type optimizations.

dangotbanned · 2024-07-13T15:44:19Z

I'm a bit concerned that this means that the latest released altair version will not pass mypy checks anymore when using the latest released version of jsonschema. Or I'm wrong in thinking this? Does this need to be raised at the typeshed repository? @binste, do you have some insights here, also on mypy_primer. I don't feel technically skilled in regards to type optimizations.

@mattijn apologies if I've misunderstood the question, but to break this down:

`jsonschema`

The new release of jsonschema adds type hints to _Error.
The only parameter that is relevant to this PR is validator_value.
- All that changed was adding the hint of Any.
Nothing was introduced in this release which would change the actual value returned by _Error.validator_value

python-jsonschema/jsonschema#1019 (comment)

From an end-user perspective as I tried to say in this PR, it's just plain incorrect -- the public type of that attribute is str, will always be str, and it's an implementation detail that in the course of assembling the object we still do this temporary _Unset nonsense which we'd never do if it were authored today. But no one downstream using the attribute should be seeing anything but str, if they did it would be a bug in this library.

v4.22.0

v4.23.0

class _Error(Exception):
    def __init__(
        self,
        message: str,
        validator=_unset,
        path=(),
        cause=None,
        context=(),
        validator_value=_unset, # <---------------------
        instance=_unset,
        schema=_unset,
        schema_path=(),
        parent=None,
        type_checker=_unset,
    ) -> None: ...

class _Error(Exception):
    def __init__(
        self,
        message: str,
        validator: str = _unset,  
        path: Iterable[str | int] = (),
        cause: Exception | None = None,
        context=(),
        validator_value: Any = _unset, # <----------------------
        instance: Any = _unset,
        schema: Mapping[str, Any] | bool = _unset, 
        schema_path: Iterable[str | int] = (),
        parent: _Error | None = None,
        type_checker: _types.TypeChecker = _unset, 
    ) -> None: ...

`typeshed`

The corresponding update to typeshed.stubs.jsonschema is not equivalent
To satisfy mypy as a result, you would need to check every parameter every time to confirm it is not Unset
This is not something that the new release of jsonschema wanted to achieve, and is mentioned multiple times throughout the PR
- 1, 2, 3

v4.22.0

v4.23.0

class _Error(Exception):
    def __init__(
        self,
        message: str,
        validator: _utils.Unset | None | protocols.Validator = ...,
        path: Sequence[str | int] = (),
        cause: Incomplete | None = None,
        context: Sequence[ValidationError] = (),
        validator_value=..., # <--------------------
        instance=...,
        schema=...,
        schema_path: Sequence[str | int] = (),
        parent: _Error | None = None,
        type_checker: _utils.Unset | TypeChecker = ...,
    ) -> None: ...

class _Error(Exception):
    def __init__(
        self,
        message: str,
        validator: str | Unset = sentinel,
        path: Iterable[str | int] = (),
        cause: Exception | None = None,
        context: Sequence[ValidationError] = (),
        validator_value: Any | Unset = sentinel, # <--------------------
        instance: Any | Unset = sentinel,
        schema: Mapping[str, Any] | bool | Unset = sentinel,
        schema_path: Iterable[str | int] = (),
        parent: _Error | None = None,
        type_checker: TypeChecker | Unset = sentinel,
    ) -> None: ...

I'm a bit concerned that this means that the latest released altair version will not pass mypy checks anymore

Specifically on this point, altair does pass mypy checks on this branch.
To the same extent that any usage of # type: ignore can be considered passing mypy.

Does this need to be raised at the typeshed repository?

I think so, but I'm unsure whether this would need to be raised by @Julian / @DanielNoord

binste

It's a joy to review PRs which are as well documented as this one! Thanks @dangotbanned for figuring out where the issue came from and explaining it, even with diffs :)

As Dan mentioned, mypy is happy if errors are ignored so that's fine + the error is due to a wrong type hint and not due to something that would cause issues at runtime -> Ignoring the error until type hints are fixed makes sense to me. Especially as Julian has mentioned that it's always str despite the different type hints.

I'm not familiar with mypy_primer but if it's easy to get Altair added, I think it would be great if PRs in mypy get tested against Altair code (if I understand it correctly).

dangotbanned · 2024-07-13T16:28:08Z

It's a joy to review PRs which are as well documented as this one! Thanks @dangotbanned for figuring out where the issue came from and explaining it, even with diffs :)

Thanks @binste

I wasn't happy with my earlier uncertainty of what caused the issue, but it was an interesting trail to follow

I'm not familiar with mypy_primer but if it's easy to get Altair added, I think it would be great if PRs in mypy get tested against Altair code (if I understand it correctly).

I'd seen it mentioned before, but it was specifically this python/typeshed#12315 (comment) which I thought was worth mentioning.

If I've understood correctly, this bot would've flagged the change in typeshed as casuing altair to fail - possibly leading to a different set of stubs being released.

This issue hauntsaninja/mypy_primer#42 seems to be tracking projects to add

fix(typing): Confirm arg type before joining in `_deduplicate_enum_er…

5124ace

…rors` Fixes `mypy` error that appeared during a merge vega#3467 (comment)

dangotbanned mentioned this pull request Jul 12, 2024

refactor: remove dead _get_channels_mapping code #3467

Merged

dangotbanned mentioned this pull request Jul 12, 2024

feat: make pandas and NumPy optional dependencies, don't require PyArrow for plotting with Polars/Modin/cuDF #3452

Merged

dangotbanned mentioned this pull request Jul 12, 2024

Add some typing to the exceptions.py module python-jsonschema/jsonschema#1019

Merged

dangotbanned added 2 commits July 12, 2024 16:27

revert: undo last commit

4d8b28a

revert: Ignore type error introduced by typeshed

74fb3d7

The issue is not related to any runtime changes python-jsonschema/jsonschema#1019 (comment)

dangotbanned changed the title ~~fix(typing): Confirm arg type before joining in _deduplicate_enum_errors~~ fix(typing): Ignore [arg-type] error in _deduplicate_enum_errors Jul 12, 2024

dangotbanned mentioned this pull request Jul 13, 2024

docs: fix CONTRIBUTING.md phrasing #3477

Merged

binste approved these changes Jul 13, 2024

View reviewed changes

binste merged commit 86d6b89 into vega:main Jul 13, 2024
13 checks passed

joelostblom added the bug label Jul 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(typing): Ignore `[arg-type]` error in `_deduplicate_enum_errors` #3475

fix(typing): Ignore `[arg-type]` error in `_deduplicate_enum_errors` #3475

dangotbanned commented Jul 12, 2024 •

edited

Loading

mattijn commented Jul 12, 2024

dangotbanned commented Jul 12, 2024 •

edited

Loading

mattijn commented Jul 12, 2024

dangotbanned commented Jul 12, 2024

dangotbanned commented Jul 12, 2024 •

edited

Loading

dangotbanned commented Jul 12, 2024 •

edited

Loading

mattijn commented Jul 13, 2024

dangotbanned commented Jul 13, 2024 •

edited

Loading

binste left a comment •

edited

Loading

dangotbanned commented Jul 13, 2024

fix(typing): Ignore [arg-type] error in _deduplicate_enum_errors #3475

fix(typing): Ignore [arg-type] error in _deduplicate_enum_errors #3475

Conversation

dangotbanned commented Jul 12, 2024 • edited Loading

Edit

mattijn commented Jul 12, 2024

dangotbanned commented Jul 12, 2024 • edited Loading

mattijn commented Jul 12, 2024

dangotbanned commented Jul 12, 2024

dangotbanned commented Jul 12, 2024 • edited Loading

dangotbanned commented Jul 12, 2024 • edited Loading

mattijn commented Jul 13, 2024

dangotbanned commented Jul 13, 2024 • edited Loading

jsonschema

typeshed

binste left a comment • edited Loading

Choose a reason for hiding this comment

dangotbanned commented Jul 13, 2024

fix(typing): Ignore `[arg-type]` error in `_deduplicate_enum_errors` #3475

fix(typing): Ignore `[arg-type]` error in `_deduplicate_enum_errors` #3475

dangotbanned commented Jul 12, 2024 •

edited

Loading

dangotbanned commented Jul 12, 2024 •

edited

Loading

dangotbanned commented Jul 12, 2024 •

edited

Loading

dangotbanned commented Jul 12, 2024 •

edited

Loading

dangotbanned commented Jul 13, 2024 •

edited

Loading

`jsonschema`

`typeshed`

binste left a comment •

edited

Loading