[ADAM-1481] Refactor ADAMContext loadXxx methods for consistency #1487

heuermh · 2017-04-12T23:50:58Z

I flagged most of the pathName method parameter param doc strings with (todo: confirm) because it is not immediately clear which support glob/directories and which do not.

There is also some inconsistency wrt when .gz is supported and when it is not.

coveralls · 2017-04-13T00:01:04Z

Coverage increased (+0.03%) to 81.667% when pulling 072ccf5 on heuermh:load-cleanup into 93b32c6 on bigdatagenomics:master.

AmplabJenkins · 2017-04-13T00:03:59Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1943/
Test PASSed.

AmplabJenkins · 2017-04-13T00:04:09Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1944/

Build result: FAILURE

[...truncated 16 lines...] > /home/jenkins/git2/bin/git rev-parse origin/pr/1487/merge^{commit} # timeout=10 > /home/jenkins/git2/bin/git branch -a --contains b99480b8032090dce76fdfb1cce900bb7800a5ff # timeout=10 > /home/jenkins/git2/bin/git rev-parse remotes/origin/pr/1487/merge^{commit} # timeout=10Checking out Revision b99480b8032090dce76fdfb1cce900bb7800a5ff (origin/pr/1487/merge) > /home/jenkins/git2/bin/git config core.sparsecheckout # timeout=10 > /home/jenkins/git2/bin/git checkout -f b99480b8032090dce76fdfb1cce900bb7800a5ffFirst time build. Skipping changelog.Triggering ADAM-prb ? 2.3.0,2.11,1.6.1,centosTriggering ADAM-prb ? 2.6.0,2.10,1.6.1,centosTriggering ADAM-prb ? 2.3.0,2.11,2.0.0,centosTriggering ADAM-prb ? 2.3.0,2.10,2.0.0,centosTriggering ADAM-prb ? 2.6.0,2.11,2.0.0,centosTriggering ADAM-prb ? 2.6.0,2.10,2.0.0,centosTriggering ADAM-prb ? 2.3.0,2.10,1.6.1,centosTriggering ADAM-prb ? 2.6.0,2.11,1.6.1,centosADAM-prb ? 2.3.0,2.11,1.6.1,centos completed with result FAILUREADAM-prb ? 2.6.0,2.10,1.6.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.11,2.0.0,centos completed with result FAILUREADAM-prb ? 2.3.0,2.10,2.0.0,centos completed with result FAILUREADAM-prb ? 2.6.0,2.11,2.0.0,centos completed with result FAILUREADAM-prb ? 2.6.0,2.10,2.0.0,centos completed with result FAILUREADAM-prb ? 2.3.0,2.10,1.6.1,centos completed with result FAILUREADAM-prb ? 2.6.0,2.11,1.6.1,centos completed with result FAILURENotifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'
Test FAILed.

coveralls · 2017-04-13T00:20:55Z

Coverage decreased (-0.1%) to 81.508% when pulling 8082b2c on heuermh:load-cleanup into 93b32c6 on bigdatagenomics:master.

coveralls · 2017-04-13T00:20:57Z

Coverage increased (+0.04%) to 81.674% when pulling 8082b2c on heuermh:load-cleanup into 93b32c6 on bigdatagenomics:master.

AmplabJenkins · 2017-04-13T00:24:05Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1945/
Test PASSed.

fnothaft · 2017-04-13T15:58:58Z

@heuermh After merging #1482, this branch has conflicts. Can you rebase?

heuermh · 2017-04-13T16:09:28Z

Will do. I have more changes coming.

fnothaft · 2017-04-13T16:09:55Z

Sounds good! Would you like me to hold off on reviewing until you're done with the changes?

heuermh · 2017-04-13T17:07:32Z

If you have feedback on some of the open questions now, that would be helpful.

fnothaft · 2017-04-13T17:15:55Z

What are the open questions? Are they the following? If so, answers below:

I flagged most of the pathName method parameter param doc strings with (todo: confirm) because it is not immediately clear which support glob/directories and which do not.

All of the loadX methods should support globs and directories.

There is also some inconsistency wrt when .gz is supported and when it is not.

GZIPed VCF is loaded (and even split if BGZIPed)
All of the files that use sc.textFile (FASTA, all of the feature formats) will load both GZIPed files (as a single split) and BZIP2 files
FileInputFormat (FASTQ) will load GZIPed data, but there is an issue, see Splittable compression and FASTQ #1457

I'm not sure about GZIPed SAM.

heuermh · 2017-04-13T18:56:19Z

All of the loadX methods should support globs and directories.

Some clearly do not, loadIndexedBam, for example.

Others forward to loadAvro and sc.newAPIHadoopFile without going through getFsAndFiles and related, do those support globs and directories? E.g. loadInterleavedFastq forwards pathName: String to sc.newAPIHadoopFile.

There is also some inconsistency wrt when .gz is supported and when it is not.

Thanks, we need to make sure .gz and .bz2 extensions pass format guessing checks where appropriate then.

fnothaft · 2017-04-13T19:37:11Z

All of the loadX methods should support globs and directories.

Some clearly do not, loadIndexedBam, for example.

https://github.com/bigdatagenomics/adam/blob/master/adam-core/src/test/scala/org/bdgenomics/adam/rdd/ADAMContextSuite.scala#L369-L375 ;)

Others forward to loadAvro and sc.newAPIHadoopFile without going through getFsAndFiles and related, do those support globs and directories? E.g. loadInterleavedFastq forwards pathName: String to sc.newAPIHadoopFile.

In general, they should, unless the underlying InputFormat is doing something funny.

Thanks, we need to make sure .gz and .bz2 extensions pass format guessing checks where appropriate then.

+1

heuermh · 2017-04-13T21:06:56Z

All of the loadX methods should support globs and directories.

Some clearly do not, loadIndexedBam, for example.

https://github.com/bigdatagenomics/adam/blob/master/adam-core/src/test/scala/org/bdgenomics/adam/rdd/ADAMContextSuite.scala#L369-L375 ;)

Dang it! Who made that work and forgot to update the @param docs? :)

fnothaft · 2017-04-13T21:08:42Z

Dang it! Who made that work and forgot to update the @param docs? :)

My bad! ;)

coveralls · 2017-04-13T22:30:53Z

Coverage increased (+0.04%) to 81.674% when pulling 579126a on heuermh:load-cleanup into 93b32c6 on bigdatagenomics:master.

AmplabJenkins · 2017-04-13T22:32:45Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1946/

Build result: FAILURE

[...truncated 16 lines...] > /home/jenkins/git2/bin/git rev-parse 579126a^{commit} # timeout=10 > /home/jenkins/git2/bin/git branch -a --contains 579126a # timeout=10 > /home/jenkins/git2/bin/git rev-parse remotes/origin/pr/1487/head^{commit} # timeout=10Checking out Revision 579126a (origin/pr/1487/head) > /home/jenkins/git2/bin/git config core.sparsecheckout # timeout=10 > /home/jenkins/git2/bin/git checkout -f 579126af17f2b882c715080dd75e309bfda3ebf5First time build. Skipping changelog.Triggering ADAM-prb ? 2.3.0,2.11,1.6.1,centosTriggering ADAM-prb ? 2.6.0,2.10,1.6.1,centosTriggering ADAM-prb ? 2.3.0,2.11,2.0.0,centosTriggering ADAM-prb ? 2.3.0,2.10,2.0.0,centosTriggering ADAM-prb ? 2.6.0,2.11,2.0.0,centosTriggering ADAM-prb ? 2.6.0,2.10,2.0.0,centosTriggering ADAM-prb ? 2.3.0,2.10,1.6.1,centosTriggering ADAM-prb ? 2.6.0,2.11,1.6.1,centosADAM-prb ? 2.3.0,2.11,1.6.1,centos completed with result FAILUREADAM-prb ? 2.6.0,2.10,1.6.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.11,2.0.0,centos completed with result FAILUREADAM-prb ? 2.3.0,2.10,2.0.0,centos completed with result FAILUREADAM-prb ? 2.6.0,2.11,2.0.0,centos completed with result FAILUREADAM-prb ? 2.6.0,2.10,2.0.0,centos completed with result FAILUREADAM-prb ? 2.3.0,2.10,1.6.1,centos completed with result FAILUREADAM-prb ? 2.6.0,2.11,1.6.1,centos completed with result FAILURENotifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'
Test FAILed.

…eral

coveralls · 2017-04-14T01:30:43Z

Coverage increased (+0.3%) to 81.854% when pulling c7814c9 on heuermh:load-cleanup into 6c32a02 on bigdatagenomics:master.

AmplabJenkins · 2017-04-14T01:34:38Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1947/
Test PASSed.

coveralls · 2017-04-14T14:46:03Z

Coverage increased (+0.2%) to 81.809% when pulling f3d1d3c on heuermh:load-cleanup into 6c32a02 on bigdatagenomics:master.

AmplabJenkins · 2017-04-14T14:49:16Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1948/
Test PASSed.

heuermh · 2017-04-14T14:50:36Z

This one is ready to review!

fnothaft

Let's check for compressors using the Hadoop factory, but otherwise this LGTM! Thanks @heuermh! This is a great cleanup.

fnothaft · 2017-04-15T21:27:11Z

adam-core/src/main/scala/org/bdgenomics/adam/rdd/ADAMContext.scala

-   * @param pathName The path name to load SAM/BAM/CRAM formatted alignments from. Globs/directories are
-   *   supported (todo: confirm).
+   * @param pathName The path name to load SAM/BAM/CRAM formatted alignment records from.
+   *   Globs/directories are supported (todo: confirm).


Can confirm; globs/directories are supported.

fnothaft · 2017-04-15T21:31:29Z

adam-core/src/main/scala/org/bdgenomics/adam/util/FileExtensions.scala

+   */
+  private[adam] def isBedExt(pathName: String): Boolean = {
+    pathName.endsWith(".bed") ||
+      pathName.endsWith(".bed.gz") ||


As an aside, for all of the compressed file format checks, we should use Hadoop's CompressionCodecFactory to check if the path is a compressed file. This allows users to swap in additional compression codecs.

ok, I suppose we could do an isCompressed check in ADAMContext, since it has access to the Hadoop configuration, then trim off the extension before checking with FileExtensions

… extensions.

coveralls · 2017-04-18T21:01:00Z

Coverage increased (+0.4%) to 81.942% when pulling b021e2c on heuermh:load-cleanup into 6c32a02 on bigdatagenomics:master.

AmplabJenkins · 2017-04-18T21:04:08Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1954/
Test PASSed.

fnothaft

This LGTM from a code perspective, but I think we should mention in each of the docstrings that we do support compressed text files, and where that comes from. I think we're very close here; thanks for driving this @heuermh!

fnothaft · 2017-04-19T06:01:39Z

adam-apis/src/main/scala/org/bdgenomics/adam/apis/java/JavaADAMContext.scala

-   * * .ifq/.ifq.gz/.ifq.bz2 as interleaved FASTQ format.
+   * * .fa/.fasta as FASTA format,
+   * * .fq/.fastq as FASTQ format, and
+   * * .ifq as interleaved FASTQ format.


Should mention that we read the configured compression codecs from the Hadoop config for this, which by default include .gz and .bz2, but can include more.

fnothaft · 2017-04-19T06:01:49Z

adam-apis/src/main/scala/org/bdgenomics/adam/apis/java/JavaADAMContext.scala

-   * If the path name has a .fa/.fa.gz/.fa.bz2/.fasta/.fasta.gz/.fasta.bz2 extension,
-   * load as FASTA format. Else, fall back to Parquet + Avro.
+   * If the path name has a .fa/.fasta extension, load as FASTA format.
+   * Else, fall back to Parquet + Avro.


fnothaft · 2017-04-19T06:01:54Z

adam-apis/src/main/scala/org/bdgenomics/adam/apis/java/JavaADAMContext.scala

@@ -92,7 +92,7 @@ class JavaADAMContext(val ac: ADAMContext) extends Serializable {
   *
   * Loads path names ending in:
   * * .bam/.cram/.sam as BAM/CRAM/SAM format and
-   * * .ifq/.ifq.gz/.ifq.bz2 as interleaved FASTQ format.
+   * * .ifq as interleaved FASTQ format.


fnothaft · 2017-04-19T06:01:58Z

adam-apis/src/main/scala/org/bdgenomics/adam/apis/java/JavaADAMContext.scala

+   * * .gff3 as GFF3 format,
+   * * .gtf/.gff as GTF/GFF2 format,
+   * * .narrow[pP]eak as NarrowPeak format, and
+   * * .interval_list as IntervalList format.


fnothaft · 2017-04-19T06:02:02Z

adam-apis/src/main/scala/org/bdgenomics/adam/apis/java/JavaADAMContext.scala

+   * * .gff3 as GFF3 format,
+   * * .gtf/.gff as GTF/GFF2 format,
+   * * .narrow[pP]eak as NarrowPeak format, and
+   * * .interval_list as IntervalList format.


fnothaft · 2017-04-19T06:02:15Z

adam-core/src/main/scala/org/bdgenomics/adam/rdd/ADAMContext.scala

+   * * .gff3 as GFF3 format,
+   * * .gtf/.gff as GTF/GFF2 format,
+   * * .narrow[pP]eak as NarrowPeak format, and
+   * * .interval_list as IntervalList format.


fnothaft · 2017-04-19T06:02:19Z

adam-core/src/main/scala/org/bdgenomics/adam/rdd/ADAMContext.scala

+   * * .gff3 as GFF3 format,
+   * * .gtf/.gff as GTF/GFF2 format,
+   * * .narrow[pP]eak as NarrowPeak format, and
+   * * .interval_list as IntervalList format.


fnothaft · 2017-04-19T06:02:25Z

adam-core/src/main/scala/org/bdgenomics/adam/rdd/ADAMContext.scala

-   * * .ifq/.ifq.gz/.ifq.bz2 as interleaved FASTQ format.
+   * * .fa/.fasta as FASTA format,
+   * * .fq/.fastq as FASTQ format, and
+   * * .ifq as interleaved FASTQ format.


fnothaft · 2017-04-19T06:02:31Z

adam-core/src/main/scala/org/bdgenomics/adam/rdd/ADAMContext.scala

@@ -1721,7 +1744,7 @@ class ADAMContext(@transient val sc: SparkContext) extends Serializable with Log
   *
   * Loads path names ending in:
   * * .bam/.cram/.sam as BAM/CRAM/SAM format and
-   * * .ifq/.ifq.gz/.ifq.bz2 as interleaved FASTQ format.
+   * * .ifq as interleaved FASTQ format.


coveralls · 2017-04-19T13:56:03Z

Coverage increased (+0.4%) to 82.018% when pulling 9b13e6e on heuermh:load-cleanup into 6c32a02 on bigdatagenomics:master.

AmplabJenkins · 2017-04-19T13:59:08Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1955/
Test PASSed.

fnothaft

LGTM! Do you want to squash in any specific way, or would you like me to hit the green squash button?

heuermh · 2017-04-19T16:59:18Z

Green squash is fine!

heuermh · 2017-04-19T17:03:51Z

Thank you!

heuermh added 4 commits April 13, 2017 20:12

Refactor ADAMContext loadXxx methods for consistency, commit 1 of sev…

b0505c4

…eral

Format long lines

7e6bca7

Minor doc fixes

f894f8a

Add FileExtensions, confirm pathName doc strings

6c988b7

heuermh force-pushed the load-cleanup branch from 579126a to 6c988b7 Compare April 14, 2017 01:19

Missed merge flotsam

c7814c9

Moar doc fixes, add'l load methods

f3d1d3c

fnothaft requested changes Apr 15, 2017

View reviewed changes

fnothaft mentioned this pull request Apr 17, 2017

[ADAM-1480] Add switch to disable the fast concat method. #1479

Merged

Use compression codecs from Hadoop configuration instead of hardcoded…

b021e2c

… extensions.

fnothaft requested changes Apr 19, 2017

View reviewed changes

Document compressed file support via Hadoop compression codecs.

9b13e6e

fnothaft approved these changes Apr 19, 2017

View reviewed changes

fnothaft merged commit 93d17b4 into bigdatagenomics:master Apr 19, 2017

heuermh deleted the load-cleanup branch April 19, 2017 17:03

heuermh mentioned this pull request Apr 19, 2017

Improve metrics coverage across ADAMContext load methods #1428

Closed

heuermh added this to the 0.23.0 milestone Dec 7, 2017

[ADAM-1481] Refactor ADAMContext loadXxx methods for consistency #1487

[ADAM-1481] Refactor ADAMContext loadXxx methods for consistency #1487

Conversation

heuermh commented Apr 12, 2017 • edited Loading

coveralls commented Apr 13, 2017 • edited Loading

AmplabJenkins commented Apr 13, 2017

AmplabJenkins commented Apr 13, 2017

Build result: FAILURE

coveralls commented Apr 13, 2017

coveralls commented Apr 13, 2017 • edited Loading

AmplabJenkins commented Apr 13, 2017

fnothaft commented Apr 13, 2017

heuermh commented Apr 13, 2017

fnothaft commented Apr 13, 2017

heuermh commented Apr 13, 2017

fnothaft commented Apr 13, 2017

heuermh commented Apr 13, 2017

fnothaft commented Apr 13, 2017

heuermh commented Apr 13, 2017

fnothaft commented Apr 13, 2017

coveralls commented Apr 13, 2017 • edited Loading

AmplabJenkins commented Apr 13, 2017

Build result: FAILURE

coveralls commented Apr 14, 2017 • edited Loading

AmplabJenkins commented Apr 14, 2017

coveralls commented Apr 14, 2017 • edited Loading

AmplabJenkins commented Apr 14, 2017

heuermh commented Apr 14, 2017

fnothaft left a 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

coveralls commented Apr 18, 2017 • edited Loading

AmplabJenkins commented Apr 18, 2017

fnothaft left a 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

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

coveralls commented Apr 19, 2017 • edited Loading

AmplabJenkins commented Apr 19, 2017

fnothaft left a comment

Choose a reason for hiding this comment

heuermh commented Apr 19, 2017

heuermh commented Apr 19, 2017

heuermh commented Apr 12, 2017 •

edited

Loading

coveralls commented Apr 13, 2017 •

edited

Loading

coveralls commented Apr 13, 2017 •

edited

Loading

coveralls commented Apr 13, 2017 •

edited

Loading

coveralls commented Apr 14, 2017 •

edited

Loading

coveralls commented Apr 14, 2017 •

edited

Loading

coveralls commented Apr 18, 2017 •

edited

Loading

coveralls commented Apr 19, 2017 •

edited

Loading