Remove dockercall (resolves #68) #78

jvivian · 2017-01-20T08:28:37Z

No description provided.

Now in core Toil (>= 3.5.0)

Fix missing subprocess prefix for some check_call / check_output calls

Some PEP adjustments

fnothaft

LGTM! Dropped two comments (no changes needed) inline. Thanks for doing this change, @jvivian!

fnothaft · 2017-01-20T08:33:06Z

src/toil_lib/tools/aligners.py

@@ -114,6 +114,7 @@ def run_bwakit(job, config, sort=True, trim=False, mark_secondary=False):
    :rtype: str
    """
    work_dir = job.fileStore.getLocalTempDir()
+    rg = None


Why'd we need this change here?

The if/elif tree that contains assignments to rg doesn't have an else, so pycharm flags it as possibly undefined variable. We can leave this assigned to None or change the final conditional to else.

SGTM; thanks, was just curious.

fnothaft · 2017-01-20T08:34:43Z

src/toil_lib/tools/variant_annotation.py

+                         '-e', 'JAVA_OPTS=-Djava.io.tmpdir=/data/ -Xmx{}'.format(job.memory)]
+    dockerCall(job=job, workDir=work_dir,
+               parameters=command,
+               tool='jpfeil/oncotator:1.9--8fffc356981862d50cfacd711b753700b886b605',


We should open a ticket to move this image into cgl-docker-lib.

jvivian · 2017-01-20T08:36:59Z

There's some repetition in the job functions that use JAVA_OPTS or JAVA_OPTIONS since I have to pass in dockerParameters, so I can refactor that out and import it if people want.

@fnothaft — In spark.py I added the subprocess. prefix to some invocations of check_call and check_output, if you want me to revert that let me know.

fnothaft · 2017-01-20T08:38:05Z

@fnothaft — In spark.py I added the subprocess. prefix to some invocations of check_call and check_output, if you want me to revert that let me know.

No, that looks correct to me. I think I have that exact fix sitting on a branch somewhere. Anywho, if it winds up causing an issue, I'll back it out when I'm retesting these soon.

jvivian · 2017-01-20T23:11:58Z

@fnothaft — Only failing test is sparktest due to a service deadlock. I don't have much experience with that if you're able to take a look. If you don't have the bandwidth let me know!

fnothaft · 2017-01-20T23:17:00Z

@jvivian mind if I take a look next week? I'm slammed today.

jvivian · 2017-01-20T23:35:36Z

@fnothaft — no problem, I don't think there's any rush on this.

jvivian · 2017-01-23T21:32:52Z

@fnothaft — We're going to make a new stable release for toil-lib after this is merged which we'll need by the end of the week, so let me know if you can't take a look before then.

fnothaft · 2017-01-23T21:33:54Z

@jvivian sounds good; I will give it a looksee tonight.

fnothaft · 2017-01-25T19:12:45Z

Sorry for the delay; looking at the service deadlock now.

jvivian · 2017-01-25T19:23:46Z

Sorry for the delay; looking at the service deadlock now.

Thanks mate!

fnothaft · 2017-01-25T19:32:44Z

No prob; sorry for the delay; this is a crazy week. BTW, I am getting test failures due to a 403 error when downloading s3://cgl-pipeline-inputs/exome/ci/chr6.normal.bam which is needed for the tests. I don't think this file is accessible to folks outside of UCSC.

fnothaft · 2017-01-25T19:37:31Z

Also, looks good on my side:

(toil_lib)dhcp-46-233:toil-lib fnothaft$ git status
HEAD detached at upstream/issues/68-remove-dockercall
Untracked files:
  (use "git add <file>..." to include in what will be committed)

	Makefile~
	src/toil_lib/programs.py~
	src/toil_lib/spark.py~
	src/toil_lib/test/__init__.py~
	src/toil_lib/test/test_programs.py~
	src/toil_lib/test/test_spark.py~
	toil_lib/

nothing added to commit but untracked files present (use "git add" to track)
(toil_lib)dhcp-46-233:toil-lib fnothaft$ git log | head -n 10
commit 443608d6bbd812956aaee5bc62fe96b7ad9d84dc
Author: John Vivian <[email protected]>
Date:   Fri Jan 20 01:14:00 2017 -0800

    Fix test_urls.py

commit 8a827aef2cbdb9afb49d668fe8ddb21cd8700e81
Author: John Vivian <[email protected]>
Date:   Fri Jan 20 01:06:19 2017 -0800

(toil_lib)dhcp-46-233:toil-lib fnothaft$ make test
PATH=$PATH:/Users/fnothaft/IdeaProjects/toil-lib/bin python2.7 -m pytest -vv --junitxml test-report.xml src
============================================================================================== test session starts ==============================================================================================
platform darwin -- Python 2.7.10, pytest-2.8.3, py-1.4.31, pluggy-0.3.1 -- /Users/fnothaft/IdeaProjects/toil-lib/toil_lib/bin/python2.7
cachedir: .cache
rootdir: /Users/fnothaft/IdeaProjects/toil-lib, inifile: 
collected 14 items 

src/toil_lib/test/test_files.py::test_tarball_files PASSED
src/toil_lib/test/test_files.py::test_copy_files PASSED
src/toil_lib/test/test_files.py::test_consolidate_tarballs_job PASSED
src/toil_lib/test/test_files.py::test_generate_file PASSED
src/toil_lib/test/test_jobs.py::test_map_job PASSED
src/toil_lib/test/test_lib.py::test_flatten PASSED
src/toil_lib/test/test_lib.py::test_partitions PASSED
src/toil_lib/test/test_spark.py::SparkTest::testSparkLocal PASSED

:p

Let me look at the Jenkins logs and see if I can divine what is failing.

jvivian · 2017-01-26T20:24:37Z

@benedictpaten — You're most familiar with the service code if you're able to take a quick look at the jenkins log.

jvivian · 2017-02-13T21:59:21Z

@benedictpaten — Any chance you can take a look at this?

cket · 2017-03-10T20:42:25Z

@jvivian the deadlock fix has been merged into Toil so this should be unblocked

fnothaft · 2017-04-20T21:32:23Z

@cket the tests here fail due to a bucket permission problem. The relevant S3 path is s3://cgl-driver-projects/test/b081862b-a069-424c-8b65-9529c4ab064d/upload_file. Do we have a new bucket to use?

cket · 2017-04-20T22:16:24Z

@fnothaft I just gave proper permissions to the toil-dev account.
Jenkins, test this please.

jvivian added 13 commits January 19, 2017 11:12

Remove dockercall (resolves #68)

b12f305

Now in core Toil (>= 3.5.0)

Change Toil min to 3.5.0

6b1ea87

Update dockerCall in spark.py

20f251f

Fix missing subprocess prefix for some check_call / check_output calls

Update dockerCall in urls.py

5b6c4de

Some PEP adjustments

Update dockerCall for aligners.py

9da4db7

Update dockerCall for indexing.py

2697714

Update dockerCall for mutation_callers.py

f5ff09d

Update dockerCall for preprocessing.py

11cc494

Update dockerCall for QC.py

f1cfd8b

Update dockerCall for quantifiers.py

a9a459b

Update dockerCall for spark_tools.py

cab574b

Update dockerCall for variant_annotation.py

7d432e8

Update dockerCall for variant_manipulation.py

5b23b7e

jvivian added the in progress label Jan 20, 2017

jvivian requested review from cket and fnothaft January 20, 2017 08:28

fnothaft approved these changes Jan 20, 2017

View reviewed changes

Update Toil version in Makefile

0408515

jvivian added 2 commits January 20, 2017 01:06

Fix test_validators.py

8a827ae

Fix test_urls.py

443608d

jvivian mentioned this pull request Jan 20, 2017

Move jpfeil/oncotator:1.9 to cgl-docker-lib #79

Open

This was referenced Jan 30, 2017

Add support for Redwood #69

Open

Output consolidation (resolves #3) BD2KGenomics/toil-rnaseq-sc#6

Merged

jvivian mentioned this pull request Feb 14, 2017

Service deadlock detection triggers when there is no service deadlock DataBiosphere/toil#1509

Closed

fnothaft mentioned this pull request Apr 20, 2017

Remove dockercall (resolves #68) #89

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove dockercall (resolves #68) #78

Remove dockercall (resolves #68) #78

jvivian commented Jan 20, 2017

fnothaft left a comment

fnothaft Jan 20, 2017

jvivian Jan 20, 2017

fnothaft Jan 20, 2017

fnothaft Jan 20, 2017

jvivian commented Jan 20, 2017

fnothaft commented Jan 20, 2017

jvivian commented Jan 20, 2017

fnothaft commented Jan 20, 2017

jvivian commented Jan 20, 2017

jvivian commented Jan 23, 2017

fnothaft commented Jan 23, 2017

fnothaft commented Jan 25, 2017

jvivian commented Jan 25, 2017

fnothaft commented Jan 25, 2017

fnothaft commented Jan 25, 2017

jvivian commented Jan 26, 2017

jvivian commented Feb 13, 2017

cket commented Mar 10, 2017

fnothaft commented Apr 20, 2017

cket commented Apr 20, 2017

Remove dockercall (resolves #68) #78

Are you sure you want to change the base?

Remove dockercall (resolves #68) #78

Conversation

jvivian commented Jan 20, 2017

fnothaft left a comment

Choose a reason for hiding this comment

fnothaft Jan 20, 2017

Choose a reason for hiding this comment

jvivian Jan 20, 2017

Choose a reason for hiding this comment

fnothaft Jan 20, 2017

Choose a reason for hiding this comment

fnothaft Jan 20, 2017

Choose a reason for hiding this comment

jvivian commented Jan 20, 2017

fnothaft commented Jan 20, 2017

jvivian commented Jan 20, 2017

fnothaft commented Jan 20, 2017

jvivian commented Jan 20, 2017

jvivian commented Jan 23, 2017

fnothaft commented Jan 23, 2017

fnothaft commented Jan 25, 2017

jvivian commented Jan 25, 2017

fnothaft commented Jan 25, 2017

fnothaft commented Jan 25, 2017

jvivian commented Jan 26, 2017

jvivian commented Feb 13, 2017

cket commented Mar 10, 2017

fnothaft commented Apr 20, 2017

cket commented Apr 20, 2017