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.

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

chore: support git dependencies with pip #1559

Closed
wants to merge 25 commits into from

Conversation

saikonen
Copy link
Collaborator

@saikonen saikonen commented Sep 29, 2023

Support for git repo dependencies.

  • Add a proper platform restriction for remote deployment that compares tags from built wheels to the target platform. Error out in case of mismatch
  • fix caching for remote execution platforms

@saikonen saikonen requested a review from savingoyal September 29, 2023 11:20
@saikonen saikonen force-pushed the chore/support-git-dependencies-with-pip branch from 6c590d0 to 7c8a405 Compare October 2, 2023 14:27
@saikonen saikonen marked this pull request as ready for review October 3, 2023 09:44
Base automatically changed from pip-decorator to master October 4, 2023 14:38
@saikonen saikonen force-pushed the chore/support-git-dependencies-with-pip branch from 812d7bf to 758641d Compare October 4, 2023 18:29
@saikonen
Copy link
Collaborator Author

saikonen commented Oct 5, 2023

this seems to be running into issues with some repositories where a specific setuptools version is requested. the pip download part is failing due to not finding any matching version in the environment

f.ex. with git+https://github.com/pytest-dev/pytest.git it runs into

ERROR: Could not find a version that satisfies the requirement setuptools>=45.0 (from versions: none)
          ERROR: No matching distribution found for setuptools>=45.0

@saikonen
Copy link
Collaborator Author

saikonen commented Oct 5, 2023

fixed original issue by applying --no-build-isolation for the pip download command, but there still exists an edge case where in some cases pre-release versions seem to build an incorrect version 0.0.0 when downloaded with --no-index --no-build-isolation, but save correctly without either of the arguments.

As an example:

pip download --dest . --no-deps git+https://github.com/pytest-dev/pytest.git

saves as pytest-8.0.0.dev273+g54623f0f.zip where as

pip download --no-index --no-build-isolation --dest . --no-deps git+https://github.com/pytest-dev/pytest.git

saves as pytest-0.0.0.zip

@saikonen saikonen force-pushed the chore/support-git-dependencies-with-pip branch from b821e9f to 7991244 Compare October 6, 2023 11:37
@saikonen
Copy link
Collaborator Author

for context: setuptools-scm is a required dependency for correctly building VCS sources with pip, otherwise the default behavior with setuptools is to use a placeholder 0.0.0 version instead of erroring out due to missing tools

@saikonen saikonen force-pushed the chore/support-git-dependencies-with-pip branch from 4bd0a8b to d5bc7a8 Compare November 21, 2023 19:21
@saikonen saikonen marked this pull request as draft November 21, 2023 19:21
@romain-intel
Copy link
Contributor

So this builds when installing correct? In other words, it doesn't build during env resolution but during env hydration? Is that correct? If so, as I mentioned previously, I think this potentially breaks reproducibility. I may have missed the part though where it builds when resolving.

@saikonen
Copy link
Collaborator Author

saikonen commented Nov 22, 2023

So this builds when installing correct? In other words, it doesn't build during env resolution but during env hydration? Is that correct? If so, as I mentioned previously, I think this potentially breaks reproducibility. I may have missed the part though where it builds when resolving.

This shouldn't break reproducibility as for non-wheel sources, specifically VCS, as a wheel will try to be built before reaching create? I still need to add the platform mismatch check though.

The intended flow is as follows for pypi env resolution:

  • solve pip dry-run with report
  • download pip download to wheels, only for solved locations ending in .whl / build_wheels for solved locations not ending in .whl
  • after download/build_wheels we have a metadata mapping of url->path_to_whl which is used in
  • create pip install --no-compile from the local wheels

@saikonen saikonen marked this pull request as ready for review November 22, 2023 16:35
def _verify_wheel_tags_supported(wheel_name):
target_tags = pip_tags(python, platform)
built_tags = tags_from_wheel_name(wheel_name)
if built_tags not in target_tags:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is quite correct. You probably want a set intersection and if empty, then error out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great catch! Fixed this and added some unit tests for validation. Coverage is not extensive but should at least handle obvious cross-platform deployment breaks (e.g. osx-arm64 -> linux-64)

@saikonen
Copy link
Collaborator Author

superseded by #1681

@saikonen saikonen closed this Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support github pulls within Conda
2 participants