-
-
Notifications
You must be signed in to change notification settings - Fork 335
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
Update to conda 22.9 with tests for boa compatibility #361
Conversation
I think anaconda is just blocking us from downloading the package index:
about 7 million lines, not 3000. |
@wolfv are you able to help look into why when I try to install boa, it seems to be unable to find packages. For reference, this is a similar PR that simply removes to boa install tests. |
The package cache is the one with only the dependencies of the packages in the installer (in this case python, conda and mamba) so that the packages inside the installer can be installed. |
You need conda/constructor@85bca74 |
scripts/build.sh
Outdated
@@ -12,16 +12,25 @@ cd "${CONSTRUCT_ROOT}" | |||
# Constructor should be latest for non-native building | |||
# See https://github.com/conda/constructor | |||
echo "***** Install constructor *****" | |||
conda install -y "constructor>=3.3.1" jinja2 curl libarchive -c conda-forge --override-channels | |||
conda install --yes jinja2 curl libarchive --channel conda-forge --override-channels |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can install constructor here and override it with pip later on so that you don't have to manually install the dependencies of constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm personally always confused about how that plays out. There are also some new nsis dependencies.
In either case the errors seem to be unrelated to the removal of this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we carry that patch (the one line with used_repodata.pop("_mod", None)
) in constructor-feedstock?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, let me try to fork and apply that patch only, and see what happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess i'm ok with debugging constructor, but this isn't any worse than before.
Can we move forward with: #362
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point of releasing something that doesn't work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works just as well as the other things used to.
The problem is just as well hidden as it always has been.
@hmaarrfk is there still something that I could do to help here? |
If you can help us understand why the following index cache isn't being invalidated that would be great:
I'm not sure what "max-age" refers to. |
@wolfv My latest modification shows that in fact, this seems to be a peculiarity that is observed with mamba and not conda. It seems at least that mamba is holding on to the cache for too long, or conda ignores it entirely. I'm not too sure. |
Looks like mamba doesn't look at |
wow. excellent work! |
I updated my branch. lets see. |
scripts/build.sh
Outdated
fi | ||
# pip install git+git://github.com/conda/constructor@3.3.1#egg=constructor --force --no-deps | ||
|
||
constructor_rev=3.3.1_patch_for_cache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the tests pass, can you change this to a commit instead of a branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to patch conda forges version with the two patches.
We might have to trim down some of the test matrix. |
Thank you for your help Isuru! |
scripts/build.sh
Outdated
conda install --yes \ | ||
--channel conda-forge --override-channels \ | ||
jinja2 curl libarchive \ | ||
"constructor>=3.3.1=*_1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means version>=3.3.1
and build=*_1
. Is that what you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah. do i need ==
in the second one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, i mean "3.3.2" and "build=0" would be ok.
I'm not sure if syntax exists for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, there's no syntax for that. Let's keep it constructor>=3.3.1
and hope for the best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, i hope the smaller test matrix passes now for the ppc64le. it has been quite slow.
.github/workflows/ci.yml
Outdated
@@ -132,6 +132,8 @@ jobs: | |||
DOCKERIMAGE: condaforge/linux-anvil-aarch64 | |||
MINIFORGE_NAME: "Mambaforge-pypy3" | |||
OS_NAME: "Linux" | |||
# Reduce the test matrix because the builds timeouts on emulated architectures | |||
TEST_IMAGE_NAMES: "ubuntu:22.04 ubuntu:16.04 centos:7" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep only one. The one where the timeout happened barely ran two tests. https://pipelines.actions.githubusercontent.com/serviceHosts/e400fe88-afca-422a-abd9-23e00445dee4/_apis/pipelines/1/runs/6160/signedlogcontent/16?urlExpires=2022-10-23T19%3A25%3A43.9598492Z&urlSigningMethod=HMACV1&urlSignature=WWLz9fSOwtV%2F6x%2BVpSGa47kLLBUt7FOlRi7Z%2FOxGBaM%3D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
K. one has a modern ubuntu, the other has centos 7. I feel like that is a good spread for these.
Seems like the thorough check is just a little too much on these emulated architectures. |
Maybe with your fix i can remove the heavy handed skipping of the tests. |
Thanks again! |
Thank you both! 🙏 Are we comfortable publishing this release as final? Or what still needs to be done to get to that point? |
Honestly, i'm lost in the number of assets generated. So if somebody can confirm that 74 i the correct count, we can publish. |
Yes, 74 is correct. |
Noticed 2 releases showed up:
Which one should we be using? |
I guess 22.9.0. They both point to the same sha. I think the version was always meant to follow the conda version, and as such, should be 22.9.0 |
Sounds good. Updating the Docker images with PR ( conda-forge/docker-images#224 ) |
You can use either. |
@isuruf Out of curious why the version "jumps" from 4.x to 22.x on the conda side. (I thought I was a time traveler when I saw the version jumped like that) Appreciate your reading and I'm sorry for commenting on an old issue. |
Jus following conda |
Checklist
0
(if the version changed)conda-smithy
(Use the phrase@conda-forge-admin, please rerender
in a comment in this PR for automated rerendering)