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

add recipe for sizemeup #51033

Merged
merged 4 commits into from
Sep 29, 2024
Merged

add recipe for sizemeup #51033

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e2aebf7
add recipe for sizemeup
rpetit3 Sep 29, 2024
a40153a
fix issue on release side
rpetit3 Sep 29, 2024
7de952b
Update meta.yaml
rpetit3 Sep 29, 2024
024c8c7
reduce python constraint
rpetit3 Sep 29, 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
14 changes: 14 additions & 0 deletions recipes/sizemeup/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#!/bin/bash

mkdir -p $PREFIX/bin ${PREFIX}/share/sizemeup

# Install sizemup library
$PYTHON -m pip install --no-deps --ignore-installed -vv .

# move main executable, and replace with wrapper
chmod 755 bin/sizemeup-bioconda
mv $PREFIX/bin/sizemeup $PREFIX/bin/sizemeup-main
cp -f bin/sizemeup-bioconda $PREFIX/bin/sizemeup

# Copy genome size database (~100kb)
cp -f data/sizemeup-sizes.txt ${PREFIX}/share/sizemeup
Comment on lines +1 to +14
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding overall error handling and logging.

While the script is functional and follows Bioconda conventions, it could benefit from some overall improvements:

  1. Add a logging mechanism to track the progress of the installation.
  2. Implement overall error handling to catch and report any unexpected issues.
  3. Consider adding a cleanup function to handle interruptions gracefully.

Here's an example of how you could implement these improvements:

#!/bin/bash

set -e  # Exit immediately if a command exits with a non-zero status.

log() {
    echo "[$(date +'%Y-%m-%d %H:%M:%S')] $1"
}

cleanup() {
    log "Cleaning up..."
    # Add any necessary cleanup steps here
}

trap cleanup EXIT

log "Starting sizemeup installation"

mkdir -p $PREFIX/bin ${PREFIX}/share/sizemeup
log "Created necessary directories"

log "Installing sizemeup library"
$PYTHON -m pip install --no-deps --ignore-installed -vv . || { log "Failed to install sizemeup library"; exit 1; }

log "Setting up executable and wrapper"
chmod 755 bin/sizemeup-bioconda || { log "Failed to set permissions for sizemeup-bioconda"; exit 1; }
mv $PREFIX/bin/sizemeup $PREFIX/bin/sizemeup-main || { log "Failed to move sizemeup executable"; exit 1; }
cp -f bin/sizemeup-bioconda $PREFIX/bin/sizemeup || { log "Failed to copy sizemeup-bioconda wrapper"; exit 1; }

log "Copying genome size database"
if [ -f data/sizemeup-sizes.txt ]; then
    cp -f data/sizemeup-sizes.txt ${PREFIX}/share/sizemeup || { log "Failed to copy genome size database"; exit 1; }
else
    log "Error: Genome size database file not found"
    exit 1
fi

log "sizemeup installation completed successfully"

This version of the script includes logging, error handling, and a cleanup function to make the installation process more robust and easier to debug.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to pass on this recommendation as it makes the build.sh way more complex then it really needs to be

Copy link
Contributor

Choose a reason for hiding this comment

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

@rpetit3, understood, thank you for letting me know.

(_/)
( •_•)
💧

✏️ Learnings added 
Learnt from: rpetit3
PR: bioconda/bioconda-recipes#51033
File: recipes/sizemeup/build.sh:1-14
Timestamp: 2024-09-29T21:59:41.620Z
Learning: In build scripts like `build.sh`, prefer simplicity and avoid adding unnecessary complexity such as detailed logging and error handling beyond what's necessary.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

48 changes: 48 additions & 0 deletions recipes/sizemeup/meta.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
{% set name = "sizemeup" %}
{% set version = "1.0.0" %}

package:
name: {{ name|lower }}
version: {{ version }}

source:
url: https://github.com/rpetit3/sizemeup/archive/v{{ version }}.tar.gz
sha256: f8307bd4c9fb7b404046c87e18f1811daeefe9e8d431c0ea55e6adc004008338

build:
number: 0
noarch: generic
run_exports:
- {{ pin_subpackage(name, max_pin='x.x') }}

requirements:
host:
- python >=3.12
- poetry
- pip
run:
- python >=3.12
- requests
- rich-click >=1.6.0
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider adjusting Python version constraint and host requirements.

While the requirements are generally well-defined, there are two points to consider:

  1. The Python version constraint of >=3.12 is very recent and might limit the package's usability. Unless there's a specific need for Python 3.12 features, consider using a lower minimum version, such as >=3.7 or >=3.8, to ensure broader compatibility.

  2. The inclusion of poetry in the host requirements is unusual for a Bioconda recipe. Typically, poetry is used for development and packaging, not for installation in the conda environment. Consider removing it unless it's absolutely necessary for the build process.

Suggested changes:

requirements:
  host:
    - python >=3.7
    - pip
  run:
    - python >=3.7
    - requests
    - rich-click >=1.6.0

If there are specific reasons for the current choices, please provide more context.

Copy link
Member Author

Choose a reason for hiding this comment

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

This one should include poetry

Copy link
Contributor

Choose a reason for hiding this comment

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

@rpetit3, understood regarding including poetry in the host requirements. If sizemeup requires poetry for its build process, then including it is appropriate.

Regarding the Python version constraint of >=3.12, is that intentional? Since Python 3.12 is very recent, setting a lower minimum version like >=3.7 or >=3.8 might help reach a broader user base, unless sizemeup specifically requires features introduced in Python 3.12.

✏️ Learnings added 
Learnt from: rpetit3
PR: bioconda/bioconda-recipes#51033
File: recipes/sizemeup/meta.yaml:0-0
Timestamp: 2024-09-29T22:52:49.536Z
Learning: The `sizemeup` package requires `poetry` in the host requirements for its build process.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used 
Learnt from: rpetit3
PR: bioconda/bioconda-recipes#51033
File: recipes/sizemeup/meta.yaml:28-38
Timestamp: 2024-09-29T21:58:52.399Z
Learning: The `sizemeup-build` command exists in the `sizemeup` package and should be included in the test scripts where applicable.


test:
imports:
- sizemeup
commands:
- pip check
- sizemeup --version
- sizemeup --help
- sizemeup --species "Staphylococcus aureus"
- sizemeup-build --help
requires:
- pip
Comment on lines +28 to +38
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

💡 Codebase verification

Remove undefined sizemeup-build command from test scripts

The sizemeup-build command is referenced in the test commands but does not exist in the codebase or documentation. Please remove this command from the test section to avoid confusion and potential test failures.

  • File: recipes/sizemeup/meta.yaml
  • Lines: 28-38
🔗 Analysis chain

LGTM! Please clarify the sizemeup-build command.

The test section is well-defined and comprehensive, covering import checks, pip check, and various CLI functionalities.

Could you please provide more information about the sizemeup-build command? It's not mentioned in the package summary, and it would be helpful to understand its purpose and ensure it's documented in the package's README or documentation.

To verify the existence and purpose of this command, we can check the project's documentation:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for sizemeup-build documentation
rg -i 'sizemeup-build' README.md || echo "No mention of sizemeup-build in README.md"

Length of output: 125

Script:

#!/bin/bash
# Description: Search for the implementation of 'sizemeup-build' in the codebase
rg -i 'sizemeup-build' src/ || echo "sizemeup-build command not found in the src directory"

Length of output: 189

Copy link
Member Author

Choose a reason for hiding this comment

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

AI growing pains, sizemeup-build does actually exist

Copy link
Contributor

Choose a reason for hiding this comment

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

@rpetit3 Thank you for the clarification! I apologize for the oversight. I'll make sure to remember that sizemeup-build does indeed exist in the package.

(>_<) Sorry for the confusion.

✏️ Learnings added 
Learnt from: rpetit3
PR: bioconda/bioconda-recipes#51033
File: recipes/sizemeup/meta.yaml:28-38
Timestamp: 2024-09-29T21:58:52.201Z
Learning: The `sizemeup-build` command exists in the `sizemeup` package and should be included in the test scripts where applicable.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Comment on lines +28 to +38
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider adjusting the specific use case test.

The test section is comprehensive and well-structured. However, there's a potential issue to address:

The specific use case test on line 35:

- sizemeup --species "Staphylococcus aureus"

might fail due to the permission error mentioned in the PR comments. This error occurs when the tool attempts to write the output file.

Consider modifying this test to avoid the permission issue. You could:

  1. Add a temporary directory with write permissions for the test.
  2. Use a --dry-run option if available, to skip file writing.
  3. Redirect the output to /dev/null or a temporary file.

For example:

- sizemeup --species "Staphylococcus aureus" --output /tmp/sizemeup_test_output.txt

or

- sizemeup --species "Staphylococcus aureus" > /dev/null

Please choose the most appropriate option based on the tool's capabilities and the desired test coverage.

Copy link
Member Author

Choose a reason for hiding this comment

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

This does not affect the tests and was corrected in the source of sizemeup

Copy link
Contributor

Choose a reason for hiding this comment

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

@rpetit3, thank you for the clarification. It's great to hear that the permission issue has been corrected in the source of sizemeup and does not affect the tests.

(・_・)?

✏️ Learnings added 
Learnt from: rpetit3
PR: bioconda/bioconda-recipes#51033
File: recipes/sizemeup/meta.yaml:28-38
Timestamp: 2024-09-29T22:53:18.469Z
Learning: The permission issue in 'sizemeup --species "Staphylococcus aureus"' has been corrected in the source of sizemeup and does not affect the tests.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used 
Learnt from: rpetit3
PR: bioconda/bioconda-recipes#51033
File: recipes/sizemeup/meta.yaml:28-38
Timestamp: 2024-09-29T21:58:52.399Z
Learning: The `sizemeup-build` command exists in the `sizemeup` package and should be included in the test scripts where applicable.

If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!


about:
home: https://github.com/rpetit3/sizemeup
summary: A simple tool to determine the genome size of an organism
license: MIT
license_file: LICENSE

extra:
recipe-maintainers:
- rpetit3
Loading