Auto upload patch #369

ackagel · 2020-12-12T01:24:49Z

Purpose

Replaces kiosk-client with direct calls to deepcell API. This bypasses the kiosk client upload bug described in issue #302 so smaller zip files don't need to be created; we can just upload one. Consequently closes #302

Implementation

Replaces kiosk-client usage in deepcell_service_utils.py with direct calls to deepcell's api. This more closely mimics direct user interaction with deepcell.org.

Remaining issues

Might want to check with Van Valen lab on how stable their deepcell API will be.

ackagel · 2021-01-04T21:33:29Z

tested .zip upload of ~183MB. Looks like the uploading is fine, but it seems like deepcell can have an issue where it times itself out with too big of an upload (happens on website too). That looks something like:

Traceback (most recent call last):
File "/usr/src/app/redis_consumer/consumers/base_consumer.py", line 202, in consume
status = self._consume(redis_hash)
File "/usr/src/app/redis_consumer/consumers/multiplex_consumer.py", line 124, in _consume
image = self.predict(image, model_name, model_version)
File "/usr/src/app/redis_consumer/consumers/base_consumer.py", line 614, in predict
model_dtype, untile=untile)
File "/usr/src/app/redis_consumer/consumers/base_consumer.py", line 464, in _predict_big_image
in_tensor_dtype=model_dtype)
File "/usr/src/app/redis_consumer/consumers/base_consumer.py", line 311, in grpc_image
prediction = client.predict(req_data, settings.GRPC_TIMEOUT)
File "/usr/src/app/redis_consumer/grpc_clients.py", line 185, in predict
response = self._retry_grpc(request, request_timeout)
File "/usr/src/app/redis_consumer/grpc_clients.py", line 134, in _retry_grpc
raise err
File "/usr/src/app/redis_consumer/grpc_clients.py", line 123, in _retry_grpc
response = api_call(request, timeout=request_timeout)
File "/usr/local/lib/python3.6/site-packages/grpc/_channel.py", line 826, in __call__
return _end_unary_response_blocking(state, call, False, None)
File "/usr/local/lib/python3.6/site-packages/grpc/_channel.py", line 729, in _end_unary_response_blocking
raise _InactiveRpcError(state)
grpc._channel._InactiveRpcError: <_InactiveRpcError of RPC that terminated with:
status = StatusCode.DEADLINE_EXCEEDED
details = "Deadline Exceeded"
debug_error_string = "{"created":"@1609789741.491197858","description":"Error received from peer ipv4:10.43.253.0:8500","file":"src/core/lib/surface/call.cc","file_line":1056,"grpc_message":"Deadline Exceeded","grpc_status":4}"

When this type of error happens, some of the files are processed, and others are not, so one possible fix for this type of error would be to recursively upload remaining files until all the expected files exist, or to just batch from the start with smaller zip files.

The only thing with the batching, is that this error doesn't seem to be directly 'size' related, so much as it's 'difficulty-of-segmentation' related, which means there's no obvious batching size/number.

That being said, if deepcell fixes or extends the redis timeouts, then this isn't really an issue, since the uploading itself works fine

ngreenwald · 2021-01-04T21:44:33Z

What do you mean it's difficulty of segmentation related?

ackagel · 2021-01-04T21:49:54Z

I suppose that's more of a guess, but basically, on Tyler's DCIS data (51MB) less than half of the images were processed, while on the duplicated 'fov8.tif' template (183MB), more than 3/4ths of the images were processed. Could be something else, but it's more of a problem of how long it takes the servers to process each image. We could ask Will if there's any reason not to bump up the default redis timeout (settings.GRPC_TIMEOUT), since that would fix the problem. Looks like it's currently set to 30s

ackagel · 2021-01-04T21:50:34Z

https://github.com/vanvalenlab/kiosk-redis-consumer/blob/cefc4736d1658505966bd8bbe3638a67b83290f2/redis_consumer/settings.py#L67

ngreenwald · 2021-01-04T22:16:28Z

Yes, will you copy the error message and open an issue in the kiosk-redis-consumer repo

alex-l-kong

Not much to add on top of Noah. Looks good so far!

ackagel · 2021-01-09T00:06:35Z

@ngreenwald same pytest/testbook bug on this PR. Other than that, it's good for review.

ngreenwald

Just one commented line left. Did the timeout issue get resolved with Redis?

ngreenwald · 2021-01-11T17:30:12Z

ark/utils/deepcell_service_utils.py

@@ -86,7 +94,8 @@ def create_deepcell_output(deepcell_input_dir, deepcell_output_dir, fovs=None,

    # pass the zip file to deepcell.org
    print('Uploading files to DeepCell server.')
-    run_deepcell_task(zip_path, deepcell_output_dir, host, job_type, scale)
+    # run_deepcell_task(zip_path, deepcell_output_dir, host, job_type, scale)


Can remove the commented out line

ackagel · 2021-01-11T19:12:13Z

not sure if the timeout has been increased yet, but I'll test it today

…n-picklability)

ngreenwald

Looks good, just some simple tests for new function.

If you could also test it yourself actually uploading to deepcell.org with real data to make sure when you supply bad data, the right error message is raised.

ngreenwald · 2021-01-23T00:13:01Z

ark/utils/deepcell_service_utils_test.py

@@ -7,7 +7,7 @@
 from ark.utils.deepcell_service_utils import create_deepcell_output


-def mocked_run_deepcell(input_dir, output_dir, host, job_type, scale):
+def mocked_run_deepcell(input_dir, output_dir, host, job_type, scale, timeout):


Can add some simple checks for _zip_run_extract
e.g. for 210 images, make sure first zip has 100, second zip has 100, third zip has 10.
Make sure first zip contains first 100 images.
Make sure changing zip_size actually changes number of images included.

Can change the default to be like 5 or 2 or something to make previous tests faster.

alex-l-kong

Nothing else to add on top of Noah's comments.

…rflow' case

ackagel · 2021-01-26T02:25:18Z

looks like I'm getting the 'correct' error messages from deepcell for bad data

ngreenwald

Everything looks good. I'm fine to merge in the PR with reduced coverage since it's mostly error messages from Deepcell.org that we're catching. The one thing that isn't tested which I think is worth testing is the parallel execution option.

In addition to adding a test, can you also confirm locally that uploading 3 zips of 50 FOVs each simultaneously doesn't lead to any issues with Deepcell.org?

ngreenwald

Nice, almost there

ngreenwald · 2021-01-27T00:03:28Z

ark/utils/deepcell_service_utils_test.py

+        assert os.path.exists(os.path.join(output_dir, 'example_output_2.zip'))
+
+        os.remove(os.path.join(output_dir, 'fov1_feature_0.tif'))
+        os.remove(os.path.join(output_dir, 'fov2_feature_0.tif'))


This would be cleaner if we made a new temp folder, rather than deleting the files we've already created. Can either make output1, output2, output3 for each part of the test, or a context manager with a temp dir

…t tests

ngreenwald · 2021-01-27T02:53:14Z

ark/utils/deepcell_service_utils_test.py

-        with pytest.warns(UserWarning):
-            create_deepcell_output(deepcell_input_dir=input_dir, deepcell_output_dir=output_dir,
-                                   fovs=['fov1'])
+            pathlib.Path(os.path.join(input_dir, 'fov4.tif')).touch()


Is there a specific warning that we're making sure is raised here, or is this just so that we now have 4 TIFs for the batch size checks that come next?

the last pytest.warns(UserWarning) is redundantly checking warnings for 'overwriting file'; definitely could be removed if needed. The extra fov is created for the batch size checking later.

… test

* adds alternate deepcell call * adds docstring to new deepcell api caller * redirects mock in deepcell service testing * adds argument to deepcell service mocker * documentation fix * sanitizes progressbar updates * upgrades tqdm to notebook version * testing readout from redis responce * decodes url error text * allows failures and better failure printing * sends expire signal to redis (out of kindness to deepcell's servers) * extends timeout on file upload * empty commit for travis testing * removes commented function call * removes kiosk-client deepcell upload function * distributes large zips for deepcell into smaller batches * fixes error with range-int mult * fixes zip auto-batching and connects zip_size argument * parallelizes deepcell uploads * changes parallelization strategy from processes to threads (due to non-picklability) * makes parallelization optional * converts map object to list to run processes * removes kiosk-client requirement * adds tests to validate zip batching, and corrects batching in 'no-overflow' case * adds test for parallel uploads * prevents race condition on file extraction * replaces manual file deletion with context management in deepcell unit tests * removes redundant warning context wrapper in deepcell upload batching test

ackagel added 11 commits December 12, 2020 01:12

adds alternate deepcell call

a6c2bfc

adds docstring to new deepcell api caller

05d695c

redirects mock in deepcell service testing

0ad2f8b

adds argument to deepcell service mocker

0eb6765

documentation fix

76dbc04

sanitizes progressbar updates

4e6d143

upgrades tqdm to notebook version

67d7278

testing readout from redis responce

c33675b

decodes url error text

9b862a8

allows failures and better failure printing

0142b3a

sends expire signal to redis (out of kindness to deepcell's servers)

c8a889b

ackagel requested review from ngreenwald and alex-l-kong January 4, 2021 19:53

extends timeout on file upload

c6f4220

alex-l-kong reviewed Jan 6, 2021

View reviewed changes

empty commit for travis testing

d0100a4

ngreenwald requested changes Jan 11, 2021

View reviewed changes

ackagel added 6 commits January 12, 2021 19:50

merge master

766de18

removes commented function call

f8300aa

removes kiosk-client deepcell upload function

1ff4505

distributes large zips for deepcell into smaller batches

6f4eb21

fixes error with range-int mult

9018e3b

fixes zip auto-batching and connects zip_size argument

7a6951e

ackagel added 5 commits January 22, 2021 20:14

parallelizes deepcell uploads

d919ef6

changes parallelization strategy from processes to threads (due to no…

573b229

…n-picklability)

makes parallelization optional

041eaa6

converts map object to list to run processes

b738fe7

removes kiosk-client requirement

feb9467

ackagel requested review from ngreenwald and alex-l-kong January 22, 2021 23:42

ngreenwald requested changes Jan 23, 2021

View reviewed changes

alex-l-kong reviewed Jan 23, 2021

View reviewed changes

adds tests to validate zip batching, and corrects batching in 'no-ove…

131ae61

…rflow' case

ackagel requested review from ngreenwald and alex-l-kong January 26, 2021 02:25

ngreenwald requested changes Jan 26, 2021

View reviewed changes

ackagel added 2 commits January 26, 2021 21:01

adds test for parallel uploads

fa9c44f

prevents race condition on file extraction

c9246ea

ackagel requested a review from ngreenwald January 26, 2021 22:03

ngreenwald requested changes Jan 27, 2021

View reviewed changes

replaces manual file deletion with context management in deepcell uni…

066b230

…t tests

ackagel requested a review from ngreenwald January 27, 2021 02:10

ngreenwald reviewed Jan 27, 2021

View reviewed changes

removes redundant warning context wrapper in deepcell upload batching…

bd6c347

… test

ackagel requested a review from ngreenwald January 27, 2021 21:21

ngreenwald approved these changes Jan 27, 2021

View reviewed changes

ngreenwald merged commit 0c9c5ba into master Jan 27, 2021

ngreenwald deleted the auto-upload-patch branch January 27, 2021 22:21

ackagel mentioned this pull request Feb 8, 2021

adds requests dependency #380

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto upload patch #369

Auto upload patch #369

ackagel commented Dec 12, 2020

ackagel commented Jan 4, 2021

ngreenwald commented Jan 4, 2021

ackagel commented Jan 4, 2021

ackagel commented Jan 4, 2021

ngreenwald commented Jan 4, 2021

alex-l-kong left a comment •

edited

Loading

ackagel commented Jan 9, 2021

ngreenwald left a comment

ngreenwald Jan 11, 2021

ackagel commented Jan 11, 2021

ngreenwald left a comment

ngreenwald Jan 23, 2021

alex-l-kong left a comment

ackagel commented Jan 26, 2021

ngreenwald left a comment

ngreenwald left a comment

ngreenwald Jan 27, 2021

ngreenwald Jan 27, 2021

ackagel Jan 27, 2021

Auto upload patch #369

Auto upload patch #369

Conversation

ackagel commented Dec 12, 2020

Purpose

Implementation

Remaining issues

ackagel commented Jan 4, 2021

ngreenwald commented Jan 4, 2021

ackagel commented Jan 4, 2021

ackagel commented Jan 4, 2021

ngreenwald commented Jan 4, 2021

alex-l-kong left a comment • edited Loading

Choose a reason for hiding this comment

ackagel commented Jan 9, 2021

ngreenwald left a comment

Choose a reason for hiding this comment

ngreenwald Jan 11, 2021

Choose a reason for hiding this comment

ackagel commented Jan 11, 2021

ngreenwald left a comment

Choose a reason for hiding this comment

ngreenwald Jan 23, 2021

Choose a reason for hiding this comment

alex-l-kong left a comment

Choose a reason for hiding this comment

ackagel commented Jan 26, 2021

ngreenwald left a comment

Choose a reason for hiding this comment

ngreenwald left a comment

Choose a reason for hiding this comment

ngreenwald Jan 27, 2021

Choose a reason for hiding this comment

ngreenwald Jan 27, 2021

Choose a reason for hiding this comment

ackagel Jan 27, 2021

Choose a reason for hiding this comment

alex-l-kong left a comment •

edited

Loading