Arvados

Fixes in branch 10194-cwl-version-skew:

• setup.py records git version to _version.py
• --submit propagates git version as tag to arvados/jobs docker image (instead of using "latest")
• tweak crunch v1 submit behavior: create job, then create pipeline instance containing that job, but mark pipeline instance as RunningOnServer instead of RunningOnClient so that a-r-p-i is responsible for marking the pipeline instance as completed.
• add script to test that python-arvados-cwl-runner package installs successfully
• packaging fixes as a result of fallout from adding test scripts

A few comments:

• sdk/cwl/setup.py:
  • Line 20 & 23: Is the CWD guaranteed to be the same as the script’s dir at the moment of execution?
  • Line 25: I think it would be better to fail instead of ignore exceptions, we could be getting version skew again without noticing, caused by unforseen reasons. (If this is executed manually, disregard the comment as the warning message should be enough)
• sdk/cwl/arvados_cwl/arvdocker.py:
  • Line 26: imageId var is not used anywhere, is the execution of cwltool.docker.get_image() needed?

Local sdk/cwl tests passed OK. So besides these coments, this LGTM.

Updated a70a555fbd30524568a0e019df184468f28556f0

(I was writing this review comment when the branch got merged, still leaving the comment just in case you find it useful)

On setup.py, if the idea is to check for subprocess.check_output() success, do you think moving the versionfile writing to an else clause would be clearer? Something like:

[...]
versionfile = os.path.join(SETUP_DIR, "arvados_cwl/_version.py")
try:
    gitinfo = subprocess.check_output(
        ['git', 'log', '--first-parent', '--max-count=1',
         '--format=format:%H', SETUP_DIR]).strip()
except Exception as e:
    # When installing from package, it won't be part of a git repository, and
    # check_output() will raise an exception.  But the package should include the
    # version file, so we can proceed.
    if not os.path.exists(versionfile):
        raise
else:
    with open(versionfile, "w") as f:
        f.write("__version__ = '%s'\n" % gitinfo)
[...]

Lucas Di Pentima wrote:

(I was writing this review comment when the branch got merged, still leaving the comment just in case you find it useful)

Whoops, sorry about that, I should have checked in with you. (I interpreted "LGTM with minor changes" as "ok to merge" but maybe we should clarify that at the next retrospective).

On setup.py, if the idea is to check for subprocess.check_output() success, do you think moving the versionfile writing to an else clause would be clearer? Something like:

I agree that would be a bit more elegant, maybe I'll sneak that into a future branch.

Peter Amstutz wrote:

Additional branch 10194-jobs-image-versioning which improves handling of package versions related to building the arvados/jobs Docker image, which is necessary for Docker image pinning to work as intended.

Reviewing 10194-jobs-image-versioning at 4264bbad60a87547cfca97e48848ff120e660372

build/run-build-docker-jobs-image.sh:

• line 121: python_version_sdk prefix is being set incorrectly, it is 0.1 not 1.0, and the suffix is -2 not -3 (this really needs to be factored out into the library, but that's for later I think)
• line 162: missing $ before 'gittag'
• line 141: this code tests the exit value for docker tag, not docker build. It needs a patch along these lines:

--- a/build/run-build-docker-jobs-image.sh
+++ b/build/run-build-docker-jobs-image.sh
@@ -147,6 +145,23 @@ fi
 checkexit $ECODE "docker build" 
 title "docker build complete (`timer`)" 

+if [[ "$ECODE" != "0" ]]; then
+  exit_cleanly
+fi
+
+timer_reset
+
+docker tag arvados/jobs:$gittag arvados/jobs:latest
+
+ECODE=$?
+
+if [[ "$ECODE" != "0" ]]; then
+    EXITCODE=$(($EXITCODE + $ECODE))
+fi
+
+checkexit $ECODE "docker tag" 
+title "docker tag complete (`timer`)" 
+
 title "uploading images" 

 timer_reset

Otherwise, this looks good to me. Thanks!