-
Notifications
You must be signed in to change notification settings - Fork 1
Resolve hanging build for ubuntu-22.04 and macos-13 (without Micromamba bump) #111
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
Conversation
`devel/build` was hanging and failing for ubuntu-22.04 and macos-13 in the CI workflow, but succeeded for macos-14. The downgrade of `awscli` in the successful macos-14 summary looked suspicious, so I decided to pin `awscli >=2` to see if it resolved the hanging issue and it did!¹ Still not sure _why_ this worked, but I'm guessing there was some weird dependency resolution during `boa build`. ¹ <#105 (comment)>
Output the `micromamba` version used in the CI workflow so we can debug any errors in the CI workflow with the same version locally.
Instead of always overriding the archspec in develd/build, only override for specific os in the CI workflow. Based on recommendation from @tsibley <#111 (comment)>
Instead of always overriding the archspec in develd/build, only override for specific os in the CI workflow. Based on recommendation from @tsibley <#111 (comment)>
0ee3f49 to
4e6a0c9
Compare
.github/workflows/ci.yaml
Outdated
| - run: ./devel/setup | ||
| - run: ./devel/build | ||
| env: | ||
| CONDA_OVERRIDE_ARCHSPEC: ${{ (matrix.os == 'ubuntu-22.04' || matrix.os == 'macos-13') && 'x86_64' || '' }} |
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.
Ah, so setting to the empty string is different than not setting it at all.
| CONDA_OVERRIDE_ARCHSPEC → prog ↓ | (not set) | x86_64 | (empty str) |
|---|---|---|---|
| conda (24.7.1) | __archspec=1=skylake | __archspec=1=x86_64 | (not present) |
| mamba (1.5.8) | __archspec=1=skylake | __archspec=1=x86_64 | (not present) |
| micromamba (2.0.02) | __archspec=1=x86_64_v3 | __archspec=1=x86_64 | __archspec=1=0 |
Putting aside Micromamba doing the wrong thing here, the empty string is used to suppress an __archspec virtual package.
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 wonder if it's easier to put
requirements:
build:
- sel(unix and x86_64): x86_64-microarch-level ==1in the recipe?
That would be (IIUC) the rough equivalent from the recipe authorship perspective and effectively stub out future work for variants.
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.
🤦♀️ hah okay, will try out the build requirements in the recipe.
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.
After adding to the build requirements in 7f53dcbb3897c3ee82794b555fb890d72c65eeb2, we are back to where we started with the CI failing during the test install of nextstrain-base...
I tried to set it as a run requirement in Codespaces, but that resulted in the same error.
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.
Oh bother. I'll dig into this and may #68 in the process while I'm at it. Instead of doing the conditional dep in the recipe, we can always make CONDA_OVERRIDE_ARCHSPEC set conditionally (e.g. inside the GitHub Actions workflow run: block instead of env: block).
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.
For posterity, the reason why adding:
requirements:
build:
- sel(unix and x86_64): x86_64-microarch-level ==1(or equivalents) to the recipe doesn't work as we want is that x86_64-microarch-level has a run_exports of _x86_64-microarch-level >=1. Note the leading underscore on the package name. This makes sense in the context of a standard package which evaluates that dep at installation time. But for our meta-package and full resolution/strict pinning of run dependencies, _x86_64-microarch-level >=1 resolves to _x86_64-microarch-level ==3 thanks to the build machine's __archspec. We can resolve this by either overriding the build machine's archspec (as we were doing in this PR before trying to modify the recipe instead) or special-casing our pinning of _x86_64-microarch-level (as the previous PR did, in a sense, by filtering it out entirely).
Lying about the build machine's archspec seems easier. We don't get any safety check that we're telling a useful/functional lie, but we just gotta keep our ducks in a row. Any future variant builds for other microarch levels can be done externally rather than using a variant config file.
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.
we can always make CONDA_OVERRIDE_ARCHSPEC set conditionally (e.g. inside the GitHub Actions workflow run: block instead of env: block).
Done in b809a9e
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.
Oh, sorry, I have a wip I'm about to push up (along with some other work in this repo, separately).
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.
(My "I'll dig into this and may #68 in the process while I'm at it." was a "I'll stop suggesting dead ends to you (sorry!) and pick up this PR myself.")
Setting `CONDA_OVERRIDE_ARCHSPEC` to an empty string does _not_ work how I expected it to...replace it with the build requirements in the recipe based on @tsibley's suggestion. <#111 (comment)>
Instead of always overriding the archspec in develd/build, only override for specific os in the CI workflow. Setting the override within `run` instead of `env` because setting it to an empty string does not work as expected. Based on recommendation from @tsibley <#111 (comment)> <#111 (comment)>
7f53dcb to
b809a9e
Compare
Avoids x86_64-microarch-level versions >1 which otherwise get resolved and pinned because the build machine (e.g. CI's GitHub Actions runner) has hardware which supports greater x86_64 microarch levels. Setting this in ./devel/build for the same reasons we set CONDA_SUBDIR there: it makes local development more similar to CI. Due to the nature of our meta-package, we need to adjust the virtual __archspec (i.e. what the machine is capable of) instead of using recipe build dependencies on x86_64-microarch-level. See discussion in links below. See-also: <#111> Resolves: <#105> Related-to: <#108> Co-authored-by: Jover Lee <joverlee521@gmail.com>
Avoids x86_64-microarch-level versions >1 which otherwise get resolved and pinned because the build machine (e.g. CI's GitHub Actions runner) has hardware which supports greater x86_64 microarch levels. Setting this in ./devel/build for the same reasons we set CONDA_SUBDIR there: it makes local development more similar to CI. Due to the nature of our meta-package, we need to adjust the virtual __archspec (i.e. what the machine is capable of) instead of using recipe build dependencies on x86_64-microarch-level. See discussion in links below. See-also: <#111> Resolves: <#105> Related-to: <#108> Co-authored-by: Jover Lee <joverlee521@gmail.com>
|
@joverlee521 Take a look at what I just pushed? And we'll see if CI passes. (For #68, I'll push two alternatives up as separate PRs.) |
|
Ugh, ancient macOS Bash doesn't support |
Avoids x86_64-microarch-level versions >1 which otherwise get resolved and pinned because the build machine (e.g. CI's GitHub Actions runner) has hardware which supports greater x86_64 microarch levels. Setting this in ./devel/build for the same reasons we set CONDA_SUBDIR there: it makes local development more similar to CI. Due to the nature of our meta-package, we need to adjust the virtual __archspec (i.e. what the machine is capable of) instead of using recipe build dependencies on x86_64-microarch-level. See discussion in links below. See-also: <#111> Resolves: <#105> Related-to: <#108> Co-authored-by: Jover Lee <joverlee521@gmail.com>
|
LGTM 👍 |
Description of proposed changes
This is an alternative to #108 that does not require bumping the version for Micromamba in the Nextstrain CLI.
awscli >=2_x86_64-microarch-level ==1withCONDA_OVERRIDE_ARCHSPEC=x86_64Related issue(s)
Resolves #105
Checklist