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

Fix url package caching #6614

Closed

Conversation

tall-josh
Copy link

Pull Request Check List

Resolves: #2415

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

Hi, I've had a crack at caching packages that are downloaded from wheel urls. This is a work in progress I'm hoping to get feedback/guidance. The points below hopefully explain some of my reasoning.

  1. @dimbleby made some suggestions within the comments of issue Poetry downloading same wheels multiple times within a single invocation #2415 before I started this MR. I chose to keep the Chef as is because I figured changing the way it determined cache locations would break existing caches.
  2. I'm not sure how I would get access to a non-default cache dir. I use locations.py:DEFAULT_CACHE_DIR as a starting point, but if someone was to configure a different location the existing implementation would not pick that up.
  3. I've made some effort to update existing tests to pass given the new code, but I wanted to get feedback before sinking too much more time into this.

@neersighted neersighted marked this pull request as draft September 24, 2022 13:16
@dimbleby
Copy link
Contributor

To my mind the key step in addressing #2415 is to arrange that the already-existing cache is shared between all parts of the codebase that could use it.

Let me re-propose the steps I suggested over there:

First:

  • rearrange the Chef so that the cache directory that it uses is more predictable, I don't see why it should need to care about anything other than the URL that it is downloading from
  • submit an MR with that change only
    • that should provoke maintainer opinion on whether this is a sensible path
    • eg we can then have a discussion about whether your worry re existing caches is an important worry. (I think it's not)

Then: find some way to rearrange the code so that this cache can be shared by the Chef and also the places you have updated here.

It's not obvious to me what the best rearrangement is for the second part. Sketching out some ideas for that would maybe be useful prototyping, or just say in words what you're thinking and see if you can't attract some maintainer opinion on that too.

As it stands, I'm not sure that there's much in this MR that I would expect to survive to a final fix.

@tall-josh
Copy link
Author

Thanks @dimbleby

I agree this work was also me trying to wrap my head around the codebase and toying with some ideas. As you rightly pointed out I did deviate from your original points but for what it's worth I plan to work towards them moving forward.

After spending some time in the code base, finding some way to rearrange the code so that this cache can be shared by the Chef seems like a bit of a challenge, it's likely if/when I figure it out someone will have already got to it. I don't have a lot of time to work on this but I'll keep plugging away when I can.

Thanks for the feedback.

@metasyn
Copy link

metasyn commented Jan 12, 2023

👋 hey @tall-josh thanks for starting this work! I run into this issue everyday at work when using spacy models that come from a URL. curious to see if i could help with this effort at all, or if you were planning on continuing this work?

@tall-josh
Copy link
Author

Hi @metasyn I am likely not going to get the time to address this issue in the foreseeable future. Happy for someone else to take the reins.

@metasyn
Copy link

metasyn commented Feb 14, 2023

I see. Sounds good - in which case, maybe we should close this draft if its not intending to be worked on any longer?

@tall-josh
Copy link
Author

Sorry, all. I don't really have time to work on this

@tall-josh tall-josh closed this Mar 4, 2023
Copy link

github-actions bot commented Mar 3, 2024

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 Mar 3, 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.

Poetry downloading same wheels multiple times within a single invocation
3 participants