Update odgi to 0.9.0 #51405
Update odgi to 0.9.0 #51405
Conversation
mencian
added
aarch64
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the In the Possibly related PRs
Suggested labels
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- recipes/odgi/build.sh (1 hunks)
- recipes/odgi/meta.yaml (1 hunks)
- recipes/odgi/patch (0 hunks)
💤 Files with no reviewable changes (1)
- recipes/odgi/patch
🧰 Additional context used
🔇 Additional comments (7)
recipes/odgi/meta.yaml (7)
2-2: Version update looks good.The package version has been correctly updated to 0.9.0, which aligns with the PR objectives.
13-14: Build section changes look good, but verify macOS compatibility.The build number has been correctly reset to 0 for the new version. The removal of the macOS skip condition suggests that odgi is now compatible with macOS.
Please confirm that odgi v0.9.0 is indeed compatible with macOS and that it has been tested on this platform.
24-25: Requirements changes look good, but verify jemalloc runtime dependency.The changes to the requirements section are appropriate:
- Moving
llvm-openmpandlibgompto thehostsection is correct as they are needed during the build process.- Adding
jemallocto therunsection ensures its availability at runtime.Please confirm that
jemallocis indeed required as a runtime dependency for odgi v0.9.0. If it's only needed during the build process, consider removing it from therunsection.Also applies to: 31-31, 33-33
41-41: About section improvements look great.The changes to the about section enhance the package metadata:
- The
homeURL is now properly quoted.- The addition of the
license_familyfield provides clearer licensing information.- The summary now has proper punctuation.
- The new
dev_urlanddoc_urlfields provide valuable links for users and developers.These improvements will make it easier for users to find information about the package.
Also applies to: 43-43, 45-47
53-56: Extra section additions enhance package discoverability and suggest expanded compatibility.The changes to the extra section are beneficial:
- The new identifiers (DOI, biotools, and usegalaxy-eu) improve package discoverability and provide additional references.
- The additional platforms (linux-aarch64 and osx-arm64) suggest expanded compatibility.
Please confirm that odgi v0.9.0 has been tested and is compatible with the newly added platforms (linux-aarch64 and osx-arm64).
Also applies to: 58-59
1-59: Overall, the updates to meta.yaml look comprehensive and well-structured.The changes successfully update the odgi package to version 0.9.0, including appropriate modifications to the version, build number, checksum, dependencies, and metadata. The expanded platform support and additional identifiers enhance the package's accessibility and discoverability.
To ensure the accuracy and appropriateness of these changes, please address the following verification points:
- Confirm the SHA256 checksum for the v0.9.0 tarball.
- Verify odgi v0.9.0's compatibility with macOS.
- Confirm that
jemallocis required as a runtime dependency.- Verify compatibility with the newly added platforms (linux-aarch64 and osx-arm64).
Once these verifications are complete, the update should be ready for merging.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
10-10: Verify the SHA256 checksum.The SHA256 checksum has been updated, which is expected for a new version. However, it's crucial to ensure that this checksum is correct for the v0.9.0 tarball.
Run the following script to verify the SHA256 checksum:
| cp -rf lib/*cpython* "${PREFIX}/lib/python${PYVER}/site-packages" | ||
| cp -rf lib/* "${PREFIX}/lib" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use standard installation methods for Python packages
Copying files directly with cp -rf to the site-packages directory can overlook important installation steps and may lead to issues with package discovery or dependencies. It's recommended to use standard Python installation mechanisms to ensure proper setup.
Consider modifying the build process to install the Python modules using a setup script or ensure that CMake handles the installation correctly. For example:
- Modify
CMakeLists.txtto include proper installation commands for Python modules. - Use
setup.pyorpyproject.tomlif applicable.
|
|
||
| cmake -S . -B build -DCMAKE_BUILD_TYPE=Generic \ | ||
| -DCMAKE_INSTALL_PREFIX="${PREFIX}" -DCMAKE_CXX_COMPILER="${CXX}" \ | ||
| -DCMAKE_CXX_FLAGS="${CXXFLAGS}" -DEXTRA_FLAGS='-march=sandybridge -Ofast' \ |
There was a problem hiding this comment.
Reconsider the use of architecture-specific and aggressive optimization flags
The flag -march=sandybridge targets a specific CPU architecture, which might not be compatible with all users' systems. This could result in binaries that fail to run on older or different CPUs. For broader compatibility, consider using a more generic architecture flag like -march=core2 or omitting the -march flag altogether.
Additionally, the -Ofast optimization level enables aggressive optimizations that may disregard strict compliance with standards, potentially leading to unexpected behavior or instability. It might be safer to use -O2 or -O3 for optimization to balance performance and reliability.
Apply the following changes to address the concerns:
- -DCMAKE_CXX_FLAGS="${CXXFLAGS}" -DEXTRA_FLAGS='-march=sandybridge -Ofast' \
+ -DCMAKE_CXX_FLAGS="${CXXFLAGS}" -DEXTRA_FLAGS='-O3' \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -DCMAKE_CXX_FLAGS="${CXXFLAGS}" -DEXTRA_FLAGS='-march=sandybridge -Ofast' \ | |
| -DCMAKE_CXX_FLAGS="${CXXFLAGS}" -DEXTRA_FLAGS='-O3' \ |
| #python -c "import sys; sys.path.append('./lib'); import odgi_ffi" | ||
| #python -c "import sys; sys.path.append('./lib'); import odgi" | ||
| mkdir -p "${PREFIX}/lib/python${PYVER}/site-packages" | ||
| cp -rf lib/*cpython* "${PREFIX}/lib/python${PYVER}/site-packages" |
There was a problem hiding this comment.
Avoid hardcoding Python implementation-specific patterns
The use of *cpython* in the copy command assumes that the Python implementation is CPython. This may not be compatible with other Python implementations like PyPy.
Consider using a more general pattern or querying Python for the correct paths. For example:
-PYVER=`python -c 'import sys; print(str(sys.version_info[0])+"."+str(sys.version_info[1]))'`
+PYTHON_SITE_PACKAGES=`python -c 'import site; print(site.getsitepackages()[0])'`Then adjust the copy command accordingly:
-mkdir -p "${PREFIX}/lib/python${PYVER}/site-packages"
-cp -rf lib/*cpython* "${PREFIX}/lib/python${PYVER}/site-packages"
+mkdir -p "${PYTHON_SITE_PACKAGES}"
+cp -rf lib/* "${PYTHON_SITE_PACKAGES}"This approach ensures that the modules are placed in the correct directory regardless of the Python implementation.
Committable suggestion was skipped due to low confidence.
mencian
removed
aarch64
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
recipes/odgi/build.sh (1)
25-25: Consider a more selective library file copy processCopying all files from the
libdirectory to${PREFIX}/libensures that all necessary library files are available. However, this approach might have some drawbacks:
- It could copy unnecessary files, increasing the package size.
- There's a risk of overwriting existing files in the
${PREFIX}/libdirectory.Consider using a more selective copy process. For example:
# Copy only specific library files cp -rf lib/libodgi.* "${PREFIX}/lib" # Or use a list of required libraries for lib in libodgi libhandlegraph; do cp -rf lib/${lib}* "${PREFIX}/lib" doneThis approach would ensure that only the necessary library files are copied, reducing the risk of conflicts and keeping the package size optimal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/odgi/build.sh (1 hunks)
- recipes/odgi/meta.yaml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- recipes/odgi/meta.yaml
🧰 Additional context used
🔇 Additional comments (4)
recipes/odgi/build.sh (4)
5-8: LGTM: Improved environment setupThe new environment variable exports provide a more structured and clear way to set up the build environment. Using
$PREFIXensures that the package is built against the correct dependencies in the conda environment. The use of-O3optimization is a good choice for balancing performance and stability.
10-14: LGTM: Proper handling of macOS-specific configurationThe addition of a conditional check for macOS and setting specific CMAKE flags (
-DCMAKE_FIND_FRAMEWORK=NEVER -DCMAKE_FIND_APPBUNDLE=NEVER) is a good practice. This helps avoid potential issues with macOS frameworks and app bundles during the build process, ensuring better cross-platform compatibility.
16-20:⚠️ Potential issueImproved build process, but reconsider optimization flags
The updated cmake command and separate build step provide better control over the build process and can improve build speed with parallel jobs. However, there are concerns about the use of architecture-specific and aggressive optimization flags.
The use of
-march=sandybridgeand-Ofastflags may cause compatibility and stability issues, as noted in a previous review. Consider using more generic optimization flags for broader compatibility and stability:- -DCMAKE_CXX_FLAGS="${CXXFLAGS}" -DEXTRA_FLAGS='-march=sandybridge -Ofast' \ + -DCMAKE_CXX_FLAGS="${CXXFLAGS}" -DEXTRA_FLAGS='-O3' \This change will maintain good optimization while ensuring better compatibility across different systems.
22-24:⚠️ Potential issueImprove Python module installation process
The dynamic Python version detection is good for flexibility. However, the current method of copying Python modules has some potential issues:
- The use of
*cpython*in the copy command assumes CPython, which may not be compatible with other Python implementations.- Directly copying files to the site-packages directory may bypass important installation steps.
Consider using a more robust method for installing Python modules:
- Use
python -c 'import site; print(site.getsitepackages()[0])'to get the correct site-packages directory.- If possible, use a
setup.pyorpyproject.tomlfor proper Python package installation.Here's a potential improvement:
PYTHON_SITE_PACKAGES=$(python -c 'import site; print(site.getsitepackages()[0])') mkdir -p "${PYTHON_SITE_PACKAGES}" cp -rf lib/* "${PYTHON_SITE_PACKAGES}"This approach ensures compatibility with different Python implementations and proper module installation.
|
@mencian, you are a life saver! |
Update
odgi: 0.8.6 → 0.9.0recipes/odgi(click to view/edit other files)@pangenomeThis pull request was automatically generated (see docs).