search

girder / girder_worker

Distributed task execution engine with Girder integration, developed by Kitware
http://girder-worker.readthedocs.io/
Apache License 2.0
34 stars 30 forks source link

Ensure all docker_run container_args are strings #358

Closed zachmullen closed 4 years ago

zachmullen commented 4 years ago

Fixes #357

@agirault can you verify if this fixes the issue?

agirault commented 4 years ago

Wouldn't that be a problem for objects like GirderItemIdToVolume or VolumePath which are valid arguments?

zachmullen commented 4 years ago

I had assumed that by that point in the code, everything coming in was already transformed, but it looks like that's not the case since the streaming types can still be present.

@kotfic can you comment on this? At what point should all transforms have been completed and it should be safe for us to str-ify the container args? Maybe it should go inside _run_container itself just before we call client.containers.run?

kotfic commented 4 years ago

Sorry I wasn't tracking the issue - I'm actually not sure automatically type casing is desirable here. I suppose we could cast to string if not type Transform if we want to be magical, or provide a more informative exception (which i would be more inclined towards). As the original author @cjh1 may have more insight?

cjh1 commented 4 years ago

My understanding is that at the point the docker run is executed everything should be a string anyway, but its been a while.

zachmullen commented 4 years ago

I've moved the logic to a place where I'm sure that all container args should have been transformed. PTAL.

zachmullen commented 4 years ago

I'm going to fix CI on a separate branch.

zachmullen commented 4 years ago

Rebased, PTAL @cjh1 @kotfic