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

toil(pkg managers:pip): refactor and unify sdist & wheel handling #467

Merged
merged 5 commits into from
Mar 22, 2024
Merged

toil(pkg managers:pip): refactor and unify sdist & wheel handling #467

merged 5 commits into from
Mar 22, 2024

Conversation

ben-alkov
Copy link
Member

@ben-alkov ben-alkov commented Feb 15, 2024 •
edited
Loading

Refactor and unify pip sdist & wheel handling (STONEBLD-1705), to wit:

  • wheels are enumerated, saved, and downloaded all at once asynchronously; sdists are downloaded one by one
  • wheels are ignored if they already exist; sdists are re-downloaded
  • wheels respect the hashes reported by PyPI; sdists do not
  • sdists are recorded in the SBOM; wheels are not

Eliminate the differences as much as possible

Maintainers will complete the following section

  • Commit messages are descriptive enough
  • Code coverage from testing does not decrease and new code is covered
  • Docs updated (if applicable)
  • [n/a] Docs links in the code are still valid (if docs were updated)

Note: if the contribution is external (not from an organization member), the CI
pipeline will not run automatically. After verifying that the CI is safe to run:

@ben-alkov ben-alkov marked this pull request as draft February 15, 2024 19:59
@ben-alkov
Copy link
Member Author

ben-alkov commented Feb 15, 2024 •
edited
Loading

Note that tests haven't been touched yet (also the reason for the mypy fail).

IDK what CodeQL's issue is...

@ben-alkov ben-alkov changed the title toil(pip): remove dead code toil(pkg managers:pip): refactor and unify sdist & wheel handling Feb 15, 2024
@ben-alkov
Copy link
Member Author

ben-alkov commented Feb 16, 2024 •
edited
Loading

I've identified an issue where existing artifacts (i.e. things which have already been downloaded) are not being hashed (if required). I haven't resolved this yet.

Fixed

@ben-alkov ben-alkov force-pushed the feat-unify-sdist-wheel-handling branch 2 times, most recently from a09b015 to 4f92edb Compare February 19, 2024 21:34
@ben-alkov ben-alkov marked this pull request as ready for review February 20, 2024 20:55
@ben-alkov ben-alkov force-pushed the feat-unify-sdist-wheel-handling branch from 4f92edb to 6e9f74e Compare February 20, 2024 20:57
slimreaper35

This comment was marked as resolved.

Sign in to view

eskultety
eskultety reviewed Feb 21, 2024
View reviewed changes
Copy link
Member

@eskultety eskultety left a comment

Choose a reason for hiding this comment

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

@ben-alkov allegedly, I'm missing some deeper understanding of pip deps handling , but overall this looks good, some polishing is required. That said I do like the end result which significantly increased readability of _download_dependencies.

cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/pip.py Show resolved Hide resolved
cachi2/core/package_managers/pip.py Show resolved Hide resolved
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
brunoapimentel
brunoapimentel reviewed Feb 21, 2024
View reviewed changes
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
@ben-alkov ben-alkov force-pushed the feat-unify-sdist-wheel-handling branch 2 times, most recently from 9a8ba2d to 2de55b9 Compare February 23, 2024 15:57
@ben-alkov ben-alkov force-pushed the feat-unify-sdist-wheel-handling branch 2 times, most recently from 9107e64 to 82cc94c Compare March 1, 2024 18:13
slimreaper35

This comment was marked as resolved.

Sign in to view

@ben-alkov
Copy link
Member Author

you can fix one failing test by:
mock_distributions.side_effect = [[pypi_package1], [pypi_package2]]

I've got that locally, but then my comment in Slack becomes the problem:

test_download_from_requirement_files is failing in async_download_files() with 'cachi2.core.errors.FetchError: exception_name: InvalidURL, details: ""'
It was failing in '.pip.download_binary_file' (same error), so I changed the mock to '.general.async_download_files'... there isn't anything "lower level" than that to mock

@ben-alkov ben-alkov force-pushed the feat-unify-sdist-wheel-handling branch 2 times, most recently from 68d9cd0 to 65fef7e Compare March 7, 2024 22:44
@eskultety
Copy link
Member

eskultety commented Mar 8, 2024 •
edited
Loading

Uhm, what's wrong with that bit of code CodeQL complains about it? I don't see a problem, it's proper instantiation, can we dismiss the error as false positive?

slimreaper35
slimreaper35 reviewed Mar 8, 2024
View reviewed changes
cachi2/core/package_managers/pip.py Show resolved Hide resolved
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
eskultety
eskultety reviewed Mar 8, 2024
View reviewed changes
cachi2/core/package_managers/pip.py Show resolved Hide resolved
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/pip.py Show resolved Hide resolved
cachi2/core/package_managers/pip.py Show resolved Hide resolved
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/pip.py Show resolved Hide resolved
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
eskultety
eskultety reviewed Mar 11, 2024
View reviewed changes
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
tests/unit/package_managers/test_pip.py Outdated Show resolved Hide resolved
tests/unit/package_managers/test_pip.py Outdated Show resolved Hide resolved
@ben-alkov ben-alkov force-pushed the feat-unify-sdist-wheel-handling branch from 65fef7e to 17b15df Compare March 12, 2024 21:31
@ben-alkov ben-alkov force-pushed the feat-unify-sdist-wheel-handling branch from e9994d2 to 7438b23 Compare March 15, 2024 20:23
slimreaper35
slimreaper35 reviewed Mar 18, 2024
View reviewed changes
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/pip.py Show resolved Hide resolved
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/pip.py Show resolved Hide resolved
eskultety
eskultety reviewed Mar 19, 2024
View reviewed changes
cachi2/core/package_managers/pip.py Show resolved Hide resolved
cachi2/core/package_managers/pip.py Show resolved Hide resolved
cachi2/core/package_managers/pip.py Show resolved Hide resolved
cachi2/core/package_managers/pip.py Show resolved Hide resolved
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/pip.py Show resolved Hide resolved
@ben-alkov ben-alkov force-pushed the feat-unify-sdist-wheel-handling branch 4 times, most recently from b7db0f5 to 99b3f2e Compare March 20, 2024 17:11
eskultety
eskultety reviewed Mar 21, 2024
View reviewed changes
Copy link
Member

@eskultety eskultety left a comment

Choose a reason for hiding this comment

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

If there's anything else besides the last details related to bisect I don't see it so this is ready to be merged once you address those bits :).

cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/pip.py Show resolved Hide resolved
tests/unit/package_managers/test_pip.py Outdated Show resolved Hide resolved
@ben-alkov ben-alkov force-pushed the feat-unify-sdist-wheel-handling branch 2 times, most recently from 02d6c71 to df11af3 Compare March 21, 2024 14:01
eskultety
eskultety reviewed Mar 21, 2024
View reviewed changes
cachi2/core/package_managers/pip.py Show resolved Hide resolved
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/pip.py Outdated Show resolved Hide resolved
@ben-alkov ben-alkov force-pushed the feat-unify-sdist-wheel-handling branch 5 times, most recently from e1f9558 to b9b5b91 Compare March 21, 2024 20:43
@ben-alkov @eskultety
toil(package managers:pip): remove _get_user_checksums()...
9320a7f 
...based on the conversation from the original pip wheels PR
 containerbuildsystem#255

Signed-off-by: Ben Alkov <ben.alkov@redhat.com>
@ben-alkov @eskultety
toil(package managers:pip): cosmetics and types
e8a9c7b 
- docstrings: add "allow_binary"
- fix dodgy return type from `from_line()`
- don't shadow existing `parsed` in `from_line()` (we end up making it change types otherwise)
- other minor typing fixes
- add missing docstring for `_process_package_distributions()`
- wrap some long lines in solution strings
- fix a docstring

Signed-off-by: Ben Alkov <ben.alkov@redhat.com>
@ben-alkov @eskultety
toil(package managers:pip): move DistributionPackageInfo to the top…
dc925b2 
… of the module

Prevents type reference issues due to the existing symbol definition
order

Proposed by: Erik Skultety <eskultet@redhat.com>

Signed-off-by: Ben Alkov <ben.alkov@redhat.com>
@ben-alkov @eskultety
toil(package managers:pip): small refactoring in _resolve_pip()
a94c057 
Signed-off-by: Ben Alkov <ben.alkov@redhat.com>
@ben-alkov @eskultety
toil(package managers:pip): refactor...
882c9f2 
- `_download_dependencies()`
- `_process_package_distributions()`
- `DistributionPackageInfo`

Also adjust unit tests

Signed-off-by: Ben Alkov <ben.alkov@redhat.com>
@eskultety eskultety force-pushed the feat-unify-sdist-wheel-handling branch from b9b5b91 to 882c9f2 Compare March 22, 2024 10:39
@ben-alkov ben-alkov added this pull request to the merge queue Mar 22, 2024
Merged via the queue into containerbuildsystem:main with commit be86e44 Mar 22, 2024
15 checks passed
@ben-alkov ben-alkov deleted the feat-unify-sdist-wheel-handling branch March 22, 2024 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taylormadore taylormadore taylormadore left review comments

@brunoapimentel brunoapimentel brunoapimentel left review comments

@slimreaper35 slimreaper35 slimreaper35 approved these changes

@eskultety eskultety eskultety approved these changes

@ejegrova ejegrova Awaiting requested review from ejegrova

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

Successfully merging this pull request may close these issues.
5 participants
@ben-alkov @eskultety @brunoapimentel @taylormadore @slimreaper35