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.

Sign up for GitHub

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

Jump to bottom

feat: Make osx-arm64 version of conda-base #80

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
+11 −11
Open

feat: Make osx-arm64 version of conda-base #80

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e9b3013
Attempt to build for osx-arm64
corneliusroemer Jul 12, 2024
25ebcce
don't require gzip for now until https://github.com/conda-forge/gzip-…
corneliusroemer Jul 12, 2024
82fa64d
temporarily remove gzip for testing
corneliusroemer Jul 12, 2024
c5d09d5
switch on x for debugging
corneliusroemer Jul 12, 2024
0ff11b4
skip diff step on arm
corneliusroemer Jul 12, 2024
2704195
undo errant diff
corneliusroemer Jul 12, 2024
05ff023
try boa selector
corneliusroemer Jul 17, 2024
0b2f72d
gzip now available for osx-arm64
corneliusroemer Jul 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ jobs:
matrix:
os:
- ubuntu-22.04
- macos-12
- macos-12 # x86_64
- macos-14 # arm64
Comment on lines +45 to +46
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing in nextstrain/cli#379. We could also test more directly by addressing this:

conda-base/.github/workflows/ci.yaml

Line 143 in 2704195

# XXX TODO: Test on multiple platforms (os, maybe arch) via the matrix too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you know how please do!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Continuing in #83. It's looking quite complicated so let's not let it block this PR and use other ways to test instead.

Copy link
Member

@victorlin victorlin Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested as a one-off using 5dfbe2f (based on this PR branch) and it passed on all the pathogen repos currently listed in CI.


name: build and test (os=${{ matrix.os }})
runs-on: ${{ matrix.os }}
Expand Down Expand Up @@ -87,6 +88,10 @@ jobs:

- name: Generate summary
run: |
if [[ "$(uname -m)" == "arm64" ]]; then
echo "Skip on arm64 until the package is published" >&2
exit 0
fi
./devel/download-latest
./devel/diff-pkgs nextstrain-base-*.conda build/locked/*/nextstrain-base-*.conda \
> "$GITHUB_STEP_SUMMARY"
Expand Down
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ To build this package locally, run:

The final built package will be written to
`build/locked/<arch>/nextstrain-base-*.conda`, where `<arch>` is a Conda
subdir, e.g. `linux-64` or `osx-64`.
subdir, i.e. `linux-64`, `osx-64` or `osx-arm64`.

[CI][] builds store the entire `build/` and `locked/` directories as an
artifact attached to each CI run. You can download the artifacts to inspect
Expand Down
6 changes: 0 additions & 6 deletions devel/build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,6 @@ main() {
export PATH="$env/bin:$PATH"
export VERSION="${VERSION:-$(./devel/generate-version)}"

# Set CONDA_SUBDIR unless it's already set. This helps macOS users on
# arm64 chips use and produce osx-64 Conda packages instead of osx-arm64
# packages (which we don't yet support).
CONDA_SUBDIR="${CONDA_SUBDIR:-$(./devel/conda-subdir)}"
export CONDA_SUBDIR

clean
build src
lock
Expand Down
2 changes: 1 addition & 1 deletion devel/conda-subdir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ case "$kernel/$machine" in
echo "osx-64";;

Darwin/arm64)
echo "osx-64";;
echo "osx-arm64";;

*)
echo "unsupported kernel/machine: $kernel/$machine" >&2
Expand Down
11 changes: 8 additions & 3 deletions src/recipe.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ requirements:
- nextclade
- nextstrain-cli
- pathogen-embed
- pango_aliasor

#
# Third-party
Expand All @@ -69,16 +70,20 @@ requirements:
- epiweeks
- git
- google-cloud-storage
- gzip
# gzip is not yet available for arm64 on conda-forge
# https://github.com/conda-forge/gzip-feedstock/issues/8
# however gzip is already installed on stock macOS
# We don't seem to support selectors, so temporarily disable gzip globally
# - gzip # [not arm64]
corneliusroemer marked this conversation as resolved.
Show resolved Hide resolved
corneliusroemer marked this conversation as resolved.
Show resolved Hide resolved
- iqtree >=2
- jq
- ncbi-datasets-cli
- pango_aliasor
- perl
# Pin pulp <2.8 for snakemake: https://github.com/snakemake/snakemake/issues/2607
# Pin pulp <2.8 for snakemake: https://github.com/snakemake/snakemake/issues/2607
- pulp <2.8
- ruby
- seqkit
# TODO: We should avoid pinning too long, this pin was placed in 2023-12-21
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand where you're coming from here. Supporting Snakemake 8 will require coordination with existing pathogen repos (e.g. nextstrain/ncov#1114). I think it'd be better to surface as an issue in nextstrain/private, not a comment in this file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not both? It's not an XOR is it 😄

I feel we are too sticky with snakemake - all the code I maintain already works with v8, just some people don't seem to migrate. I remember last time it was quite an effort and I felt quite alone doing it as no one minds using outdated stuff.

And here again, you made that comment 7 months ago yet nothing happened since then 😛

Copy link
Member

@victorlin victorlin Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a misunderstanding here... I'm not against updating. Maybe I care a bit less because I don't use Snakemake in daily work, so I value your opinion as a more active user of it. My point is that simply adding a note as an unrelated TODO in this PR isn't going to get things moving.

I've created this issue which should be more visible. Feel free to edit: https://github.com/nextstrain/private/issues/120

- snakemake <8
- tsv-utils
- unzip
Expand Down