Introduce Runnable #1163

ttung · 2019-04-11T07:37:15Z

Runnable is the base unit of a recipe. Each runnable constitutes a PipelineComponent with a specific algorithm. Constructor arguments that are FileProviders are loaded into memory and passed in. Inputs to the run() method can also include FileProviders. In all cases, FileProviders are associated with a file path or url. The type is inferred from the typing parameters of the method (currently supported: ImageStack, IntensityTable, ExpressionMatrix, and Codebook).

Runnables will be wired together to constitute a pipeline recipe.

Test plan: Added tests to verify a simple Runnable, chained Runnables, and Runnables that has constructor arguments that are FileProviders.

This is a continuation of #1097

codecov-io · 2019-04-11T07:52:30Z

Codecov Report

Merging #1163 into master will increase coverage by 0.13%.
The diff coverage is 92.76%.

@@            Coverage Diff             @@
##           master    #1163      +/-   ##
==========================================
+ Coverage   88.42%   88.55%   +0.13%     
==========================================
  Files         133      137       +4     
  Lines        4923     5075     +152     
==========================================
+ Hits         4353     4494     +141     
- Misses        570      581      +11

Impacted Files	Coverage Δ
starfish/recipe/errors.py	`100% <100%> (ø)`
starfish/recipe/__init__.py	`100% <100%> (ø)`
starfish/recipe/filesystem.py	`87.75% <87.75%> (ø)`
starfish/recipe/runnable.py	`94.44% <94.44%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 051f77c...389da68. Read the comment docs.

ambrosejcarr

Looks ok to me. Would like some usage docs for this concept and a pointer to the tip of your tree so that I can test the functionality.

Would like @shanaxel42 or @kne42 to take a close look at the code.

ambrosejcarr · 2019-04-14T22:15:45Z

starfish/recipe/filesystem.py

+    The save method is expected to be called with the object and a string, which is the path to
+    write the object to.
+    """
+    IMAGESTACK = (ImageStack, imagestack_convert, ImageStack.export)


Scope creep: I feel the save/load names for our methods should be consistent across the 4 objects we have. What do you think?

#1097 (comment)

LOL you and @shanaxel42 think alike. :)

ambrosejcarr · 2019-04-14T22:22:47Z

starfish/recipe/filesystem.py

+
+
+class FileProvider:
+    """This is used to wrap paths or URLs that are passed into Runnables via the `file_inputs` magic


file_inputs is not used anywhere in this PR, just mentioned in docs. Confused.

it's used in a separate PR. :)

ttung

Looks ok to me. Would like some usage docs for this concept and a pointer to the tip of your tree so that I can test the functionality.

check out the branch tonytung-recipe-cli

ttung · 2019-04-15T03:59:51Z

starfish/recipe/filesystem.py

+
+
+class FileProvider:
+    """This is used to wrap paths or URLs that are passed into Runnables via the `file_inputs` magic


it's used in a separate PR. :)

ttung · 2019-04-15T04:00:41Z

starfish/recipe/filesystem.py

+    The save method is expected to be called with the object and a string, which is the path to
+    write the object to.
+    """
+    IMAGESTACK = (ImageStack, imagestack_convert, ImageStack.export)


shanaxel42

I think just some more comments in the tests/runnable class explaining stuff would be helpful!

starfish/recipe/runnable.py

shanaxel42 · 2019-04-15T20:38:26Z

starfish/recipe/test/test_runnable.py

+    with pytest.raises(ConstructorError):
+        Runnable(
+            "filter", "SimpleFilterAlgorithm",
+            FileProvider(URL),


why does this error? because we don't provide params?

Yes. Updated the documentation.

shanaxel42 · 2019-04-15T20:39:34Z

starfish/recipe/test/test_runnable.py

+
+
+def test_run_type_inference_error():
+    """Verify that we get a properly typed error when we cannot properly infer the type for one of


Could use more explanation here too, maybe some comments about what input causes the error

Fixed, and added more commentary on how to fix this error in the documentation for TypeInferenceError.

ttung · 2019-04-18T19:53:13Z

Restacked on top of #1218

ambrosejcarr

Looks good enough to merge. Should we recipe -> _recipe? I am guessing users won't call this from the API.

I may have follow-up comments when I try it out more thoroughly.

ambrosejcarr · 2019-04-22T00:17:59Z

starfish/recipe/filesystem.py

+)
+
+
+def imagestack_convert(indirect_path_or_url: str) -> ImageStack:


single-line docstring please: what is this intended to do?

ambrosejcarr · 2019-04-22T00:18:04Z

starfish/recipe/filesystem.py

+    )
+
+
+def codebook_convert(indirect_path_or_url: str) -> Codebook:


single-line docstring please: what is this intended to do?

Runnable is the base unit of a recipe. Each runnable constitutes a PipelineComponent with a specific algorithm. Constructor arguments that are FileProviders are loaded into memory and passed in. Inputs to the `run()` method can also include FileProviders. In all cases, FileProviders are associated with a file path or url. The type is inferred from the typing parameters of the method (currently supported: ImageStack, IntensityTable, ExpressionMatrix, and Codebook). Runnables will be wired together to constitute a pipeline recipe. Test plan: Added tests to verify a simple Runnable, chained Runnables, and Runnables that has constructor arguments that are FileProviders. Depends on spacetx#1095

ttung requested review from ambrosejcarr, kne42 and shanaxel42 April 11, 2019 07:38

ambrosejcarr reviewed Apr 14, 2019

View reviewed changes

ttung commented Apr 15, 2019

View reviewed changes

shanaxel42 requested changes Apr 15, 2019

View reviewed changes

ttung mentioned this pull request Apr 16, 2019

Recipe and recipe execution #1192

Merged

ttung requested a review from shanaxel42 April 17, 2019 02:33

ttung force-pushed the tonytung-runnable branch from 4cb0f6a to e7afa06 Compare April 18, 2019 19:52

ttung changed the base branch from master to tonytung-normalize April 18, 2019 19:52

ttung force-pushed the tonytung-normalize branch 5 times, most recently from 057649b to 8616308 Compare April 19, 2019 16:10

ttung changed the base branch from tonytung-normalize to master April 19, 2019 16:35

ttung force-pushed the tonytung-runnable branch from e7afa06 to 389da68 Compare April 19, 2019 16:37

ambrosejcarr approved these changes Apr 22, 2019

View reviewed changes

shanaxel42 approved these changes Apr 22, 2019

View reviewed changes

ttung force-pushed the tonytung-runnable branch from 389da68 to 0ef68e4 Compare April 22, 2019 19:28

ttung merged commit 36d08a3 into spacetx:master Apr 22, 2019

ttung deleted the tonytung-runnable branch April 22, 2019 19:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Runnable #1163

Introduce Runnable #1163

ttung commented Apr 11, 2019

codecov-io commented Apr 11, 2019 •

edited

Loading

ambrosejcarr left a comment

ambrosejcarr Apr 14, 2019

ttung Apr 15, 2019

ttung Apr 15, 2019

ambrosejcarr Apr 14, 2019

ttung Apr 15, 2019

ttung left a comment

ttung Apr 15, 2019

ttung Apr 15, 2019

shanaxel42 left a comment

shanaxel42 Apr 15, 2019

ttung Apr 16, 2019

shanaxel42 Apr 15, 2019

ttung Apr 16, 2019

ttung commented Apr 18, 2019

ambrosejcarr left a comment

ambrosejcarr Apr 22, 2019

ttung Apr 22, 2019

ambrosejcarr Apr 22, 2019

ttung Apr 22, 2019



		class FileProvider:
		"""This is used to wrap paths or URLs that are passed into Runnables via the `file_inputs` magic



		def test_run_type_inference_error():
		"""Verify that we get a properly typed error when we cannot properly infer the type for one of

		)


		def imagestack_convert(indirect_path_or_url: str) -> ImageStack:

		)


		def codebook_convert(indirect_path_or_url: str) -> Codebook:

Introduce Runnable #1163

Introduce Runnable #1163

Conversation

ttung commented Apr 11, 2019

codecov-io commented Apr 11, 2019 • edited Loading

Codecov Report

ambrosejcarr 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

ttung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shanaxel42 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

ttung commented Apr 18, 2019

ambrosejcarr 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

codecov-io commented Apr 11, 2019 •

edited

Loading