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.

Sign up for GitHub

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

Jump to bottom

tarfile.open(): Handle all modes #12181

Merged
merged 6 commits into from
Oct 3, 2024
Merged

tarfile.open(): Handle all modes #12181

merged 6 commits into from
Oct 3, 2024

Conversation

srittau
Copy link
Collaborator

@srittau srittau commented Jun 21, 2024

No description provided.

srittau

This comment was marked as off-topic.

Sign in to view

@srittau srittau changed the title tarfile.open(): Handle all non-pipe modes tarfile.open(): Handle all modes Jun 21, 2024
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@AlexWaygood
Copy link
Member

Looks like you're running into python/mypy#16367. It's been fixed on mypy master, but the fix hasn't yet been included in a mypy release.

@AlexWaygood
Copy link
Member

Looks like you're running into python/mypy#16367. It's been fixed on mypy master, but the fix hasn't yet been included in a mypy release.

Oh sorry, you already noted as such in #12182 (comment) :-)

@srittau
Copy link
Collaborator Author

srittau commented Jun 21, 2024

Looks like you're running into python/mypy#16367. It's been fixed on mypy master, but the fix hasn't yet been included in a mypy release.

Oh sorry, you already noted as such in #12182 (comment) :-)

I sneak edited it in when I noticed the problem. (Which had already been noted by @max-muoto in #12117.)

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
1 similar comment
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@srittau
Copy link
Collaborator Author

srittau commented Jun 21, 2024

vision is doing some dynamic construction of the mode string. This should be easily solvable by using literals and an intermediate variable. Which sounds like a good idea to ensure that no invalid mode strings are constructed.

pip is assigning the mode string using an if/elif cascade. This could also be solved by using an intermediate variable.

Both are not great, but I'm personally fine with these, given the increases soundness of the annotations. (Of course, in pip's case it would be even better if mypy would automatically infer that mode is a literal with certain values.)

@JelleZijlstra
Copy link
Member

This has a merge conflict.

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
JelleZijlstra
JelleZijlstra reviewed Jul 12, 2024
View reviewed changes
stdlib/tarfile.pyi
@@ -121,16 +119,124 @@ def open(
pax_headers: Mapping[str, str] | None = ...,
debug: int | None = ...,
errorlevel: int | None = ...,
compresslevel: int | None = ...,
Copy link
Member

Choose a reason for hiding this comment

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

I see this argument being used in the implementation, why did you remove it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Due to this comment from the docs:

For modes 'w:gz', 'x:gz', 'w|gz', 'w:bz2', 'x:bz2', 'w|bz2', tarfile.open() accepts the keyword argument compresslevel (default 9) to specify the compression level of the file.

@srittau srittau mentioned this pull request Aug 18, 2024
Closed
tsibley added a commit to nextstrain/cli that referenced this pull request Sep 5, 2024
@tsibley
runner.conda: Ignore newly failing type check for tarfile.open()
c79a8f3 
This started failing recently with newer typeshed stubs for
tarfile.open() (and/or newer stubs for requests.Response?).

mypy fails with:

    nextstrain/cli/runner/conda.py:220: error: Argument "fileobj" to "open" has incompatible type "Union[HTTPResponse, Any]"; expected "Optional[IO[bytes]]"

and pyright with:

    nextstrain/cli/runner/conda.py:220:14 - error: No overloads for "open" match the provided arguments (reportCallIssue)
    nextstrain/cli/runner/conda.py:220:58 - error: Argument of type "Literal['r|*']" cannot be assigned to parameter "mode" of type "_FileCreationModes" in function "open"
      Type "Literal['r|*']" is not assignable to type "_FileCreationModes"
        "Literal['r|*']" is not assignable to type "Literal['a']"
        "Literal['r|*']" is not assignable to type "Literal['w']"
        "Literal['r|*']" is not assignable to type "Literal['x']" (reportArgumentType)

I'm guessing the difference is because they match the different
overloads of tarfile.open().

I dug into it a little¹, but the stubs here seem a bit scattershot at
the moment so I stopped and went with an ignore.  An alternative fix is
casting response.raw to BinaryIO, but that seemed uglier and worse than
an ignore.

¹ See <python/typeshed#12117> (implicated)
  and <python/typeshed#12181> (open, may fix?)
  and <python/typeshed#12182> (open, may fix?)
tsibley added a commit to nextstrain/cli that referenced this pull request Sep 5, 2024
@tsibley
runner.conda: Ignore newly failing type check for tarfile.open()
73ed6cb 
This started failing recently with newer typeshed stubs for
tarfile.open() (and/or newer stubs for requests.Response?).

mypy fails with:

    nextstrain/cli/runner/conda.py:220: error: Argument "fileobj" to "open" has incompatible type "Union[HTTPResponse, Any]"; expected "Optional[IO[bytes]]"

and pyright with:

    nextstrain/cli/runner/conda.py:220:14 - error: No overloads for "open" match the provided arguments (reportCallIssue)
    nextstrain/cli/runner/conda.py:220:58 - error: Argument of type "Literal['r|*']" cannot be assigned to parameter "mode" of type "_FileCreationModes" in function "open"
      Type "Literal['r|*']" is not assignable to type "_FileCreationModes"
        "Literal['r|*']" is not assignable to type "Literal['a']"
        "Literal['r|*']" is not assignable to type "Literal['w']"
        "Literal['r|*']" is not assignable to type "Literal['x']" (reportArgumentType)

I'm guessing the difference is because they match the different
overloads of tarfile.open().

I dug into it a little¹, but the stubs here seem a bit scattershot at
the moment so I stopped and went with an ignore.  An alternative fix is
casting response.raw to BinaryIO, but that seemed uglier and worse than
an ignore.

¹ See <python/typeshed#12117> (implicated)
  and <python/typeshed#12181> (open, may fix?)
  and <python/typeshed#12182> (open, may fix?)
@tsibley tsibley mentioned this pull request Sep 5, 2024
Merged
1 task
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 1, 2024

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

vision (https://github.com/pytorch/vision)
+ torchvision/datasets/utils.py:219: error: No overload variant of "open" matches argument types "str | Path", "str"  [call-overload]
+ torchvision/datasets/utils.py:219: note: Possible overload variants:
+ torchvision/datasets/utils.py:219: note:     def open(name: str | bytes | PathLike[str] | PathLike[bytes] | None = ..., mode: Literal['r', 'r:*', 'r:', 'r:gz', 'r:bz2', 'r:xz'] = ..., fileobj: IO[bytes] | None = ..., bufsize: int = ..., *, format: int | None = ..., tarinfo: type[TarInfo] | None = ..., dereference: bool | None = ..., ignore_zeros: bool | None = ..., encoding: str | None = ..., errors: str = ..., pax_headers: Mapping[str, str] | None = ..., debug: int | None = ..., errorlevel: int | None = ...) -> TarFile
+ torchvision/datasets/utils.py:219: note:     def open(name: str | bytes | PathLike[str] | PathLike[bytes] | None, mode: Literal['x', 'x:', 'a', 'a:', 'w', 'w:'], fileobj: _Fileobj | None = ..., bufsize: int = ..., *, format: int | None = ..., tarinfo: type[TarInfo] | None = ..., dereference: bool | None = ..., ignore_zeros: bool | None = ..., encoding: str | None = ..., errors: str = ..., pax_headers: Mapping[str, str] | None = ..., debug: int | None = ..., errorlevel: int | None = ...) -> TarFile
+ torchvision/datasets/utils.py:219: note:     def open(name: str | bytes | PathLike[str] | PathLike[bytes] | None = ..., *, mode: Literal['x', 'x:', 'a', 'a:', 'w', 'w:'], fileobj: _Fileobj | None = ..., bufsize: int = ..., format: int | None = ..., tarinfo: type[TarInfo] | None = ..., dereference: bool | None = ..., ignore_zeros: bool | None = ..., encoding: str | None = ..., errors: str = ..., pax_headers: Mapping[str, str] | None = ..., debug: int | None = ..., errorlevel: int | None = ...) -> TarFile
+ torchvision/datasets/utils.py:219: note:     def open(name: str | bytes | PathLike[str] | PathLike[bytes] | None, mode: Literal['x:gz', 'x:bz2', 'w:gz', 'w:bz2'], fileobj: _Fileobj | None = ..., bufsize: int = ..., *, format: int | None = ..., tarinfo: type[TarInfo] | None = ..., dereference: bool | None = ..., ignore_zeros: bool | None = ..., encoding: str | None = ..., errors: str = ..., pax_headers: Mapping[str, str] | None = ..., debug: int | None = ..., errorlevel: int | None = ..., compresslevel: int = ...) -> TarFile
+ torchvision/datasets/utils.py:219: note:     def open(name: str | bytes | PathLike[str] | PathLike[bytes] | None = ..., *, mode: Literal['x:gz', 'x:bz2', 'w:gz', 'w:bz2'], fileobj: _Fileobj | None = ..., bufsize: int = ..., format: int | None = ..., tarinfo: type[TarInfo] | None = ..., dereference: bool | None = ..., ignore_zeros: bool | None = ..., encoding: str | None = ..., errors: str = ..., pax_headers: Mapping[str, str] | None = ..., debug: int | None = ..., errorlevel: int | None = ..., compresslevel: int = ...) -> TarFile
+ torchvision/datasets/utils.py:219: note:     def open(name: str | bytes | PathLike[str] | PathLike[bytes] | None, mode: Literal['x:xz', 'w:xz'], fileobj: _Fileobj | None = ..., bufsize: int = ..., *, format: int | None = ..., tarinfo: type[TarInfo] | None = ..., dereference: bool | None = ..., ignore_zeros: bool | None = ..., encoding: str | None = ..., errors: str = ..., pax_headers: Mapping[str, str] | None = ..., debug: int | None = ..., errorlevel: int | None = ..., preset: Literal[0, 1, 2, 3, 4, 5, 6, 7, 8, 9] | None = ...) -> TarFile
+ torchvision/datasets/utils.py:219: note:     def open(name: str | bytes | PathLike[str] | PathLike[bytes] | None = ..., *, mode: Literal['x:xz', 'w:xz'], fileobj: _Fileobj | None = ..., bufsize: int = ..., format: int | None = ..., tarinfo: type[TarInfo] | None = ..., dereference: bool | None = ..., ignore_zeros: bool | None = ..., encoding: str | None = ..., errors: str = ..., pax_headers: Mapping[str, str] | None = ..., debug: int | None = ..., errorlevel: int | None = ..., preset: Literal[0, 1, 2, 3, 4, 5, 6, 7, 8, 9] | None = ...) -> TarFile
+ torchvision/datasets/utils.py:219: note:     def open(name: str | bytes | PathLike[str] | PathLike[bytes] | None = ..., *, mode: str, fileobj: IO[bytes] | None = ..., bufsize: int = ..., format: int | None = ..., tarinfo: type[TarInfo] | None = ..., dereference: bool | None = ..., ignore_zeros: bool | None = ..., encoding: str | None = ..., errors: str = ..., pax_headers: Mapping[str, str] | None = ..., debug: int | None = ..., errorlevel: int | None = ..., preset: int | None = ...) -> TarFile

pip (https://github.com/pypa/pip)
+ src/pip/_internal/utils/unpacking.py:179: error: No overload variant of "open" matches argument types "str", "str", "str"  [call-overload]
+ src/pip/_internal/utils/unpacking.py:179: note: Possible overload variants:
+ src/pip/_internal/utils/unpacking.py:179: note:     def open(name: str | bytes | PathLike[str] | PathLike[bytes] | None = ..., mode: Literal['r', 'r:*', 'r:', 'r:gz', 'r:bz2', 'r:xz'] = ..., fileobj: IO[bytes] | None = ..., bufsize: int = ..., *, format: int | None = ..., tarinfo: type[TarInfo] | None = ..., dereference: bool | None = ..., ignore_zeros: bool | None = ..., encoding: str | None = ..., errors: str = ..., pax_headers: Mapping[str, str] | None = ..., debug: int | None = ..., errorlevel: int | None = ...) -> TarFile
+ src/pip/_internal/utils/unpacking.py:179: note:     def open(name: str | bytes | PathLike[str] | PathLike[bytes] | None, mode: Literal['x', 'x:', 'a', 'a:', 'w', 'w:'], fileobj: _Fileobj | None = ..., bufsize: int = ..., *, format: int | None = ..., tarinfo: type[TarInfo] | None = ..., dereference: bool | None = ..., ignore_zeros: bool | None = ..., encoding: str | None = ..., errors: str = ..., pax_headers: Mapping[str, str] | None = ..., debug: int | None = ..., errorlevel: int | None = ...) -> TarFile
+ src/pip/_internal/utils/unpacking.py:179: note:     def open(name: str | bytes | PathLike[str] | PathLike[bytes] | None = ..., *, mode: Literal['x', 'x:', 'a', 'a:', 'w', 'w:'], fileobj: _Fileobj | None = ..., bufsize: int = ..., format: int | None = ..., tarinfo: type[TarInfo] | None = ..., dereference: bool | None = ..., ignore_zeros: bool | None = ..., encoding: str | None = ..., errors: str = ..., pax_headers: Mapping[str, str] | None = ..., debug: int | None = ..., errorlevel: int | None = ...) -> TarFile
+ src/pip/_internal/utils/unpacking.py:179: note:     def open(name: str | bytes | PathLike[str] | PathLike[bytes] | None, mode: Literal['x:gz', 'x:bz2', 'w:gz', 'w:bz2'], fileobj: _Fileobj | None = ..., bufsize: int = ..., *, format: int | None = ..., tarinfo: type[TarInfo] | None = ..., dereference: bool | None = ..., ignore_zeros: bool | None = ..., encoding: str | None = ..., errors: str = ..., pax_headers: Mapping[str, str] | None = ..., debug: int | None = ..., errorlevel: int | None = ..., compresslevel: int = ...) -> TarFile
+ src/pip/_internal/utils/unpacking.py:179: note:     def open(name: str | bytes | PathLike[str] | PathLike[bytes] | None = ..., *, mode: Literal['x:gz', 'x:bz2', 'w:gz', 'w:bz2'], fileobj: _Fileobj | None = ..., bufsize: int = ..., format: int | None = ..., tarinfo: type[TarInfo] | None = ..., dereference: bool | None = ..., ignore_zeros: bool | None = ..., encoding: str | None = ..., errors: str = ..., pax_headers: Mapping[str, str] | None = ..., debug: int | None = ..., errorlevel: int | None = ..., compresslevel: int = ...) -> TarFile
+ src/pip/_internal/utils/unpacking.py:179: note:     def open(name: str | bytes | PathLike[str] | PathLike[bytes] | None, mode: Literal['x:xz', 'w:xz'], fileobj: _Fileobj | None = ..., bufsize: int = ..., *, format: int | None = ..., tarinfo: type[TarInfo] | None = ..., dereference: bool | None = ..., ignore_zeros: bool | None = ..., encoding: str | None = ..., errors: str = ..., pax_headers: Mapping[str, str] | None = ..., debug: int | None = ..., errorlevel: int | None = ..., preset: Literal[0, 1, 2, 3, 4, 5, 6, 7, 8, 9] | None = ...) -> TarFile
+ src/pip/_internal/utils/unpacking.py:179: note:     def open(name: str | bytes | PathLike[str] | PathLike[bytes] | None = ..., *, mode: Literal['x:xz', 'w:xz'], fileobj: _Fileobj | None = ..., bufsize: int = ..., format: int | None = ..., tarinfo: type[TarInfo] | None = ..., dereference: bool | None = ..., ignore_zeros: bool | None = ..., encoding: str | None = ..., errors: str = ..., pax_headers: Mapping[str, str] | None = ..., debug: int | None = ..., errorlevel: int | None = ..., preset: Literal[0, 1, 2, 3, 4, 5, 6, 7, 8, 9] | None = ...) -> TarFile
+ src/pip/_internal/utils/unpacking.py:179: note:     def open(name: str | bytes | PathLike[str] | PathLike[bytes] | None = ..., *, mode: str, fileobj: IO[bytes] | None = ..., bufsize: int = ..., format: int | None = ..., tarinfo: type[TarInfo] | None = ..., dereference: bool | None = ..., ignore_zeros: bool | None = ..., encoding: str | None = ..., errors: str = ..., pax_headers: Mapping[str, str] | None = ..., debug: int | None = ..., errorlevel: int | None = ..., preset: int | None = ...) -> TarFile

@JelleZijlstra JelleZijlstra merged commit 9757602 into python:main Oct 3, 2024
63 checks passed
@srittau srittau deleted the tarfile-open branch October 3, 2024 08:17
@cdce8p cdce8p mentioned this pull request Oct 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@srittau @AlexWaygood @JelleZijlstra