Idea #16353: 1.2 spec work - Arvados

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

Actions

Copy link

#13

Updated by Lucas Di Pentima almost 4 years ago

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

Actions

Copy link

#14

Updated by Michael Crusoe almost 4 years ago

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.

Actions

Copy link

#15

Updated by Peter Amstutz almost 4 years ago

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

Actions

Copy link

#16

Updated by Peter Amstutz almost 4 years ago

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

Actions

Copy link

#17