Story #16353

1.2 spec work

Added by Peter Amstutz 9 months ago. Updated 6 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
CWL
Target version:
Start date:
07/07/2020
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Subtasks

Task #16577: Publish CWL 1.2ResolvedPeter Amstutz

Task #16595: review 16353-cwl-1.2ResolvedPeter Amstutz

Task #16677: Review 16353-1.2-releaseResolvedPeter Amstutz


Related issues

Blocks Arvados - Bug #16297: Dirent entry Expression not handled correctly according to CWL specResolved

Associated revisions

Revision e45d6fea
Added by Peter Amstutz 6 months ago

Merge branch '16353-cwl-1.2' refs #16353

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

Revision c6858cd3
Added by Peter Amstutz 6 months ago

Merge branch '16353-1.2-release' refs #16353

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

History

#1 Updated by Peter Amstutz 9 months ago

  • Assigned To set to Peter Amstutz
  • Category set to CWL
  • Subject changed from CWL 1.2 spec work to 1.2 spec work

#2 Updated by Peter Amstutz 9 months ago

  • Target version changed from 2020-05-06 Sprint to 2020-05-20 Sprint

#3 Updated by Peter Amstutz 9 months ago

  • Target version changed from 2020-05-20 Sprint to 2020-06-03 Sprint

#4 Updated by Peter Amstutz 8 months ago

  • Target version changed from 2020-06-03 Sprint to 2020-06-17 Sprint

#5 Updated by Peter Amstutz 8 months ago

  • Target version changed from 2020-06-17 Sprint to 2020-07-01 Sprint

#6 Updated by Peter Amstutz 7 months ago

  • Status changed from New to In Progress

#7 Updated by Peter Amstutz 7 months ago

  • Target version changed from 2020-07-01 Sprint to 2020-07-15

#8 Updated by Peter Amstutz 7 months ago

16353-cwl-1.2 @ 6563930ed602f47c1489b39b2eebfb54f9945e3c

https://ci.arvados.org/view/Developer/job/developer-run-tests/1953/

Passes all cwl 1.2.0-dev4 tests.

Hoping to release CWL 1.2 specification next week.

#9 Updated by Peter Amstutz 7 months ago

  • Blocks Bug #16297: Dirent entry Expression not handled correctly according to CWL spec added

#11 Updated by Peter Amstutz 6 months ago

  • Target version changed from 2020-07-15 to 2020-08-12 Sprint

#12 Updated by Lucas Di Pentima 6 months ago

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: https://ci.arvados.org/job/arvados-cwl-conformance-tests/480/

#13 Updated by Lucas Di Pentima 6 months ago

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

#14 Updated by Michael Crusoe 6 months 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.

#15 Updated by Peter Amstutz 6 months 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.

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

https://ci.arvados.org/view/Developer/job/developer-run-tests/1965/

https://ci.arvados.org/view/CWL/job/arvados-cwl-conformance-tests/484/

#16 Updated by Peter Amstutz 6 months 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:

https://ci.arvados.org/job/arvados-cwl-conformance-tests/486/

#17 Updated by Lucas Di Pentima 6 months ago

85a3314 LGTM, thanks!

#18 Updated by Peter Amstutz 6 months ago

  • Release set to 25

#20 Updated by Lucas Di Pentima 6 months ago

Just one suggestion:

The rest LGTM. Thanks!

#21 Updated by Peter Amstutz 6 months ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF