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

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

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

Conversation

corneliusroemer
Copy link
Member

@corneliusroemer corneliusroemer commented Jul 12, 2024

Description of proposed changes

Build conda-base for osx-arm64 as well.

To make use of it within nextstrain cli, it seems we need to download micromamba arm64 rather than the x86_64 version - but the package can already be used via:

 micromamba create -n nextstrain-arm-env nextstrain/label/pull-80::nextstrain-base --platform osx-arm64

The platform arg is so that it should also work with an x86_64 micromamba installation.

Related issue(s)

Resolves nextstrain/private#77

Checklist

  • Checks pass

Checklist post merge

  • Readd gzip once it is available at conda-forge
  • Enable summary step for arm64 (currently disabled as there is no package to diff against yet)

@corneliusroemer corneliusroemer marked this pull request as ready for review July 12, 2024 15:04
Comment on lines +45 to +46
- macos-12 # x86_64
- macos-14 # arm64
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:

# 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

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.

- 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

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

Copy link
Member

@victorlin victorlin left a comment

Choose a reason for hiding this comment

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

Looks good pending cleanup of commits or squash+merge on GitHub

src/recipe.yaml Outdated Show resolved Hide resolved
src/recipe.yaml Outdated Show resolved Hide resolved
@corneliusroemer
Copy link
Member Author

corneliusroemer commented Jul 18, 2024

Will take a few minutes for gzip arm64 to become available, rerun later then it should work

Update: it works

@corneliusroemer
Copy link
Member Author

@victorlin

Looks good pending cleanup of commits or squash+merge on GitHub

I will squash :)

@corneliusroemer corneliusroemer changed the title Attempt to build for osx-arm64 feat: Make osx-arm64 version of conda-base Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants