Refactor bwa (resolves #320, resolves #297, resolves #322) #324

jvivian · 2016-06-27T22:34:46Z

No description provided.

fnothaft · 2016-06-27T22:41:30Z

I will give this a review...

jvivian · 2016-06-28T00:01:19Z

@fnothaft I can ping you once I remove the needs work label and tests pass.

fnothaft · 2016-06-28T00:22:40Z

@fnothaft I can ping you once I remove the needs work label and tests pass.

That would be superb!

jvivian · 2016-06-28T16:31:59Z

@fnothaft — I spread out the history in an attempt to make it more readable, but its not perfect, so if something looks odd check the finished product.

fnothaft · 2016-06-28T16:45:18Z

Thanks @jvivian; I'll make a pass shortly.

jvivian · 2016-06-28T16:48:18Z

Thanks! — I'm happy to squash commits or make changes to make the review easier.

fnothaft · 2016-06-28T16:58:37Z

@jvivian that would be great actually. I think it might make sense to squash down:

86d36b3 and 7c5eb6f into a single commit that resolves Refactor BWA pipeline to use config/manifest #297
1ce621f, 8613ea3, and 60348dd into a single commit that resolves Add single-end support for BWA #322
cab3f8b, 040cc38, and edc4ea3 into a single commit that resolves Require only reference for BWA #320
I'd probably also squash all of the others down into a single "cleanup" commit.

Thoughts?

jvivian · 2016-06-28T17:02:23Z

Thanks @fnothaft I'll ping you when I've squashed and rebased.

jvivian · 2016-06-28T20:36:14Z

@fnothaft —Almost as asked. I had a demon commit (53f) that gave me merge conflicts with missing code anytime I moved commits around it or tried to squash it.

fnothaft · 2016-06-28T20:37:15Z

Demon commit; nice! I'll take a look.

fnothaft · 2016-06-28T20:58:10Z

src/toil_scripts/adam_gatk_pipeline/align_and_call.py

                        inputs,
                        [uuid,
                         's3://{s3_bucket}/{sequence_dir}/{uuid}_1.fastq.gz'.format(**args),
-                         's3://{s3_bucket}/{sequence_dir}/{uuid}_2.fastq.gz'.format(**args)],
-                        's3://{s3_bucket}/alignment{dir_suffix}'.format(**args),
-                        rg_line).encapsulate()


What happens to rg_line now?

Er. Nevermind. I see that rg_line is now being passed through inputs.

Why isn't rg_line just in inputs to start with? If we're refactoring this, mightn't we factor out passing rg_line as a parameter?

Why isn't rg_line just in inputs to start with?

Looking at the code, I'm not sure if it's even defined anywhere in align_and_call.

rg_line gets set from uuid_items[1], which is taken from uuid_rg from uuid_list, but uuid_list is parsed from the manifest which has no entry for read group... (confused yet?).

fnothaft · 2016-06-30T02:06:37Z

src/toil_scripts/batch_alignment/bwa_alignment.py

+    # If all inputs provided, download from S3
+    download_ref = job.wrapJobFn(download_url_job, inputs.ref, disk='3G')
+    faidx = job.wrapJobFn(run_samtools_faidx, download_ref.rv())
+    # If all the bwa index files are provided


Semantics, but I would note here that the FAI is considered separate from the BWA indices, so we'll check for that later.

fnothaft · 2016-06-30T02:28:34Z

Just made another pass @jvivian! Let me know your thoughts!

jvivian · 2016-06-30T03:28:43Z

Great feedback @fnothaft , thanks! I'll ping you once tests pass.

I also pinged Hannes about the toil-scripts google group.

fnothaft · 2016-06-30T18:36:13Z

src/toil_scripts/batch_alignment/README.md

@@ -2,8 +2,9 @@
 ### Guide: Running the BWA Pipeline using Toil

 This guide attempts to walk the user through running this pipeline from start to finish. If there are any questions
-please contact John Vivian ([email protected]). If you find any errors or corrections please feel free to make a 
-pull request.  Feedback of any kind is appreciated.
+please contact the Toil team at: [email protected] 


Move to project level readme, no? We should have it there, and not duplicate it across all pipeline readmes.

Oops, I'll do that now.

fnothaft · 2016-06-30T18:36:58Z

Dropped two comments; otherwise branch is good to squash down and rebase from my side.

jvivian · 2016-07-06T23:09:51Z

@hannes-ucsc — This has been reviewed, squashed and rebased, and is ready to merge.

hannes-ucsc · 2016-07-07T02:00:21Z

Hmm, don't like how the commits are organized. I should be able to revert a commit and that should undo exactly one change. I bet you that 48ffd14 touches on all of the above commits.

In that sense, I'd rather have just one giant commit. But now that I have to share my Merge button superpowers, diluted beyond recognition to a mere plus or minus one, I don't want to hold this up. Your thoughts, @fnothaft?

jvivian · 2016-07-07T02:19:15Z

I squashed the commits because I had to rebase and didn't want to
potentially deal with 33 commits worth of conflicts. I guess I should've
pinged you for review before that, but frank had already looked at them at
that point so I thought I was clear to, sorry about that.

On Wed, Jul 6, 2016, 7:00 PM Hannes Schmidt [email protected]
wrote:

Hmm, don't like how the commits are organized. I should be able to revert
a commit and that should undo exactly one change. I bet you that 48ffd14
48ffd14
touches on all of the above commits.

In that sense, I'd rather have just one giant commit. But now that I have
to share my Merge button superpowers, diluted beyond recognition to a mere
plus or minus one, I don't want to hold this up. Your thoughts, @fnothaft
https://github.com/fnothaft?

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#324 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AHOawI5fUpEcRLLxWFsXQtXH-VfQFb8bks5qTF22gaJpZM4I_kue
.

fnothaft · 2016-07-07T02:29:55Z

I should be able to revert a commit and that should undo exactly one change.

In that sense, I'd rather have just one giant commit. But now that I have to share my Merge button superpowers, diluted beyond recognition to a mere plus or minus one, I don't want to hold this up. Your thoughts, @fnothaft?

I strongly +1 the 1 commit = 1 change approach. That's how we run all of the bdgenomics repositories, and I'd love to have this be an official policy over in BD2KGenomics as well.

hannes-ucsc · 2016-07-07T03:07:48Z

@jvivian, no need to apologize. Organizing changes into commits is an art, especially when it comes to large refactorings. Given what @fnothaft said, I think squashing all commits down to one would be best. I do like your detailed commit messages, so be sure to merge them all together so as to not lose that information.

Remove email as point of contact. #324 will add the Toil google group as point of contact in parent README.

Add missing uuid to config Fix bad unpacking Add sample URLs to logger Add suffix option to test Remove unused import Move google group line to parent README Add whitespace before map_job Add comment explaining map_job Fix missing trim attribute in test Clean up logic for reference indices Change point of contact to google group Improve legibility with comments on their own lines Unindent FollowOn job Add logging for index creation Add inline explaining alt file can't be generated Clarify that FAI is created separately from BWA indices Add docstring to move_file_job Add inline comment to explain disk space Reverse use of csv Replace run_bwakit arguments with config Fix imports and spacing in bwa_alignment Add /data/ prefix Add missing import Move run_bwa_index and run_samtools_faidx to indexing.py Remove unnecessary renaming Colocate parameter logic Add inline comments for conditionals and rg Provide clarity for rg_line Move required_length to common lib Remove bad print statement Add comments explaining conditional Clarify samples type Clarify file_size option Remove attempt at humor (+5 squashed commits) Squashed commits: Move bwa_kit to tool library (resolves #297) Add bwa_index and samtools_faidx to tool library Indent Job.Runner, add sanity checks options -> args (convention) Print docstring help if no arguments provided Remove config options Change config formatting Add tab-friendly config and manifest names Replace old bwa_kit job with download_sample_and_align Clean imports Move top docstring to main() Fix test call Fix adam_gatk_pipeline's call to download_reference_files Add job version of `move_files` Update README.md Remove deprecated launch scripts Require only reference (resolves #320) Change download_shared_files -> download_reference_files Remove old reference requirements Add single end support for BWA (resolves #322) Edit manifest docs to include single-end Add nargs range for single-end and paired-end Replace parse_config job with parse_manifest function

jvivian · 2016-07-11T22:17:35Z

@hannes-ucsc — Ready for review/merge

jvivian added the needs work label Jun 27, 2016

jvivian added this to the 2.1 Release milestone Jun 27, 2016

jpfeil added the in progress label Jun 27, 2016

jvivian added the in progress label Jun 27, 2016

jvivian force-pushed the issues/297-refactor-bwa branch 2 times, most recently from d75e51c to b812b86 Compare June 28, 2016 04:58

jvivian removed the needs work label Jun 28, 2016

jvivian added the needs work label Jun 28, 2016

jvivian force-pushed the issues/297-refactor-bwa branch from b812b86 to c14ac3f Compare June 28, 2016 20:00

jvivian removed the needs work label Jun 28, 2016

fnothaft reviewed Jun 28, 2016
View reviewed changes

fnothaft reviewed Jun 30, 2016
View reviewed changes

jvivian force-pushed the issues/297-refactor-bwa branch 2 times, most recently from c783e8c to 48ffd14 Compare July 6, 2016 19:19

hannes-ucsc added the needs work label Jul 7, 2016

jvivian force-pushed the issues/297-refactor-bwa branch from 48ffd14 to 7b316cc Compare July 7, 2016 21:11

jvivian added a commit that referenced this pull request Jul 9, 2016

Update UNC README.md

801038e

Remove email as point of contact. #324 will add the Toil google group as point of contact in parent README.

jvivian added a commit that referenced this pull request Jul 9, 2016

Update UNC README.md (resolves #342)

e6fd717

Remove email as point of contact. #324 will add the Toil google group as point of contact in parent README.

jvivian removed the needs work label Jul 9, 2016

jvivian force-pushed the issues/297-refactor-bwa branch from 7b316cc to eadc6b5 Compare July 9, 2016 22:04

hannes-ucsc merged commit d4bc20f into master Jul 11, 2016

hannes-ucsc removed the in progress label Jul 11, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor bwa (resolves #320, resolves #297, resolves #322) #324

Refactor bwa (resolves #320, resolves #297, resolves #322) #324

jvivian commented Jun 27, 2016

fnothaft commented Jun 27, 2016

jvivian commented Jun 28, 2016

fnothaft commented Jun 28, 2016

jvivian commented Jun 28, 2016 •

edited

Loading

fnothaft commented Jun 28, 2016

jvivian commented Jun 28, 2016

fnothaft commented Jun 28, 2016 •

edited

Loading

jvivian commented Jun 28, 2016

jvivian commented Jun 28, 2016

fnothaft commented Jun 28, 2016

fnothaft Jun 28, 2016

fnothaft Jun 28, 2016

fnothaft Jun 28, 2016

jvivian Jun 29, 2016

fnothaft Jun 30, 2016

fnothaft Jun 30, 2016

fnothaft commented Jun 30, 2016

jvivian commented Jun 30, 2016

fnothaft Jun 30, 2016

jvivian Jun 30, 2016

fnothaft commented Jun 30, 2016

jvivian commented Jul 6, 2016

hannes-ucsc commented Jul 7, 2016

jvivian commented Jul 7, 2016

fnothaft commented Jul 7, 2016

hannes-ucsc commented Jul 7, 2016

jvivian commented Jul 11, 2016

Refactor bwa (resolves #320, resolves #297, resolves #322) #324

Refactor bwa (resolves #320, resolves #297, resolves #322) #324

Conversation

jvivian commented Jun 27, 2016

fnothaft commented Jun 27, 2016

jvivian commented Jun 28, 2016

fnothaft commented Jun 28, 2016

jvivian commented Jun 28, 2016 • edited Loading

fnothaft commented Jun 28, 2016

jvivian commented Jun 28, 2016

fnothaft commented Jun 28, 2016 • edited Loading

jvivian commented Jun 28, 2016

jvivian commented Jun 28, 2016

fnothaft commented Jun 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fnothaft commented Jun 30, 2016

jvivian commented Jun 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fnothaft commented Jun 30, 2016

jvivian commented Jul 6, 2016

hannes-ucsc commented Jul 7, 2016

jvivian commented Jul 7, 2016

fnothaft commented Jul 7, 2016

hannes-ucsc commented Jul 7, 2016

jvivian commented Jul 11, 2016

jvivian commented Jun 28, 2016 •

edited

Loading

fnothaft commented Jun 28, 2016 •

edited

Loading