Allow caching of repodata from file:/// channels #9730
Conversation
|
We require contributors to sign our Contributor License Agreement, and we don't have one on file for @LorcanHamill. In order for us to review and merge your code, please e-sign the PDF at https://conda.io/en/latest/contributing.html#conda-contributor-license-agreement. We then need to manually verify your signature. We will ping the bot to refresh the PR status when we have confirmed your signature. |
|
Okay, this is a new PR that just adds a class method that is not (yet) called from anywhere. And yet lots of tests are broken. I would be very grateful if someone could explain what I've done wrong..? |
|
To be clear, this draft PR is just meant to be a starting point; I suspected that the tests might fail, and they did, but I can't see how my code change could have caused that. When the tests are passing I'll update the PR to have a full implementation of the new caching behaviour. |
|
I've been away from this for some time but hope to start work again on it shortly. I've just signed the CLA again; I think it may not have worked previously because my GitHub profile somehow didn't include my email address. Hopefully the CLA process will work this time... |
3733364 to
92b7fbb
Compare
|
We require contributors to sign our Contributor License Agreement, and we don't have one on file for @LorcanHamill. In order for us to review and merge your code, please e-sign the PDF at https://conda.io/en/latest/contributing.html#conda-contributor-license-agreement. We then need to manually verify your signature. We will ping the bot to refresh the PR status when we have confirmed your signature. |
|
Could someone please check why the "CLA-Signed" check is failing on this PR? I've signed the CLA at least three times, and the bot eventually marked a previous PR (#9677) with the "cla-signed" label on March 3. Thanks! |
92b7fbb to
21a1809
Compare
|
We require contributors to sign our Contributor License Agreement, and we don't have one on file for @LorcanHamill. In order for us to review and merge your code, please e-sign the PDF at https://conda.io/en/latest/contributing.html#conda-contributor-license-agreement. We then need to manually verify your signature. We will ping the bot to refresh the PR status when we have confirmed your signature. |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
This process is manually. I've added you to the signed list and summoned the bot. |
Ah, I see - I had assumed it was an automated process. Thanks, @jjhelmus ! |
e097c13 to
34c555c
Compare
|
As far as I can tell, this PR is ready. The AppVeyor build didn't fail - it was killed for exceeding the 1 hour time limit. The issue that this fixes - #9661 - is preventing our company from upgrading to conda 4.8, so I'd really like to see this merged soon. Please let me know if there's anything I can do to help that happen. (This is my first conda PR, so I'm not sure of the procedure.) Thanks! |
af854a4 to
10db6bd
Compare
|
I don't understand what's happened. All checks were passing on this PR, except that the AppVeyor build was timing out. I pushed a couple of empty commits to trigger re-runs of the checks, and now some tests are failing. Guess I may have rebased on top of other changes that broke those tests? Or is something else going on? If anyone could enlighten me, that would be great! |
|
@msarahan Could you please take a look at this? |
10db6bd to
718930b
Compare
|
Ok, this PR is passing all checks again, apart from AppVeyor which appears to be broken? Is there anything else that I need to do, to get this processed? If not, can it please be reviewed and merged? I appreciate that everyone is busy, but this is essentially a one-line code change, with the additional tweaks that are required to keep the tests passing. It is very important to our company; we can't possibly upgrade to conda 4.8 without this change or something similar; the performance in our environment is so bad as to be unusable. We're not the only ones affected, either - see #9661 and comments on #9677 from @carloswanguw It's almost three months since I submitted the first version of this change, and I'd really like to see it accepted soon. |
|
I am no longer employed by Anaconda and can't merge anything. I'm not sure about this pr. How does the cache for local repodata ever get invalidated? From what I can tell, it doesn't. That will cause immense problems to anyone who builds packages. |
Thanks for the response. I was unaware of the change in your situation, I hope that's a positive development for you. The cache is an in-memory one that only lives for the duration of the process and is now handled in the same way for file:// and https:// URLs. Does the metadata for file URLs get used during package building? And does it change during package building, after it has been read, and during the lifetime of a single conda process? (To be clear, those are not rhetorical questions, I don't know the answers.) In any case, I would have assumed that issues like that would be caught by one of the many unit and integration tests? |
Yes, unfortunately. Conda-build loads this the first time it creates the build or host environments, and then it needs to reload it once the new package has been built so that the test environment creation can use the new package. Conda-build could add a call to your new function here that clears things, but this change would break all past versions of conda-build. |
There are tests that rely on "file:///" channels NOT being cached between tests. There are also some that rely on "https:///" channels being cached. So the conservative approach is to ensure that what's cached between tests remains the same as before.
This is a test that changes the value of the "CONDA_USE_ONLY_TAR_BZ2" environment variable. That affects the metadata that is returned, so the cache must be cleared when it changes. This issue does not arise in production as the cache is in memory and the variable doesn't change once the conda process is up and running.
The first one confirms that if we create SubdirData object for a channel and then do that again, the second object is actually the same one as the first; it's been fetched from the in-memory cache. The second test confirms that if we do the same thing but clear the cache between the creation of the first and second objects then we get two distinct objects, but they're equivalent; a query on each of them yields the same result.
2ce514b to
e7e8f1b
Compare
|
This now passes all tests. For some reason AppVeyor marking the py27 run of the integration tests as a failure but it succeeded the next time it ran (which just an amended commit message). Anyway, this is once again ready for review. Can someone please review and approve this? |
We now timestamp each cached metadata object with the time it was placed in the cache. When we're about to use a cached object from a "file:///" channel, we first check if the JSON file from which it was loaded still exists, and get its modification time. If the JSON file no longer exists, or if the JSON file is newer than the cached object, then we ignore the cached object and create a new one (which will replace the old one in the cache).
e7e8f1b to
e27ad54
Compare
|
@mcg1969 @chenghlee @jjhelmus Could this be re-reviewed please? This is a major regression so would be nice to have it finally patched. |
|
This PR has been awaiting review for over a week now. Is there anything else I can do to try to progress this? I'm not sure how to request review other than by tagging people in a comment here, so I'll mention everyone who recently approved or merged PRs: @chenghlee @mcg1969 @angloyna @jjhelmus Apologies if that looks like I'm spamming people, but I don't know what else to do. There doesn't seem to be any other forum to discuss this sort of thing? (The "conda" mailing list, mentioned in some docs that I've seen, appears to be dead?) |
|
@LorcanHamill as you can see we have a fair amount of backlog here. I assure you we're going to get to it, and in fact we are in the process of making some internal adjustments so we can devote more resources to clearing this backlog. That's all I can assure you of now, except to say that I acknowledge this is great work and I'm appreciative. (And to be honest, even if it were merged, you'd then be waiting for a release to be tagged, too.) |
|
Thanks for the response, @mcg1969 . It's the not knowing that is most frustrating, as I'm sure you understand. I appreciate that it's not always possible to keep up with incoming PRs. As a matter of interest, is there a schedule somewhere for releases, with cut-off dates for merges and so on? It would help with our planning if we have some idea of when this would eventually make it into a release, if all goes well. |
|
I think that's a good suggestion @LorcanHamill ... as it turns out there are active discussions in the conda community around the larger questions of governance for this project. I must admit I'm not involved in those discussions so I can't tell you when or what but I'm genuinely positive changes are coming. |
|
I gather this PR has missed the cut for 4.8.4? Is there something I need to do now, to keep this PR alive? |
|
Closing & reopening this PR to force new CI checks to run; once those pass, I'll merge this PR. |
|
Great to see this merged - thanks, @chenghlee ! |
|
My company is using the latest conda 4.9.2 and experiencing the same problem with network drive based channels. Any idea what version of conda will have this fix? @LorcanHamill Really appreciate your work on this. |
|
This was included in conda 4.9.0, so it should be in the version you're
using.
https://github.com/conda/conda/blob/master/CHANGELOG.md#490-2020-10-19
You're welcome :)
…On Fri, Apr 23, 2021 at 6:24 PM haywardlam ***@***.***> wrote:
My company is using the latest conda 4.9.2 and experiencing the same
problem with network drive based channels. Any idea what version of conda
will have this fix?
@LorcanHamill <https://github.com/LorcanHamill> Really appreciate your
work on this.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9730 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEJNLBPEBHT6KLQC4DIKVKLTKGULLANCNFSM4LARLTPA>
.
|
|
@LorcanHamill I see it in the changelog 4.9.0. However, I am using 4.9.2 (latest Anaconda install) with our file:// network drive and conda is still extremely slow. Is there any configuration in .condarc required to use this caching feature? |
|
No, there is no special configuration required, it should just happen.
Maybe your cache folder itself is on an NFS drive?
…On Mon, May 3, 2021 at 8:24 PM haywardlam ***@***.***> wrote:
@LorcanHamill <https://github.com/LorcanHamill> I see it in the changelog
4.9.0. However, I am using 4.9.2 (latest Anaconda install) with our file://
network drive and conda is still extremely slow. Is there any configuration
in .condarc required to use this caching feature?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9730 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEJNLBLDJYXKQB2QPHLYNB3TL32AVANCNFSM4LARLTPA>
.
|
|
Hi there, thank you for your contribution! This pull request has been automatically locked because it has not had recent activity after being closed. Please open a new issue or pull request if needed. Thanks! |
(This is the second attempt at this, sorry for any inconvenience.)
As discussed in issue #9661 the performance of "file:///" channels is very poor in some circumstances with the latest conda versions. This seems to be because the solver requests repodata about directories more often than it used to. There is a caching mechanism for repodata but it is explicitly disabled for local channels; this PR removes that check so that the caching mechanism works for file:/// channels in the same way as HTTP channels.
That broke some unit tests, because of the way that an environment variable can be set to force conda to use only .tar.bz2 files, not .conda files. The tests try both settings, which changes the repodata that is fetched and should therefore be cached. The tests fail because the wrong version of the data appears in the cache. This could never happen in production as the cache is in memory. So it starts out empty when the conda command is invoked, and the command will only see one setting of the environment variable, so the cache will only ever have the appropriate data. To fix the tests, I added a method to clear the cache and invoked it before each test.
This change makes a huge difference to my company's use of conda. We use large conda channels on relatively slow NFS volumes, and without this change the creation of a new environment takes minutes when it previously took a few seconds.
Fixes #9661