Scottx611x/provide vis tools with url to their input data #2677

scottx611x · 2018-03-18T21:36:15Z

Introduce detail route to /api/v2/tools/ that returns a launched Tools input data as JSON. This approach (sending a url pointing to a Tool's input data) will be used moving forward for its ease of use w/ a 2nd host running docker. Before, we noticed that there were underlying ssh commands in django_docker_engine to create directories on said host to place a launched VisualizationTool's input data, and ssh key management between the Refinery EC2 and Docker EC2 was troublesome to automate. This will also prove reusable if we ever need to move to something like ECS or Fargate.

TODO:

Update existing VisualizationTools to read input data from a url
- refinery-developer-vis-tool has been updated to do this so far

Note: This shouldn't be merged until we have the 2nd host for docker up and running w/ terraform. See #2597 EDIT: We have the 2nd host working more or less and are just making some final adjustments

…_vis_tools_with_url_to_their_input_data

…data as JSON

…rovides a nice "Please Wait" page, so this seems like a natural move

…e removed on django_docker_engine side

codecov · 2018-03-18T21:51:08Z

Codecov Report

Merging #2677 into develop will decrease coverage by 2.37%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #2677      +/-   ##
===========================================
- Coverage    57.84%   55.47%   -2.38%     
===========================================
  Files          399      399              
  Lines        26756    25278    -1478     
  Branches      1239     1239              
===========================================
- Hits         15477    14022    -1455     
+ Misses       11279    11256      -23

Impacted Files	Coverage Δ
refinery/tool_manager/tasks.py	`100% <100%> (ø)`	⬆️
refinery/tool_manager/models.py	`96.61% <100%> (-0.8%)`	⬇️
refinery/tool_manager/views.py	`96.22% <100%> (-3.21%)`	⬇️
refinery/tool_manager/tests.py	`99.81% <100%> (-0.05%)`	⬇️
refinery/tool_manager/utils.py	`100% <100%> (ø)`	⬆️
refinery/analysis_manager/tests.py	`97.57% <0%> (-0.57%)`	⬇️
... and 2 more

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 9fb0601...8faf73f. Read the comment docs.

scottx611x · 2018-03-19T13:45:46Z

refinery/tool_manager/models.py

+            image_name=self.tool_definition.image_name,
+            container_name=self.container_name,
+            labels={self.uuid: ToolDefinition.VISUALIZATION},
+            extra_directories=self.tool_definition.get_extra_directories()


Do we still need this? (extra_directories) @mccalluc

@scottx611x : Yes: This is how HiGlass gets a directory for logs that is in the host FS, instead of the limited-capacity union FS.

scottx611x · 2018-03-19T13:46:15Z

refinery/tool_manager/models.py

-
-        self._django_docker_client.run(container)
-
+        start_container.delay(self)


Comment here about async

… we provide this info as a url

mccalluc

I think it's fine, but I'd want to be sure the async code is not introducing race conditions... Probably fine?

mccalluc · 2018-03-19T15:05:17Z

refinery/tool_manager/models.py

-                                                 HistoryDatasetElement)
+from bioblend.galaxy.dataset_collections import (
+    CollectionDescription, CollectionElement, HistoryDatasetElement
+)


Did you find the setting to tweak in isort?

Yeah! In an earlier PR: see multi_line_output

mccalluc · 2018-03-19T15:12:56Z

refinery/tool_manager/models.py

-
+        # Pulls docker image if it doesn't exist yet, and launches container
+        # asynchronously
+        start_container.delay(self)
        return JsonResponse({Tool.TOOL_URL: self.get_relative_container_url()})


Was it synchronous before? I worry a little about race conditions... are we sure it will be able to return a container_url no matter where in the launch sequence it is?

It was before, but now that we have the nice please wait page I think this is okay.
A VisualizationTool will always be able to return its get_relative_container_url() regardless of the container state.

Without this, one could hit a state where a large docker image hasn't be downloaded yet, and upon clicking launch the page would seemingly be frozen, while the pending request waits for the image download to complete.

mccalluc · 2018-03-19T15:17:38Z

refinery/tool_manager/tests.py

@@ -309,6 +308,7 @@ def create_solr_mock_response(self, nodes):
            }
        )

+    @override_settings(CELERY_ALWAYS_EAGER=True)


It makes me worry a little if the tests depend on Celery...? Particularly since the actual run seems to be mocked.

This setting runs Celery on a single thread so that we don't have to worry about sleeping/waiting after launching a Tool in these tests. I could probably mock the underlying delay() call but this seemed easier.

mccalluc · 2018-03-19T15:19:28Z

refinery/tool_manager/tests.py

-        self.assertEqual(get_response.status_code, 400)
-        self.assertIn("Couldn't retrieve VisualizationTool",
-                      get_response.content)
+        self.assertEqual(get_response.status_code, 404)


I think the underlying error message should still be coming through... but if a 404 doesn't seem appropriate, let me know?

Now that we use django.shortcuts.get_object_or_404 a resonable error message is returned. Something like No VisualizationTool matches the given query. I was hesitant to test Django internals

mccalluc · 2018-03-19T15:20:46Z

refinery/tool_manager/views.py

@@ -180,7 +182,7 @@ class AutoRelaunchProxy(Proxy, object):
    """
    def __init__(self):
        super(AutoRelaunchProxy, self).__init__(
-            settings.DJANGO_DOCKER_ENGINE_DATA_DIR,
+            None,  # TODO: django_docker_engine code should remove need for arg


I'll look into this. Thanks!

scottx611x added 14 commits March 16, 2018 10:44

Merge remote-tracking branch 'origin/develop' into scottx611x/provide…

4e5f976

…_vis_tools_with_url_to_their_input_data

Bump django_docker_engine req.

7a49761

Remove temp assignment to client

6a305e5

Update DockerClientWrapper init; No slot for data_dir arg now

7715667

Provide detail route: container_input_data to fetch a Tool's input …

1ba18a4

…data as JSON

Add test coverage for container input data detail route

0b68a01

Use already available instance of DockerClientWrapper

bad4259

Make _django_docker_client public

d17a3b2

Launch VisualizationTools asynchronously; The AutoRelaunchProxy now p…

5775bc4

…rovides a nice "Please Wait" page, so this seems like a natural move

Update tests that depended on VisualizationTool.launch being synchronous

1295241

Fix out of date django_docker_engine specifics in tests

bb8f920

Fix test after method was renamed

f66d152

Add reference to container_input_data detail view

e202e3d

Use None for now to satisfy django_docker_engine arg; This should b…

78acea1

…e removed on django_docker_engine side

scottx611x added 6 commits March 18, 2018 18:15

Return 404s where appropriate in ToolsViewSet

e8b76e3

Update tests to assert the newly added 404s

dfaea12

Fix typo

b066ef6

Fix docstring

08aed80

Fit this on one line

f824ed2

Fit this on one line as well

ff8d032

scottx611x commented Mar 19, 2018

View reviewed changes

Add comment about running containers asynchronously

8b49e7b

scottx611x requested a review from mccalluc March 19, 2018 14:37

Remove code related to ToolDefinition.container_input_path now that…

1d90b02

… we provide this info as a url

mccalluc reviewed Mar 19, 2018

View reviewed changes

scottx611x added 2 commits March 19, 2018 15:31

Bump django_docker_engine req

2100fab

Remove need for temporary arg

8faf73f

This was referenced Mar 19, 2018

Scottx611x/read input from url refinery-platform/refinery-higlass-docker#7

Merged

Scottx611x/ec2 for docker #2684

Closed

mccalluc mentioned this pull request Mar 21, 2018

Mccalluc/terraform ec2 for docker engine #2597

Merged

scottx611x merged commit 1fbaf2c into develop Apr 13, 2018

scottx611x deleted the scottx611x/provide_vis_tools_with_url_to_their_input_data branch April 13, 2018 19:29

scottx611x mentioned this pull request Apr 16, 2018

Scottx611x/read input from url refinery-platform/docker_igv_js#20

Merged

scottx611x added this to the Release 1.6.4 milestone Apr 30, 2018

scottx611x self-assigned this Apr 30, 2018

scottx611x added fix-2x-small tools server labels Apr 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scottx611x/provide vis tools with url to their input data #2677

Scottx611x/provide vis tools with url to their input data #2677

scottx611x commented Mar 18, 2018 •

edited

codecov bot commented Mar 18, 2018 •

edited

scottx611x Mar 19, 2018 •

edited

mccalluc Mar 19, 2018

scottx611x Mar 19, 2018

mccalluc left a comment

mccalluc Mar 19, 2018

scottx611x Mar 19, 2018

mccalluc Mar 19, 2018

scottx611x Mar 19, 2018 •

edited

mccalluc Mar 19, 2018

scottx611x Mar 19, 2018 •

edited

mccalluc Mar 19, 2018

scottx611x Mar 19, 2018

mccalluc Mar 19, 2018


		self._django_docker_client.run(container)

		start_container.delay(self)

Scottx611x/provide vis tools with url to their input data #2677

Scottx611x/provide vis tools with url to their input data #2677

Conversation

scottx611x commented Mar 18, 2018 • edited

codecov bot commented Mar 18, 2018 • edited

Codecov Report

scottx611x Mar 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mccalluc 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

scottx611x Mar 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottx611x Mar 19, 2018 • edited

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

scottx611x commented Mar 18, 2018 •

edited

codecov bot commented Mar 18, 2018 •

edited

scottx611x Mar 19, 2018 •

edited

scottx611x Mar 19, 2018 •

edited

scottx611x Mar 19, 2018 •

edited