Fix ValueError on enzyme validation #178
Conversation
WalkthroughThe changes made in the Changes
Assessment against linked issues
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 (
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
sdrf_pipelines/sdrf/sdrf_schema.py (1)
59-62: Add unit tests for the updated parsing logicConsider adding unit tests to ensure that the
ontology_term_parserfunction correctly handles 'CS' terms with '=' in the value, and continues to raiseValueErrorfor invalid formats in other keys.Would you like assistance in writing these unit tests or creating a new GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- sdrf_pipelines/sdrf/sdrf_schema.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
sdrf_pipelines/sdrf/sdrf_schema.py (1)
59-62: LGTM: Enhanced parsing logic for 'CS' termThe modifications to the
ontology_term_parserfunction correctly handle key-value pairs where the value contains '=' characters, specifically for the 'CS' key. This resolves theValueErrorencountered during enzyme validation and preserves existing behavior for other cases.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
recipe/meta.yaml (1)
Line range hint
1-53: Minor improvements and suggestions.
The indentation corrections for the run dependencies (duckdb, rdflib) are good. They improve readability and ensure correct parsing of the YAML file.
There's a commented out
license_fileentry with a note that a patch has been applied upstream. Consider adding a TODO comment or creating an issue to remind the team to uncomment this line in the next release that includes the upstream changes.Consider adding a TODO comment like this:
##license_file: LICENSE ## patch is applied upstream, next version will have it # TODO: Uncomment the license_file entry in the next release that includes the upstream changes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- recipe/meta.yaml (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
recipe/meta.yaml (1)
4-4: Version update looks good.The version bump from 0.0.29 to 0.0.30 is appropriate for the bug fix described in the PR objectives. This minor version increment correctly reflects the nature of the changes made.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
requirements-dev.txt (1)
Line range hint
1-6: Consider enhancing requirements file structure and specificity.While the current format is clear, consider the following improvements:
- Sort the packages alphabetically for easier readability and maintenance.
- Consider adding version specifications to ensure consistency across development environments.
Example:
black==<version> isort==<version> pre-commit==<version> pytest==<version> pytest-datadir==<version> setuptools==<version>requirements.txt (1)
Line range hint
3-8: Consider migrating frompandas_schematopandera.The comment in the file indicates that
pandas_schemamight not be maintained anymore and suggestspanderaas an alternative. However,pandas_schemais still listed as a dependency.To improve the project's long-term maintainability:
- Evaluate the feasibility of migrating from
pandas_schematopandera.- If migration is possible, create a separate issue or PR to handle this transition.
- If migration is not currently possible, consider adding a TODO comment with a link to an issue tracking this potential future work.
Would you like assistance in creating an issue to track the potential migration from
pandas_schematopandera?.github/workflows/ci.yml (1)
20-21: Approved: Good addition of pyproject.toml support for Black.The addition of
use_pyproject: trueis a positive change that allows Black to use the configuration from pyproject.toml. This aligns with best practices for Python project configuration and enables centralized management of Black settings.Consider adding a comment explaining why this parameter was added, to help future maintainers understand the reasoning behind this change. For example:
with: use_pyproject: true # Use Black configuration from pyproject.toml for consistent formatting
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .github/workflows/ci.yml (1 hunks)
- requirements-dev.txt (1 hunks)
- requirements.txt (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
requirements-dev.txt (2)
5-5: LGTM: Addition of pytest-datadir enhances testing capabilities.The inclusion of
pytest-datadiris a good addition to the development requirements. This pytest plugin will facilitate better management of test data directories and files, improving the overall testing infrastructure of the project.
6-6: LGTM: Addition of setuptools improves package management.The inclusion of
setuptoolsin the development requirements is a positive step. This tool will enhance the project's packaging and distribution capabilities, ensuring a more robust and consistent development environment across different setups.requirements.txt (1)
14-14: Verify the necessity of re-addingrdflib.The
rdflibpackage has been re-added to the requirements. While this library is commonly used for working with RDF data and could be relevant for ontology-related tasks, it's important to confirm its necessity in the context of this PR.Could you please clarify:
- Why was
rdflibpreviously removed?- How does re-adding
rdflibrelate to fixing the ValueError on enzyme validation?- Are there any specific features of
rdflibthat are now required for the ontology term parsing or validation?✅ Verification successful
Verified the necessity of re-adding
rdflib.The
rdflibpackage is actively used insdrf_pipelines/ols/ols.pyand is listed as a dependency insetup.py. This confirms that re-addingrdflibis essential for handling RDF data and ontology-related functionalities within the project.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for rdflib usage in the codebase # Test: Search for rdflib imports or usage echo "Searching for rdflib usage:" rg --type python 'import rdflib|from rdflib' # Test: Check if rdflib is used in ontology_term_parser or related files echo "Checking ontology_term_parser for rdflib usage:" rg --type python 'rdflib' $(fd -t f ontology_term_parser)Length of output: 336
Script:
#!/bin/bash # Description: Check for rdflib usage in the codebase with corrected file types # Test: Search for rdflib imports or usage echo "Searching for rdflib usage:" rg --type py 'import rdflib|from rdflib' # Test: Check if rdflib is used in ontology_term_parser or related files echo "Checking ontology_term_parser for rdflib usage:" rg --type py 'rdflib' $(fd -t f ontology_term_parser)Length of output: 614
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
.github/workflows/ci.yml (2)
25-28: Pin the Black version for consistencyWhile installing Black is correct, it's recommended to pin the version to ensure consistent behavior across different environments and over time.
Consider modifying the installation command:
run: | python -m pip install --upgrade pip - pip install black + pip install black==23.10.1 # Replace with the desired versionThis change will ensure that the same version of Black is used consistently, preventing potential issues caused by version differences.
32-32: LGTM! Consider adding more Black options for consistencyThe direct invocation of Black is correct and provides more control over the linting process. Great job on this change!
To ensure full consistency with your project's coding standards, consider adding more options to the Black command. For example:
- run: black . --check + run: black . --check --line-length 100 --target-version py38Adjust the
--line-lengthand--target-versionoptions according to your project's specific requirements. This will ensure that Black enforces consistent formatting across all contributors' environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .github/workflows/ci.yml (1 hunks)
- recipe/meta.yaml (2 hunks)
- setup.py (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- setup.py
🚧 Files skipped from review as they are similar to previous changes (1)
- recipe/meta.yaml
🧰 Additional context used
🪛 actionlint
.github/workflows/ci.yml
20-20: the runner of "actions/setup-python@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
18-32: Summary of CI workflow improvementsThe changes to the
PythonBlackjob in the CI workflow are beneficial:
- Setting up a specific Python version (3.12) ensures consistency in the linting environment.
- Directly installing and running Black provides more control over the linting process.
These updates will help maintain code quality and consistency across the project. The suggested improvements (updating action versions, pinning Black version, and adding Black options) will further enhance the robustness of the CI process.
🧰 Tools
🪛 actionlint
20-20: the runner of "actions/setup-python@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
| - name: Set up Python 3.12 | ||
| uses: actions/setup-python@v2 | ||
| with: | ||
| python-version: 3.12 # Set your desired Python version here |
There was a problem hiding this comment.
Update the actions/setup-python action version
The current version (v2) of the actions/setup-python action is outdated. To ensure compatibility and access to the latest features, consider updating to the latest stable version.
Apply this change:
- uses: actions/setup-python@v2
+ uses: actions/setup-python@v4This update will resolve the warning from the static analysis tool and ensure you're using the most recent stable version of the action.
📝 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.
| - name: Set up Python 3.12 | |
| uses: actions/setup-python@v2 | |
| with: | |
| python-version: 3.12 # Set your desired Python version here | |
| - name: Set up Python 3.12 | |
| uses: actions/setup-python@v4 | |
| with: | |
| python-version: 3.12 # Set your desired Python version here |
🧰 Tools
🪛 actionlint
20-20: the runner of "actions/setup-python@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
User description
Closes #176.
This exempts
CSfrom checking for extra=signs inontology_term_parser, preserving the old behavior in all other cases but avoiding the unwantedValueErrorwhen parsing cleave rule specification.PR Type
Bug fix
Description
ValueErrorin theontology_term_parserfunction by exempting theCSprefix from additional=checks.ValueErrorwhen a non-key-value pair is detected.Changes walkthrough 📝
sdrf_schema.py
Fix ValueError in ontology_term_parser for enzyme validationsdrf_pipelines/sdrf/sdrf_schema.py
ontology_term_parserto handleCSprefix without raisingValueError.CSfrom additional=checks.Summary by CodeRabbit
New Features
Bug Fixes
Chores
setuptoolsas a dependency.rdflibin the requirements.pytest-datadirto the development requirements.These updates aim to provide users with a more reliable experience when working with ontology terms.