Fix typing.get_type_hints call on a ModelHubMixin
#2729
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
hanouticelina
approved these changes
Jan 6, 2025
There was a problem hiding this comment.
thanks @aliberts for the fix! all good to me :)
failing tests are unrelated.
src/huggingface_hub/hub_mixin.py
Outdated
Comment on lines
28
to
36
| from _typeshed import DataclassInstance # type: ignore | ||
| else: | ||
|
|
||
| class DataclassInstance(Protocol): # type: ignore | ||
| __dataclass_fields__: ClassVar[Dict[str, Field]] | ||
|
|
||
|
|
||
| Dataclass = TypeVar("Dataclass", bound=DataclassInstance) | ||
| DataclassType = Type[Dataclass] |
There was a problem hiding this comment.
Suggested change
| from _typeshed import DataclassInstance # type: ignore | |
| else: | |
| class DataclassInstance(Protocol): # type: ignore | |
| __dataclass_fields__: ClassVar[Dict[str, Field]] | |
| Dataclass = TypeVar("Dataclass", bound=DataclassInstance) | |
| DataclassType = Type[Dataclass] | |
| # Type alias for dataclass instances | |
| class DataclassInstance(Protocol): | |
| __dataclass_fields__: ClassVar[dict[str, Field[Any]]] |
not a strong opinion on that and feel free to ignore, but maybe we can just define DataclassInstance directly here and remove the if/else and _typeshed dependency.
There was a problem hiding this comment.
Agreed, fixed: b1b9f5f
|
@hanouticelina not sure why but that definition doesn't work with python 3.8 as it's saying that dataclass.Field is not subscriptable. I've replaced |
hanouticelina
added a commit
that referenced
this pull request
Jan 6, 2025
* Add DataclassInstance for runtime type_checking * Add suggestion from code review * Fix type annotation * Add test * Actually fix type annotation (3.8) --------- Co-authored-by: Celina Hanouti <hanouticelina@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Proposed fix for #2727
typing.get_type_hintstries to resolve the type of an object at runtime. This means that theglobalandlocalnamespaces this function uses won't have access to whatever has been defined only for static type checkers (typically usingtyping.TYPE_CHECKING).In this case,
DataclassInstanceis only defined in such a way.This proposes to add a default fallback definition of
DataclassInstancefor runtime type checking.