-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Add M1 builds for PopPUNK #50011
Add M1 builds for PopPUNK #50011
Conversation
Maybe need to try rebuild tomorrow once these are definitely uploaded |
@bioconda/core I am again having issues with the linux build timing out here – any advice? |
@johnlees if related to build, anyway to reduce space used?
|
@rpetit3 not that I can think of! Don't think any of these dependencies can be dropped. It's coming up as about ~350Mb for the conda download which doesn't seem that big compared to a typical docker image. Biggest one I think is graph-tool-base at about 60Mb. Any way we can increase the space on the build images? |
This is the compressed size. The "no space left on device" happens at/after decompression.
Conda-forge recipes have free-disk-space but I am not aware of such property for Bioconda builds. |
I think this is likely to impact future releases. Would it be sensible to move to conda-forge? That would also require moving over dependencies rapidnj, treeswift and dendropy... or I could make those optional. |
Note to self, all of those tree packages would be completely fine as optional dependencies – should come back to this |
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request involves updates to the Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
recipes/poppunk/meta.yaml (1)
Line range hint
1-104
: Consider build optimization strategies.Given the build space issues mentioned in the PR comments:
- Consider using
build/pre-link
andbuild/post-link
scripts to defer optional dependency installation- If migration to conda-forge is planned, consider:
- Making tree-related packages (rapidnj, treeswift, dendropy) optional
- Breaking down the package into smaller components
This could help address the build space constraints while maintaining functionality.
Would you like assistance in implementing any of these suggestions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/poppunk/meta.yaml
(4 hunks)
🧰 Additional context used
🪛 yamllint
recipes/poppunk/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (4)
recipes/poppunk/meta.yaml (4)
Line range hint 1-10
: LGTM: Version and checksum updates are properly handled.
The version bump and corresponding SHA256 update follow the standard package update process.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
15-15
: Verify if removing --no-build-isolation is intentional.
The --no-build-isolation
flag prevents pip from creating an isolated build environment. This could lead to build inconsistencies if the package has complex build dependencies.
Would you like me to help investigate if any of the dependencies require build isolation?
33-34
: LGTM: Platform-specific OpenMP support properly configured.
The OpenMP configuration looks correct:
llvm-openmp
for macOS (including ARM64/M1)libgomp
for all Linux architectures
91-104
: LGTM: Enhanced metadata and ARM64 support.
Good improvements:
- Added proper documentation and development URLs
- Included license file reference
- Explicitly declared
osx-arm64
support
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/poppunk/meta.yaml
(5 hunks)
🧰 Additional context used
🪛 yamllint
recipes/poppunk/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (2)
recipes/poppunk/meta.yaml (2)
105-106
: Verify all requirements for osx-arm64 platform.
The addition of osx-arm64
to additional-platforms is aligned with the PR objectives. However, based on the PR comments about space issues during builds, we should verify:
- All dependencies are available for ARM64
- Build resource requirements are within limits
#!/bin/bash
# Check build size requirements
echo "Checking compressed sizes of dependencies..."
conda search --json "graph-tool-base" "pp-sketchlib" "rapidnj" | jq '.[] | select(.name) | {name: .name, version: .version, size: .size}'
33-34
: Verify ARM64 compatibility of dependencies.
The OpenMP and Fortran compiler dependencies have been adjusted for different architectures, which is good. However, we should verify that all dependencies (especially C++ ones like graph-tool, pp-sketchlib) have ARM64 builds available.
Also applies to: 66-67
✅ Verification successful
I'm generating these scripts to:
- Examine the full requirements section to understand all dependencies
- Look for architecture-specific configurations
- Check build scripts for any architecture-specific handling
This will help us verify if the ARM64 support is properly configured in the recipe itself.
ARM64 support appears properly configured in the recipe
The recipe already includes necessary architecture-specific configurations:
- Appropriate OpenMP libraries for different platforms (
llvm-openmp
for macOS,libgomp
for Linux) osx-arm64
is explicitly listed in the supported platforms- Uses generic
{{ compiler('cxx') }}
which adapts to the target architecture - Core dependencies (Python, numpy, scipy, boost-cpp, etc.) are architecture-agnostic in their specification
The recipe follows conda-forge best practices for cross-platform support, and the architecture-specific dependencies are properly conditioned. No issues found with the ARM64 support configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if key dependencies have ARM64 builds
echo "Checking for ARM64 builds of critical dependencies..."
conda search --json "graph-tool[subdir=osx-arm64]" "pp-sketchlib[subdir=osx-arm64]" "rapidnj[subdir=osx-arm64]" | jq '.[] | select(.subdir=="osx-arm64")'
Length of output: 415
Script:
#!/bin/bash
# Let's check the recipe's dependencies and their architecture constraints
cat recipes/poppunk/meta.yaml | grep -A 20 "requirements:"
# Let's also check if there are any architecture-specific build configurations
cat recipes/poppunk/meta.yaml | grep -A 5 "\[osx\]"
cat recipes/poppunk/meta.yaml | grep -A 5 "\[linux\]"
cat recipes/poppunk/meta.yaml | grep -A 5 "arm64"
# Check for any build.sh or other build scripts that might have architecture-specific logic
fd "build\.(sh|bat)" recipes/poppunk/
cat recipes/poppunk/build.sh || true
Length of output: 1020
skip: true # [py < 38] | ||
script: "{{ PYTHON }} -m pip install . --no-deps --ignore-installed --no-cache-dir -vvv" | ||
skip: True # [py < 38] | ||
script: "{{ PYTHON }} -m pip install . --no-deps --no-build-isolation --no-cache-dir -vvv" |
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.
💡 Codebase verification
🛠️ Refactor suggestion
Based on the search results, I can see that --no-build-isolation
is widely used across numerous bioconda recipes (200+ packages). This is a strong indication that it's a standard practice in the bioconda ecosystem to ensure reproducible builds by preventing pip from downloading or building dependencies during installation.
Keep the --no-build-isolation
flag to maintain build reproducibility
The --no-build-isolation
flag is a crucial part of conda's build process, preventing pip from downloading or building dependencies during installation. Removing it could lead to inconsistent builds.
🔗 Analysis chain
Consider keeping the build isolation flag.
The removal of --no-build-isolation
might affect build reproducibility. This flag prevents pip from downloading or building dependencies during installation, ensuring consistent builds.
Consider restoring the flag:
- script: "{{ PYTHON }} -m pip install . --no-deps --no-build-isolation --no-cache-dir -vvv"
+ script: "{{ PYTHON }} -m pip install . --no-deps --no-build-isolation --no-cache-dir -vvv"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other bioconda recipes use build isolation
rg --type yaml "no-build-isolation" -A 2 -B 2
Length of output: 415
Script:
#!/bin/bash
# Let's check for build isolation patterns in conda-forge and bioconda recipes
fd -e yaml -e yml meta.yaml$ | xargs rg "pip install.*--no-build-isolation" -l
Length of output: 16423
Script:
#!/bin/bash
# Let's also check for any pip install commands in meta.yaml files to understand the common patterns
fd -e yaml -e yml meta.yaml$ | xargs rg "pip install\." -A 1 -B 1
Length of output: 69
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
recipes/poppunk/meta.yaml (1)
12-15
: Consider build space optimization.Based on the PR discussion, the build process is hitting space limitations. Consider these optimizations:
- Use
cleanup_pkgs: true
in conda_build_config.yaml- Remove intermediate build artifacts
- Consider making large dependencies like
graph-tool
optional for builds where space is constrainedThis will help prevent the "no space left on device" error mentioned in the PR comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/poppunk/meta.yaml
(4 hunks)
🧰 Additional context used
🪛 yamllint
recipes/poppunk/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (5)
recipes/poppunk/meta.yaml (5)
1-1
: LGTM: Version bump and checksum update.
The version update to 2.7.1 and corresponding SHA256 checksum update are properly implemented.
Also applies to: 10-10
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
92-93
: LGTM: Improved metadata documentation.
The following improvements enhance package metadata quality:
- Properly quoted URLs and license string
- Added license file reference
- Added development and documentation URLs
Also applies to: 95-95, 97-98
14-15
:
Restore the --no-build-isolation
flag.
The removal of --no-build-isolation
could affect build reproducibility. This flag is a crucial part of conda's build process, preventing pip from downloading or building dependencies during installation.
Apply this diff to restore the flag:
- script: "{{ PYTHON }} -m pip install . --no-deps --no-build-isolation --no-cache-dir -vvv"
+ script: "{{ PYTHON }} -m pip install . --no-deps --no-build-isolation --no-cache-dir -vvv"
103-105
: Verify complete ARM64 support implementation.
The addition of osx-arm64
to additional platforms is aligned with the PR objectives. However, we should verify that all dependencies are also available for ARM64.
#!/bin/bash
# Check if all dependencies have ARM64 support
# Extract dependencies
deps=$(rg "requirements:" -A 50 | rg "^\s+-\s+\w+" -o | cut -d- -f2)
# For each dependency, check if it's available for osx-arm64
echo "Checking ARM64 support for dependencies:"
for dep in $deps; do
echo "Checking $dep..."
conda search -c conda-forge -c bioconda --platform osx-arm64 "$dep"
done
34-35
: Verify ARM64 compiler requirements.
The platform-specific OpenMP implementations look correct:
llvm-openmp
for macOS (including M1/ARM64)libgomp
for Linux
However, we should verify if any additional compiler flags or settings are needed specifically for ARM64 builds.
@BiocondaBot please fetch artifacts |
Package(s) built are ready for inspection:
|
Have add arm64 for pp-sketchlib and mandrake, looks like rapidnj supported too
Please read the guidelines for Bioconda recipes before opening a pull request (PR).
General instructions
@BiocondaBot please add label
command.@bioconda/core
in a comment.Instructions for avoiding API, ABI, and CLI breakage issues
Conda is able to record and lock (a.k.a. pin) dependency versions used at build time of other recipes.
This way, one can avoid that expectations of a downstream recipe with regards to API, ABI, or CLI are violated by later changes in the recipe.
If not already present in the meta.yaml, make sure to specify
run_exports
(see here for the rationale and comprehensive explanation).Add a
run_exports
section like this:with
...
being one of:{{ pin_subpackage("myrecipe", max_pin="x") }}
{{ pin_subpackage("myrecipe", max_pin="x.x") }}
{{ pin_subpackage("myrecipe", max_pin="x.x") }}
(in such a case, please add a note that shortly mentions your evidence for that){{ pin_subpackage("myrecipe", max_pin="x.x.x") }}
(in such a case, please add a note that shortly mentions your evidence for that){{ pin_subpackage("myrecipe", max_pin=None) }}
while replacing
"myrecipe"
with eithername
if aname|lower
variable is defined in your recipe or with the lowercase name of the package in quotes.Bot commands for PR management
Please use the following BiocondaBot commands:
Everyone has access to the following BiocondaBot commands, which can be given in a comment:
@BiocondaBot please update
@BiocondaBot please add label
please review & merge
label.@BiocondaBot please fetch artifacts
You can use this to test packages locally.
Note that the
@BiocondaBot please merge
command is now depreciated. Please just squash and merge instead.Also, the bot watches for comments from non-members that include
@bioconda/<team>
and will automatically re-post them to notify the addressed<team>
.