Migrate to docker sdk #96

cjh1 · 2017-01-31T18:06:50Z

Finally got this to behaving on Travis ( real bug, so was worth persevering ).

This PR moves to docker-py for all docker interactions. It refactors run_process(...) to break out the select loops so it can be used with a process or with sockets returned by docker-py when running a container. I think this was the right approach, but open to other ideas.

@zachmullen @manthey I would appreciate your reviews/input. I would also like some testing using a real world application that is using the docker plugin, particularly using named pipes as this has all changed, before we push any buttons.

Implement remove on client side

cjh1 · 2017-04-18T20:48:03Z

girder_worker/plugins/docker/tests/docker_integration_test.py

@@ -135,7 +135,7 @@ def testDockerModeStdio(self):
        sys.stdout = _old

        lines = stdout_captor.getvalue().splitlines()
-        message = '%s\n' % self._test_message
+        message = '%s\r\n' % self._test_message


Are we OK with the extra \r?

cjh1 · 2017-04-28T12:42:48Z

@zachmullen I have added support for add extra docker run args. I am using the same property docker_run_args, but its now a dict, that can contain one of these parameters

zachmullen · 2017-04-28T13:27:22Z

Great!

zachmullen · 2017-04-28T18:30:23Z

I am finally getting around to testing this out. I am getting the following error when running a docker task:

AttributeError: 'module' object has no attribute 'get_config_header'
  File "/usr/local/lib/python2.7/site-packages/celery/app/trace.py", line 367, in trace_task
    R = retval = fun(*args, **kwargs)
  File "/usr/local/lib/python2.7/site-packages/celery/app/trace.py", line 622, in __protected_call__
    return self.run(*args, **kwargs)
  File "/Users/zach/dev/girder_worker/girder_worker/tasks.py", line 17, in run
    return core.run(*pargs, **kwargs)
  File "/Users/zach/dev/girder_worker/girder_worker/core/utils.py", line 123, in wrapped
    return fn(*args, **kwargs)
  File "/Users/zach/dev/girder_worker/girder_worker/core/__init__.py", line 319, in run
    auto_convert=auto_convert, validate=validate, **kwargs)
  File "/Users/zach/dev/girder_worker/girder_worker/plugins/docker/executor.py", line 221, in run
    _pull_image(image)
  File "/Users/zach/dev/girder_worker/girder_worker/plugins/docker/executor.py", line 21, in _pull_image
    client.images.pull(image)
  File "/usr/local/lib/python2.7/site-packages/docker/models/images.py", line 259, in pull
    self.client.api.pull(name, **kwargs)
  File "/usr/local/lib/python2.7/site-packages/docker/api/image.py", line 358, in pull
    header = auth.get_config_header(self, registry)

Here's the job kwargs:

{
  "auto_convert": false,
  "cleanup": true,
  "inputs": {
    "--InputImage": {
      "api_url": "http://localhost:8080/api/v1",
      "fetch_parent": false,
      "format": "none",
      "id": "584ef0bdc2231b5048016c35",
      "mode": "girder",
      "name": "RoughDetectorTest.nrrd",
      "resource_type": "item",
      "token": "6lCvnknmA9dAhMxsU9ascw7BNkJZDxp5Rgr96vGEJ6qjbfM7CUPYPoGJ9I69STFj",
      "type": "string"
    },
    "--MaximumLineStraightnessDeviation": {
      "data": 1.2,
      "mode": "inline"
    },
    "--MaximumRadius": {
      "data": 20,
      "mode": "inline"
    },
    "--MaximumSphereDistance": {
      "data": 42,
      "mode": "inline"
    },
    "--MinimumRadius": {
      "data": 2,
      "mode": "inline"
    },
    "--MinimumSphereActivity": {
      "data": 5002,
      "mode": "inline"
    },
    "--MinimumSphereDistance": {
      "data": 32,
      "mode": "inline"
    },
    "--SpheresPerPhantom": {
      "data": 3,
      "mode": "inline"
    },
    "--StrictSorting": {
      "data": false,
      "mode": "inline"
    }
  },
  "jobInfo": {
    "headers": {
      "Girder-Token": "MOPpRLJ2dRkeKnhTMdqJJe5qI91oRORmKdrBeicmb5AV4hydYg7MFOQTDVvN6s98"
    },
    "logPrint": true,
    "method": "PUT",
    "reference": "590389f0d527a35adb181c9e",
    "url": "http://localhost:8080/api/v1/job/590389f0d527a35adb181c9e"
  },
  "outputs": {
    "--DetectedPoints": {
      "api_url": "http://localhost:8080/api/v1",
      "format": "none",
      "mode": "girder",
      "name": "out2.txt",
      "parent_id": "584ef0abc2231b5048016c33",
      "parent_type": "folder",
      "reference": "{\"type\": \"item_tasks.output\", \"id\": \"--DetectedPoints\", \"jobId\": \"590389f0d527a35adb181c9e\"}",
      "token": "6lCvnknmA9dAhMxsU9ascw7BNkJZDxp5Rgr96vGEJ6qjbfM7CUPYPoGJ9I69STFj",
      "type": "string"
    }
  },
  "task": {
    "container_args": [
      "PETPhantomDetectorCLI",
      "--InputImage",
      "$input{--InputImage}",
      "--MaximumLineStraightnessDeviation",
      "$input{--MaximumLineStraightnessDeviation}",
      "--MaximumRadius",
      "$input{--MaximumRadius}",
      "--MaximumSphereDistance",
      "$input{--MaximumSphereDistance}",
      "--MinimumRadius",
      "$input{--MinimumRadius}",
      "--MinimumSphereActivity",
      "$input{--MinimumSphereActivity}",
      "--MinimumSphereDistance",
      "$input{--MinimumSphereDistance}",
      "--SpheresPerPhantom",
      "$input{--SpheresPerPhantom}",
      "$flag{--StrictSorting}",
      "--DetectedPoints",
      "/mnt/girder_worker/data/--DetectedPoints"
    ],
    "docker_image": "dzenanz/petct:v6",
    "inputs": [
      {
        "description": "Input image to be analysed.",
        "format": "file",
        "id": "--InputImage",
        "name": "InputImage",
        "target": "filepath",
        "type": "file"
      },
      {
        "default": {
          "data": 1
        },
        "description": "Used for eliminating detections which are not in a straight line. Unit: multiples of geometric average of voxel spacing",
        "format": "number",
        "id": "--MaximumLineStraightnessDeviation",
        "name": "MaximumLineStraightnessDeviation",
        "type": "number"
      },
      {
        "default": {
          "data": 20
        },
        "description": "Used for eliminating too big blobs. Unit: millimeter [mm]",
        "format": "number",
        "id": "--MaximumRadius",
        "name": "MaximumRadius",
        "type": "number"
      },
      {
        "default": {
          "data": 40
        },
        "description": "Signifies maximum distance between adjacent sphere centers [mm]. Used to separate phantoms from tumors.",
        "format": "number",
        "id": "--MaximumSphereDistance",
        "name": "MaximumSphereDistance",
        "type": "number"
      },
      {
        "default": {
          "data": 3
        },
        "description": "Used for eliminating too small blobs. Unit: millimeter [mm]",
        "format": "number",
        "id": "--MinimumRadius",
        "name": "MinimumRadius",
        "type": "number"
      },
      {
        "default": {
          "data": 5000
        },
        "description": "Used for thresholding in blob detection. Unit: becquerels per milliliter [Bq/ml]",
        "format": "number",
        "id": "--MinimumSphereActivity",
        "name": "MinimumSphereActivity",
        "type": "number"
      },
      {
        "default": {
          "data": 30
        },
        "description": "Signifies minimum distance between adjacent sphere centers [mm]. Used to separate phantoms from tumors.",
        "format": "number",
        "id": "--MinimumSphereDistance",
        "name": "MinimumSphereDistance",
        "type": "number"
      },
      {
        "default": {
          "data": 3
        },
        "description": "What kind of phantom are we working with here?",
        "format": "integer",
        "id": "--SpheresPerPhantom",
        "name": "SpheresPerPhantom",
        "type": "integer"
      },
      {
        "default": {
          "data": false
        },
        "description": "Controls whether spheres within a phantom must have descending activities. If OFF, they can have approximately same activities (within 15%).",
        "format": "boolean",
        "id": "--StrictSorting",
        "name": "StrictSorting",
        "type": "boolean"
      }
    ],
    "mode": "docker",
    "outputs": [
      {
        "description": "Fiducual points, one for each detected sphere. Will be multiple of 3.",
        "format": "new-file",
        "id": "--DetectedPoints",
        "name": "DetectedPoints",
        "target": "filepath",
        "type": "new-file"
      }
    ]
  },
  "validate": false
}

cjh1 · 2017-04-28T18:39:59Z

Is this in a virtualenv? This issue seems similar ...

zachmullen · 2017-04-28T18:42:16Z

No, installed at system level. I'll try it in a VE so it's a more fresh environment.

zachmullen · 2017-04-28T18:43:25Z

requirements.txt

@@ -7,3 +7,4 @@ pytz==2016.4
 requests[security]==2.10.0
 six==1.10.0
 wsgiref==0.1.2
+docker==2.1.0


We have per-plugin requirements files that can be installed as extras, so this should go in plugins/docker/requirements.txt.

Sure, not sure why it ended up here

zachmullen · 2017-04-28T18:44:38Z

Reinstalling in a virtualenv worked 👍

cjh1 · 2017-04-28T18:47:21Z

From the look of that issue, it might be related to a naming conflict with docker-compose, did you have that installed?

zachmullen · 2017-04-28T18:49:48Z

girder_worker/plugins/docker/__init__.py

+            client.containers.run('busybox', args, **config)
+        except DockerException as dex:
+            logger.error('Error setting perms on docker tempdir %s.' % tmpdir)
+            logger.error(dex)


I think logger.exception() will automatically log the most recently raised exception.

zachmullen · 2017-04-28T18:50:05Z

girder_worker/plugins/docker/executor.py

+        client.images.pull(image)
+    except DockerException as dex:
+        logger.error('Error pulling Docker image %s:' % image)
+        logger.error(dex)


Sure, will fix

zachmullen · 2017-04-28T18:52:11Z

girder_worker/plugins/docker/tests/docker_integration_test.py

@@ -135,7 +135,7 @@ def testDockerModeStdio(self):
        sys.stdout = _old

        lines = stdout_captor.getvalue().splitlines()
-        message = '%s\n' % self._test_message
+        message = '%s\r\n' % self._test_message


zachmullen · 2017-04-28T18:53:56Z

No, but one of these others might have a conflict:

[zach ~]$ pip freeze | grep docker
docker==2.2.1
docker-py==1.10.6
docker-pycreds==0.2.1

mgrauer · 2017-04-28T21:00:52Z

Will this require a change to installation or configuration instructions? I don't see any such changes in the docs.

mgrauer · 2017-04-28T22:29:41Z

What I meant to say primarily is, great feature, thanks for sticking with this over a long period and getting it done @cjh1 .

🤖

zachmullen · 2017-04-28T22:34:40Z

Agreed! Thanks!

cjh1 · 2017-05-04T13:56:24Z

@mgrauer Doesn't require any installation changes, the update to the requirements.txt should take care of everything. In fact now it should be possible to run the docker plugin without the docker cli, not that this is going to be a big use-case 😄

zachmullen · 2017-05-04T14:19:07Z

@cjh1 I wanted to bring up something @cdeepakroy asked me about yesterday -- apparently to use CUDA with docker, nvidia provides an alternative client called nvidia-docker that is a drop-in wrapper over the normal docker client executable. I don't know about the internals of it, but I'm curious if you already had any experience trying to use nvidia-docker from the SDK?

zachmullen · 2017-05-04T14:19:41Z

Here is a high level overview on their wiki, BTW. It makes it sound like it's mostly a matter of mounting special volumes...

cjh1 · 2017-05-04T15:01:26Z

@zachmullen Was I too quick to push the green button? I have not come across nvidia-docker. Read the docs it looks like nvidia-docker-plugin would still work, as this is server side, then we would just have to work what calls to the plugin endpoints the modified client is makes. Actually we may be in luck, looks like docker-py are working on added support docker/docker-py#1560, looks like it be actively worked on ...

zachmullen · 2017-05-04T15:04:12Z

Nope, this is for a future use case, just trying to contemplate how it would work in the new setup. Great to hear they are adding first-class support; if that doesn't land, I think we'll still be OK with rolling our own thing down the road.

danlamanna · 2017-05-04T15:07:59Z

+1 for supporting nvidia-docker, this is often used with SMQTK.

zachmullen · 2017-05-19T22:32:52Z

Just a heads up, I'm moving forward on the nvidia-docker exploration. If anybody has already started down this path, let's sync up.

cjh1 force-pushed the docker-sdk branch 2 times, most recently from 6636864 to 40a38d0 Compare January 31, 2017 18:29

cjh1 added 12 commits April 7, 2017 14:35

Factor out select loop into separate function

593a456

Add docker-py to requirements.txt

dbea85e

select_loop renames

4c4b748

Migrate 'docker run' to docker-py

4d3da76

Fix up test

9716a66

Migrate 'docker pull' to docker-py

f16f4a7

Fix formatting

0099dec

Fix typo

2978208

Update docker_test to expect docker-py calls

fa28e2b

Fix formatting

a45eeac

Migrate cleanup to SDK

686c307

Fix up tests after rebase

ea3be0b

cjh1 force-pushed the docker-sdk branch from 40a38d0 to ea3be0b Compare April 7, 2017 20:37

zachmullen and others added 11 commits April 12, 2017 15:19

Support streaming progress via named pipe

08dfe3b

Flake8 fixes

2c29351

Docs for new functionality

4c243f0

Merge remote-tracking branch 'origin/master' into docker-sdk

be91621

Bump docker-py version to 2.1.0

f88cf07

auto_remove is not available until 1.25

dab7b9e

Implement remove on client side

Call exit_condition at start of loop

933fdc0

Add sleep in select_loop to avoid hot loop

9ddbeac

Remove unecessary p.wait()

ff6d7d0

Fix default function for exit_condition

475173e

Fix container exit states and ensure container is reloaded

c1aae0a

cjh1 commented Apr 18, 2017

View reviewed changes

Move timeout to select call

e299660

cjh1 force-pushed the docker-sdk branch from 2331e4c to e299660 Compare April 18, 2017 20:58

Evaluate exit_condition before select

e18a024

cjh1 added 2 commits April 27, 2017 16:33

Allow extra docker run arguments to be passed

78ce781

Merge remote-tracking branch 'origin/master' into docker-sdk

e9ab515

cjh1 changed the title ~~WIP: Migrate to docker sdk~~ Migrate to docker sdk Apr 28, 2017

zachmullen suggested changes Apr 28, 2017

View reviewed changes

zachmullen reviewed Apr 28, 2017

View reviewed changes

cjh1 added 2 commits April 28, 2017 15:03

Use logger.exception(...) for logging exceptions

4ef390f

Move docker package requirement into plugin

e53abb0

zachmullen approved these changes Apr 28, 2017

View reviewed changes

cjh1 merged commit 3e8cb2d into master May 4, 2017

zachmullen deleted the docker-sdk branch May 4, 2017 14:23

manthey mentioned this pull request May 9, 2017

When pulling docker images without a tag, we get ALL tags #131

Open

cjh1 mentioned this pull request May 12, 2017

Look at migrating docker plugin to use Docker SDK for Python #87

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to docker sdk #96

Migrate to docker sdk #96

cjh1 commented Jan 31, 2017 •

edited

Loading

cjh1 Apr 18, 2017

zachmullen Apr 28, 2017

cjh1 commented Apr 28, 2017

zachmullen commented Apr 28, 2017

zachmullen commented Apr 28, 2017

cjh1 commented Apr 28, 2017

zachmullen commented Apr 28, 2017

zachmullen Apr 28, 2017

cjh1 Apr 28, 2017

zachmullen commented Apr 28, 2017

cjh1 commented Apr 28, 2017

zachmullen Apr 28, 2017

zachmullen Apr 28, 2017

cjh1 Apr 28, 2017

zachmullen Apr 28, 2017

zachmullen commented Apr 28, 2017

mgrauer commented Apr 28, 2017

mgrauer commented Apr 28, 2017

zachmullen commented Apr 28, 2017

cjh1 commented May 4, 2017

zachmullen commented May 4, 2017

zachmullen commented May 4, 2017

cjh1 commented May 4, 2017

zachmullen commented May 4, 2017

danlamanna commented May 4, 2017

zachmullen commented May 19, 2017

Migrate to docker sdk #96

Migrate to docker sdk #96

Conversation

cjh1 commented Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjh1 commented Apr 28, 2017

zachmullen commented Apr 28, 2017

zachmullen commented Apr 28, 2017

cjh1 commented Apr 28, 2017

zachmullen commented Apr 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachmullen commented Apr 28, 2017

cjh1 commented Apr 28, 2017

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

zachmullen commented Apr 28, 2017

mgrauer commented Apr 28, 2017

mgrauer commented Apr 28, 2017

zachmullen commented Apr 28, 2017

cjh1 commented May 4, 2017

zachmullen commented May 4, 2017

zachmullen commented May 4, 2017

cjh1 commented May 4, 2017

zachmullen commented May 4, 2017

danlamanna commented May 4, 2017

zachmullen commented May 19, 2017

cjh1 commented Jan 31, 2017 •

edited

Loading