Bug #11948

Fix failing CWL conformance tests

Added by Peter Amstutz 16 days ago. Updated 3 days ago.

Status:ResolvedStart date:07/11/2017
Priority:NormalDue date:
Assignee:Peter Amstutz% Done:

100%

Category:-
Target version:2017-08-02 sprint
Story points-Remaining (hours)0.00 hour
Velocity based estimate-

Description

New CWL conformance tests were introduced which fail on Arvados.

Fix arvados-cwl-runner to pass these tests.


Subtasks

Task #11963: Review 11948-cwl-conformance-fixResolvedPeter Amstutz

Associated revisions

Revision a95c9948
Added by Tom Clegg 11 days ago

Merge branch '11948-cwl-conformance-fix'

refs #11948

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

History

#1 Updated by Peter Amstutz 16 days ago

  • Description updated (diff)
  • Status changed from New to In Progress
  • Assignee set to Peter Amstutz

#2 Updated by Peter Amstutz 13 days ago

11948-cwl-conformance-fix @ d1a7f9691ff2b9d77106f953c714b1455b228c64

The main change in this branch is to accommodate a new CWL conformance test. This test checks that when a parameter has a default value that is a File or Directory, the default File is not required to exist provided some other File is assigned for the input parameter.

The implementation approach is to scan the document for "default" blocks, and check for Files under "default", and then check if those files exist. If the files do not exist, it deletes the "default" block (so that downstream processing will require a value for that parameter) and removes it from the list of files to try and upload.

#3 Updated by Lucas Di Pentima 13 days ago

Some questions:

  • File sdk/cwl/arvados_cwl/runner.py
    • Line 53: Could that if be changed to an elif, or there’s some case when a variable is a list and also a dict?
    • Lines 123-128: If I understand it correctly, is an in-place list modification, and I think could be written like this, what do you think?
      sc[:] = [x for x in sc if x[“location”] != f[“location”]]
      
    • Line 116: Why is remove a list and not simply a boolean var?

#4 Updated by Peter Amstutz 13 days ago

Lucas Di Pentima wrote:

Some questions:

  • File sdk/cwl/arvados_cwl/runner.py
    • Line 53: Could that if be changed to an elif, or there’s some case when a variable is a list and also a dict?

You're right. Fixed.

  • Lines 123-128: If I understand it correctly, is an in-place list modification, and I think could be written like this, what do you think?

That's a good one. Fixed.

  • Line 116: Why is remove a list and not simply a boolean var?

Python scopes are a little weird. When you define a function inside a function, you can access variables in the outer scope, but assignment only modifies the local scope. So setting "removed = True" wouldn't change the value of the outer "removed" variable. Instead, I use a list to add a level of indirection (sort of like a pointer).

Now @ 2feec8cbac7ef28a47f3c3a9d071b070ef38cb6e

Unit testing

arvbox start test --only sdk/cwl

Conformance tests

To set up, create a virtualenv on your host, install the CWL SDKs from the branch, start arvbox, then build a arvados/job Docker image:

virtualenv venv && . venv/bin/activate
cd ~/arvados/sdk/cwl && python setup.py install
arvbox restart dev
export ARVADOS_API_HOST=
export ARVADOS_API_TOKEN=
cd ~/arvados/build && WORKSPACE=$HOME/arvados ./build-dev-docker-jobs-image.sh 

Now you can clone the repository with the CWL tests and run them:

git clone https://github.com/common-workflow-language/common-workflow-language.git
cd common-workflow-language
./run_test.sh RUNNER=arvados-cwl-runner EXTRA="--api=containers --compute-checksum --disable-reuse" 

#5 Updated by Lucas Di Pentima 12 days ago

Local unit test worked ok on my end.
Updates LGTM.
Conformance tests couldn't be run because I wasn't able to start arvbox, just letting you know instead of blocking you because of my problem with arvbox.

#6 Updated by Tom Morris 5 days ago

  • Target version changed from 2017-07-19 sprint to 2017-08-02 sprint

#7 Updated by Tom Clegg 5 days ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF