fix: Correctly handle non str index list for bwa-mem2/mem #7
Conversation
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
As reported in #522, currently this wrapper has issues if `idx` is a sequence is it incorrectly checked `input.index` to be as tring instead of `input.idx`. This is fixed now. While the discussin in #522 indicates, that there is an underlying issue with the concept of `input.idx` being as sequence of indices, this may need rework later. Until all the details are decided, having a correct version should be still valuable.
|
Clone of the PR snakemake/snakemake-wrappers#3101 |
![[development] Korbit - AI Mentor](https://avatars.githubusercontent.com/in/334508?s=40&u=8e7ea115878aacba56242d7ac87807ea98eac709&v=4){kind=avatar}
|
👋 I'm here to help you review your pull request. When you're ready for me to perform a review, you can comment anywhere on this pull request with this command: As a reminder, here are some helpful tips on how we can collaborate together:
|
|
My review is in progress 📖 - I will have feedback for you in a few minutes! |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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.
Review Summary by Korbit AI
Code Execution Comments
- Use a defensive approach to handle
idxinputs, including non-string, non-list, and empty cases, to prevent errors.
Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Current Korbit Configuration
General Settings
Setting Value Review Schedule Automatic excluding drafts Max Issue Count 10 Automatic PR Descriptions ✅ Issue Categories
Category Enabled Naming ✅ Database Operations ✅ Documentation ✅ Logging ✅ Error Handling ✅ Systems and Environment ✅ Objects and Data Structures ✅ Tests ❌ Readability and Maintainability ✅ Asynchronous Processing ✅ Design Patterns ✅ Third-Party Libraries ✅ Performance ✅ Security ✅ Functionality ✅ Feedback and Support
| if isinstance(idx, str): | ||
| index = path.splitext(idx)[0] | ||
| else: | ||
| index = path.splitext(snakemake.input.idx[0])[0] | ||
| index = path.splitext(idx[0])[0] |
There was a problem hiding this comment.
The code does not handle the case where 'idx' is neither a string nor a list. This can lead to unexpected errors and make the code harder to maintain. Consider adding an else clause to handle this case. This could involve raising an error with a descriptive message, which would make the code more robust and easier to maintain.
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
| idx = snakemake.input.get("idx", "") | ||
| if isinstance(idx, str): | ||
| index = path.splitext(idx)[0] | ||
| else: | ||
| index = path.splitext(snakemake.input.idx[0])[0] | ||
| index = path.splitext(idx[0])[0] |
There was a problem hiding this comment.
The current implementation assumes that if idx is not a string, it's a list with at least one element. However, this assumption might not always hold true. To improve robustness and ensure correct functionality, consider using a more defensive approach. For example:
idx = snakemake.input.get('idx', '')
if isinstance(idx, str):
index = path.splitext(idx)[0]
elif isinstance(idx, (list, tuple)) and len(idx) > 0:
index = path.splitext(idx[0])[0]
else:
raise ValueError(f'Invalid idx input: {idx}')This change will handle string and non-empty list/tuple inputs correctly, while also raising an informative error for other input types or empty sequences.
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
As reported in #522, currently the
bwa2-memwrapper has issues ifidxis a sequence is it incorrectly checkedinput.indexto be as string instead ofinput.idx.This bug was silent, as usually
snakemake.input.indexwould always be a available asfunctools.partial(<function Namedlist._used_attribute at 0x73ac4dbc0c20>, _name='index')or similar. Thus the predicate that it was not astrwas always true.Now with
snakemake=8.16.0this changed and the bug becomes an error running the already existing tests.While the discussion in #522 indicates, that there is an underlying issue with the concept of
input.idxbeing as sequence of indices, this may need rework later. Until all the details are decided, having a correct working version should be still valuable. With the current state any workflow using this wrapper usingsnakemake=8.16.0is broken.QC
snakemake-wrappers.While the contributions guidelines are more extensive, please particularly ensure that:
test.pywas updated to call any added or updated example rules in aSnakefileinput:andoutput:file paths in the rules can be chosen arbitrarilyinput:oroutput:)tempfile.gettempdir()points tometa.yamlcontains a link to the documentation of the respective tool or command underurl:Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Description by Korbit AI
What change is being made?
Correctly handle non-string index lists in the
bwa-mem2/memwrapper by updating the variable handling logic.Why are these changes being made?
The previous implementation incorrectly assumed the index input was always a string, which could lead to errors when a list was provided. This change ensures that both string and list inputs are handled correctly, improving the robustness of the code.