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

Revert "memoryview: remove inheritance from Sequence" #12800

Merged
merged 2 commits into from
Oct 16, 2024

Conversation

JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented Oct 13, 2024

Reverts #12781

Although it does not implement the full Sequence API at runtime, memoryview is in fact a subclass of Sequence at runtime (through ABC registration). I will work soon on changes in CPython to remedy the discrepancy, but that doesn't solve the problem on existing versions of Python.

I'd prefer to revert to the previous behavior where memoryview is a subclass of Sequence, for a few reasons:

  • It's been like this for a long time and hasn't led to a lot of issues. Both options introduce potential problems; it's better to stick with the existing solution in case people are relying on it in their code already.
  • Removing the inheritance makes it so the behavior of isinstance(..., Sequence) becomes inconsistent between type checkers and the runtime. That's confusing and can lead to soundness issues.
  • I don't find the arguments in memoryview: remove inheritance from Sequence #12781 (comment) convincing. The missing methods are relatively rarely used, not core parts of the Sequence API. Sequence may not appear in the MRO of memoryview, but that's hos we handle ABC inheritance throughout typeshed. There wasn't fallout in mypy-primer, but most typechecked code isn't in mypy-primer. Inheritance from ABCs can lead to metaclass problems in type checkers, but removing the ABC from a single builtin class hardly fixes that problem.

This comment has been minimized.

@Avasam
Copy link
Collaborator

Avasam commented Oct 14, 2024

Is it possible to override the missing methods in a way that makes them unusable / almost always raise typing errors? Maybe something like a NoReturn + deprecated + Never args?

@JelleZijlstra
Copy link
Member Author

Done. I set index and count to None as @AlexWaygood suggested in the other thread. I didn't attempt to set __contains__ and __reversed__ to None because while those specific methods don't exist on memoryview, the operations they represent do work.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

I like it much more with the None overrides, and your point about this causing unsound type narrowing with isinstance() is a good one. I still think you're underrating the impact of IDEs and autocomplete tools suggesting nonexistent "public-API" methods in autocompletions (and failing to error when the user accesses them in their code). But fixing it in CPython is clearly the correct long-term fix (thank you for taking up that cause!).

TL;DR: still not sure it's holistically better overall to have it inherit from Sequence, but fine with it being reverted, especially with the None overrides for index/count

Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

hydra-zen (https://github.com/mit-ll-responsible-ai/hydra-zen)
- src/hydra_zen/structured_configs/_implementations.py:1137: error: No overload variant of "builds" of "BuildsFn" matches argument types "type[DataclassInstance]", "dict[str, int | float | memoryview[int] | Path | DataClass_ | <6 more items> | None]", "bool | None", "Literal['none', 'partial', 'all', 'object'] | None", "DataclassOptions"  [call-overload]
+ src/hydra_zen/structured_configs/_implementations.py:1137: error: No overload variant of "builds" of "BuildsFn" matches argument types "type[DataclassInstance]", "dict[str, int | float | Path | DataClass_ | type[DataClass_] | ListConfig | DictConfig | Enum | Sequence[HydraSupportedType] | Mapping[Any, HydraSupportedType] | None]", "bool | None", "Literal['none', 'partial', 'all', 'object'] | None", "DataclassOptions"  [call-overload]
- src/hydra_zen/structured_configs/_implementations.py:2666: error: Incompatible types in assignment (expression has type "tuple[int | float | memoryview[int] | Path | DataClass_ | <6 more items> | None, ...]", variable has type "list[Importable | Callable[P, R] | type[Builds[Importable]] | type[BuildsWithSig[Importable, Any]] | type[BuildsWithSig[type[R], P]] | Any]")  [assignment]
+ src/hydra_zen/structured_configs/_implementations.py:2666: error: Incompatible types in assignment (expression has type "tuple[int | float | Path | DataClass_ | type[DataClass_] | ListConfig | DictConfig | Enum | Sequence[HydraSupportedType] | Mapping[Any, HydraSupportedType] | None, ...]", variable has type "list[Importable | Callable[P, R] | type[Builds[Importable]] | type[BuildsWithSig[Importable, Any]] | type[BuildsWithSig[type[R], P]] | Any]")  [assignment]

@JelleZijlstra JelleZijlstra merged commit 2370b8b into main Oct 16, 2024
63 checks passed
@JelleZijlstra JelleZijlstra deleted the revert-12781-fix-memoryview branch October 16, 2024 14:28
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.

3 participants