Skip to content

Conversation

@bridgetallen
Copy link
Collaborator

  • Replaced staging file download with conditional local path handling
  • Added checks for bedtools FASTQ output existence and file size
  • Updated output to return a KBase report with Reads object reference
  • Printed bedtools version for debugging container environment
  • Improved parameter logging and error handling in do_analysis()
  • Updated test BAM filename and added mock for download_staging_file
  • Fixed test to assert on report_name and report_ref in returned output

- Replaced staging file download with conditional local path handling
- Added checks for bedtools FASTQ output existence and file size
- Updated output to return a KBase report with Reads object reference
- Printed bedtools version for debugging container environment
- Improved parameter logging and error handling in do_analysis()
- Updated test BAM filename and added mock for download_staging_file
- Fixed test to assert on report_name and report_ref in returned output
@dakotablair
Copy link
Collaborator

This PR is looking good! General feedback:

  • Be sure your code adheres to PEP 8
    • "Imports are always put at the top of the file"
    • Line lengths are appropriate.
  • Remove any commented out lines of code.
    • In the kb_bedtoolsImpl.py some comments are actually KB SDK directives, so take care to preserve them when modifying that file.

Beyond that, I have a few more requests:

  • Remove references to ExampleReadsApp. Now that there is a working app, this repo does not need the example app.

Other than that, it looks good to me! Once these changes are made we can merge this PR to main.

logging.warning(f"{'@'*30} params: {params}")
print(f"{json.dumps(params)=}")
bam_file = params['bam_file']
staging_path = bam_file if os.path.isfile(bam_file) else os.path.join("/staging/", bam_file)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Be sure your line lengths are compatible with PEP8.

Bridget Allen added 2 commits July 23, 2025 17:52
- adheres to PEP 8
- Imports are always put at the top of the file"
- Line lengths are appropriate
- Remove references to ExampleReadsApp
- remove commented out lines
@dakotablair dakotablair merged commit e9d7793 into main Jul 23, 2025
4 checks passed
@dakotablair dakotablair deleted the july23 branch July 23, 2025 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants