Skip to content

add size to pylock.toml #13395

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/13395.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
add size attribute to pylock.toml
17 changes: 17 additions & 0 deletions src/pip/_internal/models/link.py
Original file line number Diff line number Diff line change
Expand Up @@ -508,6 +508,23 @@ def hash_name(self) -> str | None:
def show_url(self) -> str:
return posixpath.basename(self._url.split("#", 1)[0].split("?", 1)[0])

@property
def size(self) -> 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'm not convinced we should do additional network access to get the sizes.

I would rather envision an approach where we add a size attribute to InstallRequirement.download_info (as in a subclass of DirectUrl, or another class that would be more specialized to record the provenance URL, PEP710-style), and populate it at the same time as we currently populate download_info. This also means storing the original size in the cache (origin.json) next to the hashes.

It's more complex but like more efficient and robust.

That's just an intuition though, as I have not investigated deeply.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's a patch over an "experimental" feature. you need the "complex" and "efficient" version at this point ?

Copy link
Member

Choose a reason for hiding this comment

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

My understanding is the term experimental means here is that pip may change the CLI or semantics of the feature between releases. Not that the solution isn't expected to be robust.

I have the same concern here, particularly for the use case of locking a large existing environment, doing hundreds of extra network requests may cause the lock to be significantly slower or fail.

Copy link
Member

Choose a reason for hiding this comment

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

Another reason is that it would make sense for file sizes to follow the same flow as hashes, as they are closely related. I believe the could be obtained and verified at the same places in the codebase. It will likely make things easier to follow and understand.

Copy link
Contributor Author

@stonebig stonebig May 18, 2025

Choose a reason for hiding this comment

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

OK, I agree, but that is a bigger change, will need time. Competitive PR is welcomed

"""Fetch the size of the file from HTTP headers if available."""
from pip._internal.network.session import PipSession

try:
session = PipSession()
response = session.head(self.url, allow_redirects=True)
response.raise_for_status()
Copy link
Member

@uranusjr uranusjr May 17, 2025

Choose a reason for hiding this comment

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

This probably should be handled. Does ValueError cover this? It is not obvious.

# Check if the 'Content-Length' header is present
size = response.headers.get("Content-Length")
if size:
return int(size)
except ValueError as e:
logger.warning("Could not fetch size for %s: %s", self.url, e)
return None

@property
def is_file(self) -> bool:
return self.scheme == "file"
Expand Down
9 changes: 6 additions & 3 deletions src/pip/_internal/models/pylock.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class PackageDirectory:
class PackageArchive:
url: str | None
# (not supported) path: Optional[str]
# (not supported) size: Optional[int]
# (not supported) size: int | None
# (not supported) upload_time: Optional[datetime]
hashes: dict[str, str]
subdirectory: str | None
Expand All @@ -59,7 +59,7 @@ class PackageSdist:
# (not supported) upload_time: Optional[datetime]
url: str | None
# (not supported) path: Optional[str]
# (not supported) size: Optional[int]
size: int | None
hashes: dict[str, str]


Expand All @@ -69,7 +69,7 @@ class PackageWheel:
# (not supported) upload_time: Optional[datetime]
url: str | None
# (not supported) path: Optional[str]
# (not supported) size: Optional[int]
size: int | None
hashes: dict[str, str]


Expand Down Expand Up @@ -125,6 +125,7 @@ def from_install_requirement(cls, ireq: InstallRequirement, base_dir: Path) -> S
raise NotImplementedError()
package.archive = PackageArchive(
url=download_info.url,
# size=link.size,
hashes=download_info.info.hashes,
subdirectory=download_info.subdirectory,
)
Expand All @@ -142,13 +143,15 @@ def from_install_requirement(cls, ireq: InstallRequirement, base_dir: Path) -> S
PackageWheel(
name=link.filename,
url=download_info.url,
size=link.size,
hashes=download_info.info.hashes,
)
]
else:
package.sdist = PackageSdist(
name=link.filename,
url=download_info.url,
size=link.size,
hashes=download_info.info.hashes,
)
else:
Expand Down
17 changes: 17 additions & 0 deletions tests/functional/test_lock.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ def test_lock_wheel_from_findlinks(
"wheels": [
{
"name": "simplewheel-2.0-1-py2.py3-none-any.whl",
"size": 2156,
"url": path_to_url(
str(
shared_data.root
Expand Down Expand Up @@ -80,6 +81,7 @@ def test_lock_sdist_from_findlinks(
),
},
"name": "simple-2.0.tar.gz",
"size": 673,
"url": path_to_url(
str(shared_data.root / "packages" / "simple-2.0.tar.gz")
),
Expand Down Expand Up @@ -171,6 +173,7 @@ def test_lock_local_editable_with_dep(
/ "simplewheel-2.0-1-py2.py3-none-any.whl"
)
),
"size": 2156,
"hashes": {
"sha256": (
"71e1ca6b16ae3382a698c284013f6650"
Expand Down Expand Up @@ -234,3 +237,17 @@ def test_lock_archive(script: PipTestEnvironment, shared_data: TestData) -> None
},
},
]


def test_lock_includes_size(script: PipTestEnvironment, shared_data: TestData) -> None:
script.pip(
"lock",
"simplewheel==2.0",
"--no-index",
"--find-links",
str(shared_data.root / "packages/"),
expect_stderr=True, # for the experimental warning
)
pylock = tomllib.loads(script.scratch_path.joinpath("pylock.toml").read_text())
assert "size" in pylock["packages"][0]["wheels"][0]
assert pylock["packages"][0]["wheels"][0]["size"] > 0
8 changes: 8 additions & 0 deletions tests/unit/test_link.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
from __future__ import annotations

from unittest.mock import patch

import pytest

from pip._internal.models.link import Link, links_equivalent
Expand Down Expand Up @@ -188,6 +190,12 @@ def test_is_vcs(self, url: str, expected: bool) -> None:
link = Link(url)
assert link.is_vcs is expected

def test_link_size(self) -> None:
with patch("pip._internal.network.session.PipSession.head") as mock_head:
mock_head.return_value.headers = {"Content-Length": "12345"}
link = Link(url="https://example.com/package.tar.gz")
assert link.size == 12345


@pytest.mark.parametrize(
"url1, url2",
Expand Down