Arvados

16353-cwl-1.2 @ 6563930ed602f47c1489b39b2eebfb54f9945e3c

developer-run-tests: #1953

Passes all cwl 1.2.0-dev4 tests.

Hoping to release CWL 1.2 specification next week.

16353-cwl-1.2 @ 150a601dbcaf6b999e54bd17d5152f15b626c7f3

developer-run-tests: #1957

Reviewing 150a601

• File sdk/cwl/arvados_cwl/runner.py Line 176 & 177: I assume that conditional is to support 1.0 documents and the new spec is about 1.2 docs, right? If that’s a correct assumption, shouldn’t the conditional be inverted? I mean, builder.do_eval(sf["pattern"], context=primary) was being used for 1.0 documents before, right?
• Do we need to add some test for the new features or the conformance tests are enough?
• Tried to run conformance tests on Jenkins and failed: arvados-cwl-conformance-tests: #480

Hmm, the conformance tests seem to be failing because of some docker issue.

Lucas Di Pentima wrote:

Reviewing 150a601

• File sdk/cwl/arvados_cwl/runner.py Line 176 & 177: I assume that conditional is to support 1.0 documents and the new spec is about 1.2 docs, right? If that’s a correct assumption, shouldn’t the conditional be inverted? I mean, builder.do_eval(sf["pattern"], context=primary) was being used for 1.0 documents before, right?

pattern was introduced in CWL v1.1: https://www.commonwl.org/v1.1/CommandLineTool.html#SecondaryFileSchema . I'm not familiar with the arvados codebase, but in cwltool we would have upgraded to the latest syntax at this point. If that isn't the case for arvados, then the logic is correct.

Lucas Di Pentima wrote:

Reviewing 150a601

• File sdk/cwl/arvados_cwl/runner.py Line 176 & 177: I assume that conditional is to support 1.0 documents and the new spec is about 1.2 docs, right? If that’s a correct assumption, shouldn’t the conditional be inverted? I mean, builder.do_eval(sf["pattern"], context=primary) was being used for 1.0 documents before, right?

sf["pattern"] is correct for v1.1 and later, and just sf is correct for v1.0. That's because optional secondaryFiles was introduced in v1.1.

What did change is the updater behavior. Previously this was run on CWL that was always updated to latest, but now it is also run on CWL that could be v1.0 as well. So it needs to handle both patterns.

• Do we need to add some test for the new features or the conformance tests are enough?

I'm relying on the conformance tests, and the Arvados integration tests for Arvados specific features / regressions.

• Tried to run conformance tests on Jenkins and failed: arvados-cwl-conformance-tests: #480

It wants to pull an arvbox/demo image with the git commit matching the branch, but it hasn't been built yet. The pipeline is currently set up so that run-tests build the Docker image and then that kicks off the conformance tests. I'm manually running just the Docker image job.

16353-cwl-1.2 @ 339da6b41fe014db93bd123cc9285cbeefe16936

developer-run-tests: #1965

arvados-cwl-conformance-tests: #484

Running conformance tests on jenkins on a branch is actually a little more complicated.

It requires building arvbox and arvados/jobs images for exactly the right version.

Turns out there's a bug in arvbox where, when building the 'localdemo' image, it wasn't checking out the right version. Fixed.

It also turns out that to build an arvados/jobs image on jenkins, it need the Debian 10 packages for that version in the dev repository.

So I built Debian 10 packages on jenkins. Which might cause any dev clusters that run Debian 10 (I don't know if any do) to update to the development branch, even though it isn't merged.

Using Debian 10 packages, I built an arvados/jobs image, which makes it possible for the conformance tests to run against the code in the branch:

arvados-cwl-conformance-tests: #486

85a3314 LGTM, thanks!

16353-1.2-release @ 509084f45ffd17d740f7a26285210f8cde51f84c

developer-run-tests: #1998

Just one suggestion:

• We could mention support for CWL 1.2 on the user guide: https://doc.arvados.org/master/user/cwl/cwl-versions.html

The rest LGTM. Thanks!