sandboxjs.py: check for valid version of node/nodejs engine #337

manu-chroma · 2017-03-16T17:43:44Z

raises runtime exception if install node version is below a threshold version.
Fixes check for valid version of node / nodejs #254
added tests

cwl-bot · 2017-03-16T17:43:45Z

Can one of the admins verify this patch?

manu-chroma · 2017-03-16T18:15:05Z

the build is failing for python2 because of dependency mock i've added: https://travis-ci.org/common-workflow-language/cwltool/jobs/211850354#L176 i don't quite understand the logs here.

tests were passing locally when running tox

manu-chroma · 2017-03-16T18:19:35Z

cc @mr-c, @tetron

mr-c · 2017-03-16T18:39:38Z

cwltool/sandboxjs.py

+    v1, v2, v3 = [int(v) for v in subprocess.check_output(
+        [working_alias, "-v"]).decode('ascii').strip().strip('v').split('.')]
+
+    print(v1, v2, v3)


This should be removed

mr-c · 2017-03-16T18:40:00Z

cwltool/sandboxjs.py

+    print(v1, v2, v3)
+
+    if v1 == 0 and v2 <= 10 and v3 <= 25:
+        return "Failed"


Please return a boolean

mr-c · 2017-03-16T18:41:42Z

Jenkins, okay to test

FYI, @manu-chroma no need to tag me in issues or pull requests, I subscribe to the entire repository

manu-chroma · 2017-03-16T18:52:43Z

FYI, @manu-chroma no need to tag me in issues or pull requests, I subscribe to the entire repository

got it. sorry for unnecessary tagging.

manu-chroma · 2017-03-16T19:26:04Z

i've made the necessary changes.

conflicting requirement for rdflib seems to be triggering build failure:

pkg_resources.VersionConflict: 
(rdflib 4.2.1 (/home/travis/build/common-workflow-language/cwltool/.tox/py27-unit/lib/python2.7/site-packages), Requirement.parse('rdflib<4.3.0,>=4.2.2'))

i tested locally by bumping up rdflib in requirements.txt to 4.2.2 since tox derives its dependency from that file and that seems to have fixed the error. not sure if this is the right way counter this error.
travis build log: https://travis-ci.org/common-workflow-language/cwltool/jobs/211874138#L210

mr-c · 2017-03-20T21:02:31Z

@manu-chroma Can you examine the conformance test failures at https://ci.commonwl.org/job/cwltool-pr-conformance-multiver/208/version=v1.0/console ?

manu-chroma · 2017-03-20T22:19:57Z

hey @mr-c, i've encountered this rather weird error:
when mock was present in this order

install_requires=[
          'setuptools',
          'requests >= 1.0',
          'ruamel.yaml >= 0.12.4',
          'rdflib >= 4.2.2, < 4.3.0',
          'shellescape >= 3.4.1, < 3.5',
          'schema-salad >= 2.4.20170308171942, < 3',
          'typing >= 3.5.2, < 3.6',
          'six >= 1.10.0',
          'mock >= 2.0.0',
      ],

travis gave me error: https://travis-ci.org/common-workflow-language/cwltool/jobs/211874138
however, when I placed mock in the beginning

 install_requires=[
          'mock >= 2.0.0',
          'setuptools',
          'requests >= 1.0',
          'ruamel.yaml >= 0.12.4',
          'rdflib >= 4.2.2, < 4.3.0',
          'shellescape >= 3.4.1, < 3.5',
          'schema-salad >= 2.4.20170308171942, < 3',
          'typing >= 3.5.2, < 3.6',
          'six >= 1.10.0',
      ],

travis passed without any errors.
I could observe the same behavior when I ran tests locally.

I'm having look at jenkins log. I'll try to figure out and see what's going wrong with that. thanks.

manu-chroma · 2017-03-22T17:54:14Z

@manu-chroma Can you examine the conformance test failures at https://ci.commonwl.org/job/cwltool-pr-conformance-multiver/208/version=v1.0/console ?

also, im not able to dissect error log from the jenkins build: https://ci.commonwl.org/job/cwltool-pr-conformance-multiver/219/version=v1.0/console

this expression evaluation error in the build log is the error for almost all failing tests.

Test failed: /var/lib/jenkins/jobs/cwltool-pr-conformance-multiver/workspace/version/v1.0/cwltool-venv/bin/cwltool --enable-dev --leave-container --outdir=/tmp/tmpeC8S1_ --quiet v1.0/template-tool.cwl v1.0/cat-job.json
Test InitialWorkDirRequirement ExpressionEngineRequirement.engineConfig feature
Returned non-zero
Workflow error, try again with --debug for more information:
Expression evaluation error:
'NoneType' object has no attribute 'rfind'

mr-c · 2017-03-22T17:56:38Z

Try running a single test with --debug

For example: cwltool --debug v1.0/count-lines5-wf.cwl v1.0/empty.json

manu-chroma · 2017-03-22T19:37:29Z

cwltool/sandboxjs.py

+    """ parse node version: 'v4.2.6\n' -> ['4', '2', '6'],
+    https://github.com/nodejs/node/blob/master/CHANGELOG.md#nodejs-changelog """
+
+    v1, v2, v3 = [int(v) for v in subprocess.check_output(


@mr-c after lot's of debugging, I've come to understand that calling subprocess.check_output is the reason for failure. can I not invoke subprocess here?

manu-chroma · 2017-03-22T19:41:56Z

count-lines3-wf.cwl

#!/usr/bin/env cwl-runner
class: Workflow
cwlVersion: v1.0

inputs:
    file1:
      type: File[]

outputs:
    count_output:
      type: int[]
      outputSource: step1/output

requirements:
  ScatterFeatureRequirement: {}

steps:
  step1:
    run: wc2-tool.cwl
    scatter: file1
    in:
      file1: file1
    out: [output]

count-lines3-job.json

{
    "file1": [
        {
            "class": "File",
            "location": "whale.txt"
        },
        {
            "class": "File",
            "location": "hello.txt"
        }
    ]
}

Running cwltool

(venv) manu@hp:~/github/cwltool$ cwltool --debug v1.0/count-lines3-wf.cwl v1.0/count-lines3-job.json
/home/manu/github/cwltool/venv/bin/cwltool 1.0.20170322174222
Resolved 'v1.0/count-lines3-wf.cwl' to 'file:///home/manu/github/cwltool/v1.0/count-lines3-wf.cwl'
[workflow count-lines3-wf.cwl] initialized from file:///home/manu/github/cwltool/v1.0/count-lines3-wf.cwl
[workflow count-lines3-wf.cwl] workflow starting
[workflow count-lines3-wf.cwl] starting step step1
[job step1] initializing from file:///home/manu/github/cwltool/v1.0/wc2-tool.cwl as part of step step1
[job step1] {
    "file1": {
        "class": "File", 
        "location": "file:///home/manu/github/cwltool/v1.0/whale.txt", 
        "basename": "whale.txt"
    }
}
[job step1] path mappings is {
    "file:///home/manu/github/cwltool/v1.0/whale.txt": [
        "/home/manu/github/cwltool/v1.0/whale.txt", 
        "/tmp/tmpJgFzXN/stg068a1b3b-e7d1-4e1b-8dcf-d14380ab892a/whale.txt", 
        "File", 
        true
    ]
}
[job step1] command line bindings is [
    {
        "position": [
            -1000000, 
            0
        ], 
        "datum": "wc"
    }, 
    {
        "position": [
            0, 
            "file1"
        ], 
        "datum": {
            "class": "File", 
            "location": "file:///home/manu/github/cwltool/v1.0/whale.txt", 
            "basename": "whale.txt", 
            "path": "/tmp/tmpJgFzXN/stg068a1b3b-e7d1-4e1b-8dcf-d14380ab892a/whale.txt", 
            "dirname": "/tmp/tmpJgFzXN/stg068a1b3b-e7d1-4e1b-8dcf-d14380ab892a", 
            "nameroot": "whale", 
            "nameext": ".txt"
        }
    }
]
[job step1] /tmp/tmpDHty4M$ wc \
    /tmp/tmpJgFzXN/stg068a1b3b-e7d1-4e1b-8dcf-d14380ab892a/whale.txt > /tmp/tmpDHty4M/output.txt
Error collecting output for parameter 'output'
Traceback (most recent call last):
  File "/home/manu/github/cwltool/cwltool/draft2tool.py", line 412, in collect_output_ports
    compute_checksum=compute_checksum)
  File "/home/manu/github/cwltool/cwltool/draft2tool.py", line 507, in collect_output
    r = builder.do_eval(binding["outputEval"], context=r)
  File "build/bdist.linux-x86_64/egg/schema_salad/sourceline.py", line 150, in __exit__
    raise self.makeError(unicode(exc_value))
WorkflowException: v1.0/wc2-tool.cwl:15:9: Expression evaluation error:
v1.0/wc2-tool.cwl:15:9: [Errno 2] No such file or directory
[job step1] Job error:
v1.0/wc2-tool.cwl:10:7: Error collecting output for parameter 'output':
v1.0/wc2-tool.cwl:15:9:   Expression evaluation error:
v1.0/wc2-tool.cwl:15:9:   [Errno 2] No such file or directory
[job step1] completed permanentFail
[job step1] {}
[job step1] Removing input staging directory /tmp/tmpJgFzXN
[job step1] Removing temporary directory /tmp/tmpI28oH8
Workflow cannot make any more progress.
Removing intermediate output directory /tmp/tmpeEQn7D
Final process status is permanentFail

im afraid the traceback wasn't very useful. I tried making changes to def check_js_threshold_version(working_alias): method I had added and few logging.debug statements to identify the error.

manu-chroma · 2017-03-29T18:27:01Z

I've put a try, catch block around the subprocess call similar to what is done here for checking nodejs output : https://github.com/common-workflow-language/cwltool/blob/master/cwltool/sandboxjs.py#L35.
Tests are passing but I still don't understand why calling subprocess causes an exception.

mock is used to mock output from subprocess in tests. this is a standard module in py3 but no present in py27, that's why this dependency

- raises runtime exception if install node version is below a threshold version. - Fixes common-workflow-language#254

mr-c · 2017-05-22T07:17:58Z

cwltool/sandboxjs.py

+    raise Runtime Exception. Such a test won't be required for docker nodejs"""
+    if nodejs is not None:
+        if check_js_threshold_version(working_alias) is False:
+            raise JavascriptException(u"cwltool requires updated version of Node.js engine. "


refactor out required version of node.js engine and share the required version with the user

mr-c · 2017-05-22T07:21:44Z

tests/test_js_sandbox.py

+
+    def test_node_version(self):
+        subprocess.check_output = Mock(return_value=b'v0.8.26\n')
+        self.assertEquals(check_js_threshold_version('node'), False)


Probably need to undo the monkey patching at the end of this test

mr-c · 2017-05-22T07:36:51Z

cwltool/sandboxjs.py

+    except Exception as e:
+        _logger.debug(str(e))
+        _logger.debug("Calling subprocess failed")
+        return True


Shouldn't return true in the event of an exception

exception is from working_alias = None

mr-c · 2017-05-22T07:58:45Z

cwltool/sandboxjs.py

    for n in trynodes:
        try:
            if subprocess.check_output([n, "--eval", "process.stdout.write('t')"]) != "t":
+                working_alias = n


this branch means that node wasn't found (or found to be working for the trivial example)
I'd put your check for the correct version as the else to this if using n as your working_alias

manu-chroma · 2017-05-24T19:43:32Z

@mr-c, thanks for the review. I'll fix these errors tomo.

I tried re-running this workflow mentioned in prev comments (from the v1.0 spec) using cwltool 1.0.20170524155638 (latest code checkout from master branch). Got the following error:

(venv) [venv] ~/github/common-workflow-language/v1.0 · (master±) $ 
cwl-runner  --debug count-lines3-wf.cwl  count-lines3-job.json 
/home/manu/Desktop/cwltool/venv/bin/cwl-runner 1.0.20170524155638
Resolved 'count-lines3-wf.cwl' to 'file:///home/manu/github/common-workflow-language/v1.0/count-lines3-wf.cwl'
Tool definition failed validation:
count-lines3-wf.cwl:17:1: checking field `steps`
count-lines3-wf.cwl:18:3:   checking object `count-lines3-wf.cwl#step1`
count-lines3-wf.cwl:19:5:     Field `run` contains undefined reference to `file:///home/manu/github/common-workflow-language/v1.0/wc2-tool.cwl`
Traceback (most recent call last):
  File "build/bdist.linux-x86_64/egg/cwltool/main.py", line 692, in main
    fetcher_constructor=fetcher_constructor)
  File "build/bdist.linux-x86_64/egg/cwltool/load_tool.py", line 183, in validate_document
    processobj, metadata = document_loader.resolve_all(workflowobj, fileuri)
  File "build/bdist.linux-x86_64/egg/schema_salad/ref_resolver.py", line 828, in resolve_all
    self.validate_links(document, u"", all_doc_ids)
  File "build/bdist.linux-x86_64/egg/schema_salad/ref_resolver.py", line 988, in validate_links
    raise errors[0]
ValidationException: count-lines3-wf.cwl:17:1: checking field `steps`
count-lines3-wf.cwl:18:3:   checking object `count-lines3-wf.cwl#step1`
count-lines3-wf.cwl:19:5:     Field `run` contains undefined reference to `file:///home/manu/github/common-workflow-language/v1.0/wc2-tool.cwl`

Is this a potential issue or am I doing something wrong?

mr-c · 2017-05-24T19:52:37Z

@manu-chroma Did you move some files around? The path to that workflow is common-workflow-language/v1.0/v1.0/count-lines3-wf.cwl but you are in common-workflow-language/v1.0

manu-chroma · 2017-05-24T20:03:53Z

Did you move some files around?

Yes, I had moved these files one folder up for some reason and was running them from there.

I'm able to get the correct output now. Didn't check the backtrace properly to realize there was missing workflow file which caused the error. Thanks!

- better method to cmp versions - remove tests monkey patch - better condition checking while raising exceptions

manu-chroma · 2017-05-25T14:13:02Z

@mr-c, I've made some changes here. Few notes:

now removing monkey patching of function call after tests are done
using tuple comparison in python to compare node versions: simplified check_js_threshold_version function
made minimum_node_version_str variable global so that it can be imported in tests
due to the way we check the nodejs (both local and docker) on system, i had to use a couple of extra variables and an extra if..else condition in the end

mr-c · 2017-05-25T14:19:29Z

cwltool/main.py

-        else:
-            _logger.info(versionfunc())
+        # else:
+            # _logger.info(versionfunc())


removed this in later commit.

mr-c · 2017-05-25T15:55:07Z

Thanks!

manu-chroma · 2017-05-25T16:10:47Z

@mr-c you could've have squashed and merged the PR. Some of the commit messages were poorly written this time.

manu-chroma force-pushed the node_fix branch 4 times, most recently from cd9b6e2 to cea0de1 Compare March 16, 2017 18:00

mr-c reviewed Mar 16, 2017

View reviewed changes

manu-chroma force-pushed the node_fix branch from cea0de1 to 5a27760 Compare March 16, 2017 19:12

manu-chroma mentioned this pull request Mar 16, 2017

check for valid version of node / nodejs #254

Closed

manu-chroma force-pushed the node_fix branch 2 times, most recently from 67219c8 to 653aacb Compare March 20, 2017 22:07

manu-chroma force-pushed the node_fix branch from 653aacb to aad1d4b Compare March 21, 2017 21:44

manu-chroma mentioned this pull request Mar 22, 2017

make coverage.html; make coverage-report fails #334

Closed

manu-chroma force-pushed the node_fix branch from aad1d4b to 45db824 Compare March 22, 2017 17:42

manu-chroma commented Mar 22, 2017

View reviewed changes

manu-chroma force-pushed the node_fix branch from 45db824 to 3478420 Compare March 29, 2017 09:52

manu-chroma force-pushed the node_fix branch 2 times, most recently from 60f1a25 to a2af80a Compare March 29, 2017 19:30

manu-chroma added 2 commits March 30, 2017 05:26

setup.py: add mock module as dependency

7c4ad54

mock is used to mock output from subprocess in tests. this is a standard module in py3 but no present in py27, that's why this dependency

sandboxjs.py: checks for valid version of node/nodejs

79f5d89

- raises runtime exception if install node version is below a threshold version. - Fixes common-workflow-language#254

manu-chroma and others added 3 commits March 30, 2017 00:59

sandboxjs.py: catch subprocess exception

a2af80a

Merge branch 'master' into node_fix

a8ca7d4

Merge branch 'master' into node_fix

8ff32c3

mr-c reviewed May 22, 2017

View reviewed changes

manu-chroma changed the title ~~sandboxjs.py: checks for valid version of node/nodejs~~ sandboxjs.py: check for valid version of node/nodejs engine May 22, 2017

manu-chroma added 3 commits May 25, 2017 01:35

Merge branch 'master' into node_fix

29303c9

sandbox temp diff

1ed6bc3

sandboxjs.py :

b8c647e

- better method to cmp versions - remove tests monkey patch - better condition checking while raising exceptions

manu-chroma added 4 commits May 25, 2017 19:43

fix func return type annotaion

70605b3

Merge branch 'master' into node_fix

58ebaf8

revert changes in cwltool/main.py

8a54e7c

revert erroneous changes in cwltool/sandbox.py

342194b

mr-c approved these changes May 25, 2017

View reviewed changes

mr-c merged commit 3de4159 into common-workflow-language:master May 25, 2017

manu-chroma deleted the node_fix branch June 19, 2017 13:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sandboxjs.py: check for valid version of node/nodejs engine #337

sandboxjs.py: check for valid version of node/nodejs engine #337

manu-chroma commented Mar 16, 2017

cwl-bot commented Mar 16, 2017

manu-chroma commented Mar 16, 2017 •

edited

Loading

manu-chroma commented Mar 16, 2017

mr-c Mar 16, 2017

mr-c Mar 16, 2017

mr-c commented Mar 16, 2017

manu-chroma commented Mar 16, 2017

manu-chroma commented Mar 16, 2017

mr-c commented Mar 20, 2017

manu-chroma commented Mar 20, 2017 •

edited

Loading

manu-chroma commented Mar 22, 2017

mr-c commented Mar 22, 2017

manu-chroma Mar 22, 2017

manu-chroma commented Mar 22, 2017

manu-chroma commented Mar 29, 2017

mr-c May 22, 2017

mr-c May 22, 2017

mr-c May 22, 2017

mr-c May 22, 2017

mr-c May 22, 2017

manu-chroma commented May 24, 2017

mr-c commented May 24, 2017

manu-chroma commented May 24, 2017

manu-chroma commented May 25, 2017 •

edited

Loading

mr-c May 25, 2017

manu-chroma May 25, 2017

mr-c commented May 25, 2017

manu-chroma commented May 25, 2017

sandboxjs.py: check for valid version of node/nodejs engine #337

sandboxjs.py: check for valid version of node/nodejs engine #337

Conversation

manu-chroma commented Mar 16, 2017

cwl-bot commented Mar 16, 2017

manu-chroma commented Mar 16, 2017 • edited Loading

manu-chroma commented Mar 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mr-c commented Mar 16, 2017

manu-chroma commented Mar 16, 2017

manu-chroma commented Mar 16, 2017

mr-c commented Mar 20, 2017

manu-chroma commented Mar 20, 2017 • edited Loading

manu-chroma commented Mar 22, 2017

mr-c commented Mar 22, 2017

Choose a reason for hiding this comment

manu-chroma commented Mar 22, 2017

manu-chroma commented Mar 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manu-chroma commented May 24, 2017

mr-c commented May 24, 2017

manu-chroma commented May 24, 2017

manu-chroma commented May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mr-c commented May 25, 2017

manu-chroma commented May 25, 2017

manu-chroma commented Mar 16, 2017 •

edited

Loading

manu-chroma commented Mar 20, 2017 •

edited

Loading

manu-chroma commented May 25, 2017 •

edited

Loading