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

Cache git dependencies as wheels #6896

Conversation

maksbotan
Copy link
Contributor

Currently, poetry install will clone, build and install every git dependency when it's not present in the environment. This is OK for developer's machines, but not OK for CI - there environment is always fresh, and installing git dependencies takes significant time on each CI run, especially if the dependency has C extensions that need to be built.

This commit builds a wheel for every git dependency that has precise reference hash in lock file and is not required to be in editable mode, stores that wheel in a cache dir and will install from it instead of cloning the repository again.

Pull Request Check List

Resolves:

  • Added tests for changed code.
  • Updated documentation for changed code.

I've checked locally that this code works, can you give me an advice on how test could look like?

Should this approach also be extended to handle 'tar.gz'-only distributions? See #3439 for rationale.

@maksbotan maksbotan force-pushed the maksbotan/cache-git-deps branch from b3477ca to 7f089ac Compare October 26, 2022 15:01
@neersighted
Copy link
Member

Overall the approach looks decent, I think it needs some refinement but this is a productive path. However, this is pretty low priority in the review backlog -- it will probably take some time to give detailed feedback.

@maksbotan
Copy link
Contributor Author

Thanks! In the meantime I'll try to extend the approach to tar.gz distributions and write a test.

Could you please set hacktoberfest-accepted label on this pr?

@dimbleby
Copy link
Contributor

I'd be much less enthusiastic about this, it brings complexity to poetry - and increases reliance on pip - for what seems like a niche use case.

If you're repeatedly using the same commit of a project, perhaps it would be better simply to build a wheel from that commit, and rely on that wheel.

@neersighted
Copy link
Member

neersighted commented Oct 29, 2022

Poetry is not participating in Hacktoberfest; participation is opt-in this year (labeled repositories only) and we have not chosen to opt in.

@dimbleby I agree it adds complexity and I think the implementation will need to change a decent amount to land this. However, I also think that this does not increase reliance on pip, as we were building those git clones like directory deps when they were non-Poetry projects anyway, and the way this PR is architected makes it easy to swap in build later when we start removing pip.

That being said, there's nothing that stops us from incrementally introducing build here -- would you be interested in attempting a version of this PR making use of build, @maksbotan?

@maksbotan
Copy link
Contributor Author

@neersighted re Hacktoberfest — got it, no problem.

As for the use case, I don't think it's niche. IMO it affects everyone who runs CI jobs for projects with git dependencies and/or tar.gz only distributions.

I'll try to update the PR following your suggestion about build. You mean the code that is used to install packages from directories? To change it to build wheels?

BTW, what is the point of removing pip?

@maksbotan maksbotan force-pushed the maksbotan/cache-git-deps branch from 7f089ac to a28e5e0 Compare November 6, 2022 19:11
@neersighted
Copy link
Member

neersighted commented Nov 6, 2022

Your PR has moved from using the code we use for directory deps to a standalone solution for building and caching wheels; I was explaining that to @dimbleby. I would suggest that you preserve the scope of this PR, and experiment with build as the build tool (via the Python API) instead of pip.

We are working to extract pip from Poetry for a variety of reasons. These include:

  • pip is an implementation detail and users try to configure it because they are mislead by thinking Poetry's architecture/design is similar to Pipenv
  • pip has a massive surface area that frequently causes bugs
  • It would be nice to not require pip to be present in every environment Poetry interacts with
  • pip is a major performance limitation
  • The ecosystem strongly benefits from multiple implementations of standards

You can see the WIP several maintainers are working on #6205.

@maksbotan
Copy link
Contributor Author

Thank you for the response.

I still don't quite get what you mean by build. Stuff like EditableBuilder, SdistBuilder, etc?

@neersighted
Copy link
Member

neersighted commented Nov 6, 2022

Thank you for the response.

I still don't quite get what you mean by build. Stuff like EditableBuilder, SdistBuilder, etc?

I mean the PyPI package, which has an API.

@maksbotan
Copy link
Contributor Author

Ah, thanks, now I see what you mean. I've never heard of that think before, will take a look.

@maksbotan maksbotan force-pushed the maksbotan/cache-git-deps branch from a28e5e0 to 0ee52b9 Compare December 16, 2022 12:05
@maksbotan maksbotan changed the base branch from master to feature/wheel-installer-and-builder December 16, 2022 12:05
@maksbotan
Copy link
Contributor Author

Hi @neersighted!

I finally had time for this PR again. I've reworked my changes on top of PR #6205 which you mentioned. There is a lot of work done there, so it was actually pretty easy to add caching of wheels for git dependencies.

I've set this PR's base branch to PR #6205, so that it can be incorporated into that feature once reviewed.

Please tell me what do you think about this.

@radoering radoering force-pushed the feature/wheel-installer-and-builder branch from 510c7df to 595af76 Compare December 18, 2022 13:41
@radoering radoering force-pushed the feature/wheel-installer-and-builder branch from 310e3c0 to 54c4ac3 Compare January 15, 2023 13:28
Currently, poetry install will clone, build and install every git
dependency when it's not present in the environment. This is OK for
developer's machines, but not OK for CI - there environment is always
fresh, and installing git dependencies takes significant time on each CI
run, especially if the dependency has C extensions that need to be
built.

This commit builds a wheel for every git dependency that has precise
reference hash in lock file and is not required to be in editable mode,
stores that wheel in a cache dir and will install from it instead of
cloning the repository again.
@maksbotan maksbotan force-pushed the maksbotan/cache-git-deps branch from 0ee52b9 to 3d72da1 Compare January 26, 2023 12:17
@maksbotan
Copy link
Contributor Author

@neersighted hi! Can I ping you on this one? This is an useful feature IMO, and PR code is now actually pretty concise.

@radoering radoering force-pushed the feature/wheel-installer-and-builder branch from bd35898 to 11efc64 Compare January 29, 2023 09:55
@radoering radoering force-pushed the feature/wheel-installer-and-builder branch 4 times, most recently from 82d0bde to 838f0ee Compare February 5, 2023 13:04
@radoering radoering deleted the branch python-poetry:feature/wheel-installer-and-builder February 5, 2023 13:15
@radoering radoering closed this Feb 5, 2023
@maksbotan maksbotan mentioned this pull request Feb 5, 2023
2 tasks
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants