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.

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

🧹 D3b-ify Sentieon Updates #15

Merged
merged 11 commits into from
Feb 27, 2025
Merged

🧹 D3b-ify Sentieon Updates #15

merged 11 commits into from
Feb 27, 2025

Conversation

dmiller15
Copy link
Contributor

@dmiller15 dmiller15 commented Feb 14, 2025

Description

Ports over the changes made by Sentieon to their Long Reads workflows.
Integrates their new CLI tool and models

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Test Configuration:

  • Environment:
  • Test files:

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings
  • I have committed any related changes to the PR

@dmiller15 dmiller15 added feature New functionality bix-dev This issue or pull request is bix-dev work labels Feb 14, 2025
@dmiller15 dmiller15 self-assigned this Feb 14, 2025
Copy link
Member

@migbro migbro left a comment

Choose a reason for hiding this comment

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

Seems good overall, just some small concerns and edits. Not sure if you can address or Haodong needs to

$include: ../scripts/get_dnascope_model.py
arguments:
- position: 0
valueFrom: 'pip install pyyaml requests;'
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I am a big fan of having to install a package every time. Can we find/create an image that has what's needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normally I'd agree but:

  • this isn't part of the workflow and would only be run in a one-off context
  • there really isn't a good docker out there that has pyyaml and requests built in

secondaryFiles:
- pattern: .fai
required: true
- pattern: ^.dict
Copy link
Member

Choose a reason for hiding this comment

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

Is it that the dict is not actually needed, or the tool will create it if it doesn't exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hard to say. The tool is a bit of a panacea so it might be required for different run modes but not for everything.

prefix: --minimap2_args
util_sort_args:
type: string?
doc: "Extra arguments for sentieon util sort (default: '--cram_write_options version=3.0,compressor=rans')"
Copy link
Member

Choose a reason for hiding this comment

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

So --util_sort_args takes in by default an arg that looks like an option? Maybe it's a wrapper thing that's not apparent from here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what it says. Seems like it's the equivalent of doing "extra args"

- pattern: .crai
required: false
outputBinding:
glob: ["*.cram", "*.bam"]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
glob: ["*.cram", "*.bam"]
glob: "*.[b|cr]am"

this works too, right?

Copy link
Contributor Author

@dmiller15 dmiller15 Feb 27, 2025

Choose a reason for hiding this comment

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

No. Haodong is leveraging the fact that CWL's glob declaration can take an array of strings. What you're probably thinking about is *.{b,cr}am which would be a singular string that can grab both BAMs and CRAMs. Both solutions are acceptable.

Co-authored-by: Miguel Brown <[email protected]>
@dmiller15 dmiller15 requested a review from migbro February 27, 2025 16:27
Copy link
Member

@migbro migbro left a comment

Choose a reason for hiding this comment

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

send it!

@dmiller15 dmiller15 merged commit 35adcfb into main Feb 27, 2025
1 check passed
@dmiller15 dmiller15 deleted the dm-d3bify-changes branch February 27, 2025 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bix-dev This issue or pull request is bix-dev work feature New functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants