Use Literal for compression in zipfile - #9346
AlexWaygood merged 3 commits into
Conversation
Sorry, something went wrong.
This comment has been minimized.
AlexWaygood
left a comment
There was a problem hiding this comment.
Note: We could use Literal for the module constants, e.g. ZIP_STORED, ZIP_DEFLATED, etc. I can add another commit to this PR if required.
We need to either make both changes together, or not at all. Otherwise, type checkers will emit false positives on code like this, due to the fact that they will infer ZIP_DEFLATED as being an int, while the function requires a Literal:
import zipfile
zip_output = zipfile.ZipFile("some/path.zip", "w", zipfile.ZIP_DEFLATED, allowZip64=True)This is the cause of the pip error in the mypy_primer output: https://github.com/pypa/pip/blob/5f3f592c4581a059ff9de0fb8052ef5c6ef25fd4/src/pip/_internal/req/req_install.py#L714-L719
Note: I'm not yet sure I'm sold on the idea of making a change here at all, due to the risk of false-positive errors being emitted. But in order to see the impact of the change, we'll need to see what it looks like if we change the module-level constants to Literal values as well.
Sorry, something went wrong.
Yes I've been checking the reported mypy_primer errors and they all seem related to this. I'll add another commit and see what is the updated report. |
Sorry, something went wrong.
This comment has been minimized.
|
Related code failing: # vision (https://github.com/pytorch/vision)
_ZIP_COMPRESSION_MAP: Dict[str, int] = {
".bz2": zipfile.ZIP_BZIP2,
".xz": zipfile.ZIP_LZMA,
}
def _extract_zip(from_path: str, to_path: str, compression: Optional[str]) -> None:
with zipfile.ZipFile(
from_path, "r", compression=_ZIP_COMPRESSION_MAP[compression] if compression else zipfile.ZIP_STORED
) as zip:
zip.extractall(to_path)# jinja (https://github.com/pallets/jinja)
zip_file = ZipFile(
target, "w", dict(deflated=ZIP_DEFLATED, stored=ZIP_STORED)[zip]
)This would require a manual fix on these repos. So this is still breaking things, but imo it would result in safer code, as ZipFile will throw a NotImplementedError if the compression method is not supported. |
Sorry, something went wrong.
srittau
left a comment
There was a problem hiding this comment.
LGTM. I think we need to bite the bullet and need to make these kind of changes rather sooner than later.
Sorry, something went wrong.
AlexWaygood
left a comment
There was a problem hiding this comment.
Mostly LGTM too, though I'm not sure it makes as much sense for ZIP64_LIMIT, ZIP_FILECOUNT_LIMIT and ZIP_MAX_COMMENT to be made Literals as it does for the other constants. Those are very big numbers, and the constants serve a different purpose to ZIP_STORED, ZIP_DEFLATED, ZIP_BZIP2 and ZIP_LZMA.
Sorry, something went wrong.
|
I agree. Personally, I would have left them as int. But it doesn't hurt to annotate them, either. |
Sorry, something went wrong.
AlexWaygood
left a comment
There was a problem hiding this comment.
Thanks @Viicos!
Sorry, something went wrong.
|
The stubtest failures are due to the recent release of Python 3.10.9, and are unrelated to this PR. |
Sorry, something went wrong.
|
Diff from mypy_primer, showing the effect of this PR on open source code: vision (https://github.com/pytorch/vision)
+ torchvision/datasets/utils.py:301: error: Argument "compression" to "ZipFile" has incompatible type "int"; expected "Literal[0, 8, 12, 14]" [arg-type]
jinja (https://github.com/pallets/jinja)
+ src/jinja2/environment.py:864: error: Argument 3 to "ZipFile" has incompatible type "int"; expected "Literal[0, 8, 12, 14]" [arg-type]
|
Sorry, something went wrong.
This reverts commit 034cfab. The commit exposed the value of the compression constants, which seem to be implementation details, in the public API. I don't see anything in the documentation about the values of the constants such as `ZIP_STORED`: https://docs.python.org/3/library/zipfile.html?highlight=zipfile#zipfile.ZipFile Example where this makes a difference: ``` from typing import Literal import zipfile def f1(p: str, compression: int) -> None: """Error: compression should have the type Literal[0, 8, 12, 14]""" zipfile.ZipFile(p, compression=compression) def f2(p: str, compression: Literal[0, 8, 12, 14]) -> None: """Works, but cryptic and exposes internal implementation details""" zipfile.ZipFile(p, compression=compression) ``` The values are of constants need to be explicitly specified if somebody wants to wrap `zipfipe.ZipFile`, which arguably exposes implementation details in a problematic fashion. Here is a real-world example where this caused a regression: https://github.com/pytorch/vision/blob/main/torchvision/datasets/utils.py#L301
This reverts commit 034cfab. The commit exposed the value of the compression constants, which seem to be implementation details, in the public API. I don't see anything in the documentation about the values of the constants such as `ZIP_STORED`: https://docs.python.org/3/library/zipfile.html?highlight=zipfile#zipfile.ZipFile Example where this makes a difference: ``` from typing import Literal import zipfile def f1(p: str, compression: int) -> None: """Error: compression should have the type Literal[0, 8, 12, 14]""" zipfile.ZipFile(p, compression=compression) def f2(p: str, compression: Literal[0, 8, 12, 14]) -> None: """Works, but cryptic and exposes internal implementation details""" zipfile.ZipFile(p, compression=compression) ``` The values are of constants need to be explicitly specified if somebody wants to wrap `zipfipe.ZipFile`, which arguably exposes implementation details in a problematic fashion. Here is a real-world example where this caused a regression: https://github.com/pytorch/vision/blob/main/torchvision/datasets/utils.py#L301
According to docs:
Literal could be used for the compresslevel parameter as well:
But this would require extra overloads for each compression value, so considering the fact that there's already a split between 3.8 and 3.11 this would make things more complicated.
Note: We could use Literal for the module constants, e.g. ZIP_STORED, ZIP_DEFLATED, etc. I can add another commit to this PR if required.