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

Misc cleanup needed for bigdatagenomics/cannoli#65 #1704

Merged
merged 2 commits into from Sep 12, 2017

Conversation

@fnothaft
Copy link
Member

@fnothaft fnothaft commented Aug 30, 2017

Resolves #1701, #1702, #1703.

@@ -59,11 +60,11 @@ class InterleavedFASTQInFormatter private (
iter.flatMap(frag => {
val reads = converter.convertFragment(frag).toSeq

if (reads.size < 2) {
if (enableLogging && reads.size < 2) {

This comment has been minimized.

@heuermh

heuermh Aug 30, 2017
Member

would simply dropping to log.info accomplish the same goal?

This comment has been minimized.

@fnothaft

fnothaft Aug 30, 2017
Author Member

I mean, it really is a warning message. However, if you're doing development of a piped tool and you want to read your executor logs to figure out why the tool running under the pipe is blowing up, the warnings get in the way of reading the logs.

If you're not 100% sold on this change, I'm OK with dropping it. It was just for my convenience when developing.

This comment has been minimized.

@heuermh

heuermh Aug 30, 2017
Member

I don't have data to test these particular checks (though I suppose I could invent some) and am not all that confident our logging configuration works. Could you try adding

log4j.logger.org.bdgenomics.adam.rdd.fragment.InterleavedFASTQInFormatter=ERROR

to your log4j.properties and see if it does the trick?

This comment has been minimized.

@fnothaft

fnothaft Sep 6, 2017
Author Member

Yeah, agreed. I've rebased this and dropped the commit that resolves #1701.

@coveralls
Copy link

@coveralls coveralls commented Aug 30, 2017

Coverage Status

Coverage decreased (-0.003%) to 83.59% when pulling 69dcfa5 on fnothaft:issues/1701-1702-ifq-cleanup into 511f925 on bigdatagenomics:master.

@AmplabJenkins
Copy link

@AmplabJenkins AmplabJenkins commented Aug 30, 2017

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

Build result: FAILURE

[...truncated 15 lines...] > /home/jenkins/git2/bin/git fetch --tags --progress https://github.com/bigdatagenomics/adam.git +refs/pull/:refs/remotes/origin/pr/ # timeout=15 > /home/jenkins/git2/bin/git rev-parse origin/pr/1704/merge^{commit} # timeout=10 > /home/jenkins/git2/bin/git branch -a -v --no-abbrev --contains ae21c0b # timeout=10Checking out Revision ae21c0b (origin/pr/1704/merge) > /home/jenkins/git2/bin/git config core.sparsecheckout # timeout=10 > /home/jenkins/git2/bin/git checkout -f ae21c0b582a262fb3b7087830e45423c876b97cdFirst time build. Skipping changelog.Triggering ADAM-prb ? 2.3.0,2.11,1.6.1,centosTriggering ADAM-prb ? 2.6.0,2.11,1.6.1,centosTriggering ADAM-prb ? 2.6.0,2.10,1.6.1,centosTriggering ADAM-prb ? 2.6.0,2.10,2.1.0,centosTriggering ADAM-prb ? 2.3.0,2.10,2.1.0,centosTriggering ADAM-prb ? 2.3.0,2.11,2.1.0,centosTriggering ADAM-prb ? 2.3.0,2.10,1.6.1,centosTriggering ADAM-prb ? 2.6.0,2.11,2.1.0,centosADAM-prb ? 2.3.0,2.11,1.6.1,centos completed with result SUCCESSADAM-prb ? 2.6.0,2.11,1.6.1,centos completed with result SUCCESSADAM-prb ? 2.6.0,2.10,1.6.1,centos completed with result FAILUREADAM-prb ? 2.6.0,2.10,2.1.0,centos completed with result SUCCESSADAM-prb ? 2.3.0,2.10,2.1.0,centos completed with result SUCCESSADAM-prb ? 2.3.0,2.11,2.1.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,2.1.0,centos completed with result FAILURENotifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'
Test FAILed.

@heuermh heuermh modified the milestone: 0.23.0 Sep 1, 2017
@fnothaft fnothaft force-pushed the fnothaft:issues/1701-1702-ifq-cleanup branch from 69dcfa5 to 9a08d2c Sep 6, 2017
@fnothaft
Copy link
Member Author

@fnothaft fnothaft commented Sep 6, 2017

This is good to go from my side.

@AmplabJenkins
Copy link

@AmplabJenkins AmplabJenkins commented Sep 6, 2017

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

Build result: FAILURE

[...truncated 15 lines...] > /home/jenkins/git2/bin/git fetch --tags --progress https://github.com/bigdatagenomics/adam.git +refs/pull/:refs/remotes/origin/pr/ # timeout=15 > /home/jenkins/git2/bin/git rev-parse origin/pr/1704/merge^{commit} # timeout=10 > /home/jenkins/git2/bin/git branch -a -v --no-abbrev --contains 2bfd867aff6fd7412a9d05324462bf10b5b8ffa8 # timeout=10Checking out Revision 2bfd867aff6fd7412a9d05324462bf10b5b8ffa8 (origin/pr/1704/merge) > /home/jenkins/git2/bin/git config core.sparsecheckout # timeout=10 > /home/jenkins/git2/bin/git checkout -f 2bfd867aff6fd7412a9d05324462bf10b5b8ffa8First time build. Skipping changelog.Triggering ADAM-prb ? 2.3.0,2.11,1.6.1,centosTriggering ADAM-prb ? 2.6.0,2.11,1.6.1,centosTriggering ADAM-prb ? 2.6.0,2.10,1.6.1,centosTriggering ADAM-prb ? 2.6.0,2.10,2.1.0,centosTriggering ADAM-prb ? 2.3.0,2.10,2.1.0,centosTriggering ADAM-prb ? 2.3.0,2.11,2.1.0,centosTriggering ADAM-prb ? 2.3.0,2.10,1.6.1,centosTriggering ADAM-prb ? 2.6.0,2.11,2.1.0,centosADAM-prb ? 2.3.0,2.11,1.6.1,centos completed with result FAILUREADAM-prb ? 2.6.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.6.0,2.10,2.1.0,centos completed with result FAILUREADAM-prb ? 2.3.0,2.10,2.1.0,centos completed with result FAILUREADAM-prb ? 2.3.0,2.11,2.1.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,2.1.0,centos completed with result FAILURENotifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'
Test FAILed.

@fnothaft fnothaft force-pushed the fnothaft:issues/1701-1702-ifq-cleanup branch from 9a08d2c to c8550bc Sep 6, 2017
@coveralls
Copy link

@coveralls coveralls commented Sep 6, 2017

Coverage Status

Coverage decreased (-0.009%) to 83.463% when pulling c8550bc on fnothaft:issues/1701-1702-ifq-cleanup into 51efbaf on bigdatagenomics:master.

@coveralls
Copy link

@coveralls coveralls commented Sep 6, 2017

Coverage Status

Coverage decreased (-0.4%) to 83.072% when pulling c8550bc on fnothaft:issues/1701-1702-ifq-cleanup into 51efbaf on bigdatagenomics:master.

@AmplabJenkins
Copy link

@AmplabJenkins AmplabJenkins commented Sep 6, 2017

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

Build result: FAILURE

[...truncated 15 lines...] > /home/jenkins/git2/bin/git fetch --tags --progress https://github.com/bigdatagenomics/adam.git +refs/pull/:refs/remotes/origin/pr/ # timeout=15 > /home/jenkins/git2/bin/git rev-parse origin/pr/1704/merge^{commit} # timeout=10 > /home/jenkins/git2/bin/git branch -a -v --no-abbrev --contains 311615c # timeout=10Checking out Revision 311615c (origin/pr/1704/merge) > /home/jenkins/git2/bin/git config core.sparsecheckout # timeout=10 > /home/jenkins/git2/bin/git checkout -f 311615c4341eb3d25d23ae77095be8381a06005aFirst time build. Skipping changelog.Triggering ADAM-prb ? 2.3.0,2.11,1.6.1,centosTriggering ADAM-prb ? 2.6.0,2.11,1.6.1,centosTriggering ADAM-prb ? 2.6.0,2.10,1.6.1,centosTriggering ADAM-prb ? 2.6.0,2.10,2.1.0,centosTriggering ADAM-prb ? 2.3.0,2.10,2.1.0,centosTriggering ADAM-prb ? 2.3.0,2.11,2.1.0,centosTriggering ADAM-prb ? 2.3.0,2.10,1.6.1,centosTriggering ADAM-prb ? 2.6.0,2.11,2.1.0,centosADAM-prb ? 2.3.0,2.11,1.6.1,centos completed with result SUCCESSADAM-prb ? 2.6.0,2.11,1.6.1,centos completed with result SUCCESSADAM-prb ? 2.6.0,2.10,1.6.1,centos completed with result FAILUREADAM-prb ? 2.6.0,2.10,2.1.0,centos completed with result SUCCESSADAM-prb ? 2.3.0,2.10,2.1.0,centos completed with result SUCCESSADAM-prb ? 2.3.0,2.11,2.1.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,2.1.0,centos completed with result FAILURENotifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'
Test FAILed.

@@ -29,7 +29,7 @@ import scala.collection.mutable.ListBuffer
* An OutFormatter that automatically infers whether the piped input is SAM or
* BAM. Autodetecting streamed CRAM is not currently supported.
*/
class AnySAMOutFormatter extends OutFormatter[AlignmentRecord] {
case class AnySAMOutFormatter(stringency: ValidationStringency = ValidationStringency.STRICT) extends OutFormatter[AlignmentRecord] {

This comment has been minimized.

@heuermh

heuermh Sep 6, 2017
Member

From the Jenkins failures, looks like the default ctr parameter value isn't available to Python. Could you add a no-arg apply method or update the Python code?

@fnothaft fnothaft force-pushed the fnothaft:issues/1701-1702-ifq-cleanup branch from c8550bc to f386347 Sep 12, 2017
@AmplabJenkins
Copy link

@AmplabJenkins AmplabJenkins commented Sep 12, 2017

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

@heuermh heuermh merged commit 9cb0c76 into bigdatagenomics:master Sep 12, 2017
1 of 2 checks passed
1 of 2 checks passed
codacy/pr Not so good... This pull request quality could be better.
Details
default Merged build finished.
Details
@heuermh
Copy link
Member

@heuermh heuermh commented Sep 12, 2017

Thank you, @fnothaft

@heuermh heuermh added this to Completed in Release 0.23.0 Jan 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.