Experimental fast validator code path, using cwl-utils #1720

tetron · 2022-08-26T21:19:50Z

Codegen changes to support cwltool --fast-parse cwl-utils#156 merged & released
documentation of limitations
tests
rename to --fast-parser
post-process hints to expand class fields in extensions

This is a work in progress.

Requires schema salad codegen improvements in common-workflow-language/schema_salad#587 which will need to be applied to cwl-utils.

On a very large workflow I was testing with, the validation time went
120 seconds to 20 seconds.

Requires schema salad codegen improvements, these are pending On a very large workflow I was testing with, the validation time went 120 seconds to 20 seconds. This is a work in progress.

tetron · 2022-08-26T21:25:14Z

To-do:

fix the bugs that are breaking conformance tests (16 failures from the draft v1.2.1 tests as of this writing)
fixing type annotations

codecov · 2022-08-26T21:27:31Z

Codecov Report

Merging #1720 (2aa44d0) into main (1d23218) will decrease coverage by 8.92%.
The diff coverage is 91.01%.

@@            Coverage Diff             @@
##             main    #1720      +/-   ##
==========================================
- Coverage   66.61%   57.68%   -8.93%     
==========================================
  Files          89       99      +10     
  Lines       15856    11479    -4377     
  Branches     4190     2310    -1880     
==========================================
- Hits        10562     6622    -3940     
- Misses       4206     4368     +162     
+ Partials     1088      489     -599

Impacted Files	Coverage Δ
setup.py	`0.00% <ø> (ø)`
cwltool/update.py	`72.50% <75.00%> (-0.05%)`	⬇️
cwltool/context.py	`97.05% <83.33%> (-0.54%)`	⬇️
cwltool/load_tool.py	`86.66% <93.05%> (+1.53%)`	⬆️
cwltool/argparser.py	`95.21% <100.00%> (+0.41%)`	⬆️
cwltool/main.py	`74.29% <100.00%> (+2.31%)`	⬆️
cwltool/cwltool/cwlviewer.py
cwltool/cwltool/workflow.py
cwltool/cwltool/loghandler.py
cwltool/cwltool/__main__.py
... and 112 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

lgtm-com · 2022-08-26T21:32:13Z

This pull request introduces 1 alert when merging f6d8198 into 0e2ced5 - view on LGTM.com

new alerts:

1 for Wrong name for an argument in a class instantiation

tetron · 2022-08-27T18:21:42Z

With parser from latest codegen fixes, now passing the entire v1.2.1-proposed conformance suite.

lgtm-com · 2022-08-27T18:28:48Z

This pull request introduces 1 alert when merging b145e3c into 0e2ced5 - view on LGTM.com

new alerts:

1 for Wrong name for an argument in a class instantiation

mr-c · 2022-08-27T18:31:36Z

Great progress!

Requires schema salad codegen improvements in common-workflow-language/schema_salad#587 which will need to be applied to cwl-utils.

Can we get a draft cwl-utils PR with the new parsers?

tetron · 2022-08-29T17:18:44Z

Can we get a draft cwl-utils PR with the new parsers?

As soon as I can make mypy happy

tetron · 2022-08-31T18:24:25Z

We probably also want a CI action that run the conformance tests with --fast-validator

lgtm-com · 2022-09-01T16:25:40Z

This pull request introduces 1 alert when merging b00b17e into 2a2216b - view on LGTM.com

new alerts:

1 for Unused import

lgtm-com · 2022-09-01T18:03:35Z

This pull request introduces 1 alert when merging cf846bc into 2a2216b - view on LGTM.com

new alerts:

1 for Unused import

lgtm-com · 2022-09-02T15:13:22Z

This pull request introduces 1 alert when merging 3adbcbc into 2a2216b - view on LGTM.com

new alerts:

1 for Unused import

mr-c · 2022-09-02T15:44:59Z

For the docs, we need to explain if there are any downsides. (Are error messages less helpful? Are some types of errors not caught by this code path?)

tetron · 2022-09-02T15:50:06Z

For the docs, we need to explain if there are any downsides. (Are error messages less helpful? Are some types of errors not caught by this code path?)

There are two main gaps currently:

It doesn't do link checking (so things like a dangling 'source' field won't be reported, it'll fail at runtime)
Runtime errors won't have file/row/column at all

Validation errors do include file/row/column but are somewhat less polished.

mr-c · 2022-09-02T15:57:02Z

For the docs, we need to explain if there are any downsides. (Are error messages less helpful? Are some types of errors not caught by this code path?)

There are two main gaps currently:
1. It doesn't do link checking (so things like a dangling 'source' field won't be reported, it'll fail at runtime)

2. Runtime errors won't have file/row/column at all
Validation errors do include file/row/column but are somewhat less polished.

Confirmed by editing tests/echo_broken_outputs.cwl to be a v1.2 file:

$ cwltool --fast-validator --validate tests/echo_broken_outputs.cwl
INFO /home/michael/cwltool/env3.10/bin/cwltool 3.1.20220902151754
INFO Resolved 'tests/echo_broken_outputs.cwl' to 'file:///home/michael/cwltool/tests/echo_broken_outputs.cwl'
ERROR Tool definition failed validation:
- tried CommandLineTool but
  Trying 'CommandLineTool'
    the `outputs` field is not valid because:
      Expected a list, was <class 'NoneType'>
- tried ExpressionTool but
  Not a ExpressionTool
- tried Workflow but
  Not a Workflow
- tried Operation but
  Not a Operation
- tried array<CommandLineTool | ExpressionTool | Workflow | Operation> but
  Expected a list, was <class 'dict'>
$ cwltool  --validate tests/echo_broken_outputs.cwl
INFO /home/michael/cwltool/env3.10/bin/cwltool 3.1.20220902151754
INFO Resolved 'tests/echo_broken_outputs.cwl' to 'file:///home/michael/cwltool/tests/echo_broken_outputs.cwl'
ERROR Tool definition failed validation:
tests/echo_broken_outputs.cwl:10:1: "outputs" section is not valid.

A candidate for a unit test

lgtm-com · 2022-09-02T16:07:36Z

This pull request introduces 1 alert when merging 80271c4 into f43ca87 - view on LGTM.com

new alerts:

1 for Unused import

lgtm-com · 2022-09-02T16:20:36Z

This pull request introduces 1 alert when merging 8c1c82d into f43ca87 - view on LGTM.com

new alerts:

1 for Unused import

mr-c · 2022-09-02T16:21:35Z

ooh, it doesn't catch the empty requirements section in tests/wc-tool-bad-reqs.cwl

$ cwltool  --validate tests/wc-tool-bad-reqs.cwl
INFO /home/michael/cwltool/env3.10/bin/cwltool 3.1.20220902151754
INFO Resolved 'tests/wc-tool-bad-reqs.cwl' to 'file:///home/michael/cwltool/tests/wc-tool-bad-reqs.cwl'
ERROR Tool definition failed validation:
tests/wc-tool-bad-reqs.cwl:6:1: If 'requirements' is present then it must be a list or
                                map/dictionary, not empty.
$ cwltool --fast-validator --validate tests/wc-tool-bad-reqs.cwl
INFO /home/michael/cwltool/env3.10/bin/cwltool 3.1.20220902151754
INFO Resolved 'tests/wc-tool-bad-reqs.cwl' to 'file:///home/michael/cwltool/tests/wc-tool-bad-reqs.cwl'
tests/wc-tool-bad-reqs.cwl is valid CWL.

and default file objects are not checked either

$ cwltool  --validate tests/wf/default-dir5.cwl
INFO /home/michael/cwltool/env3.10/bin/cwltool 3.1.20220902151754
INFO Resolved 'tests/wf/default-dir5.cwl' to 'file:///home/michael/cwltool/tests/wf/default-dir5.cwl'
tests/wf/default-dir5.cwl:20:13: Warning: Field `location` contains undefined reference to
                                 `file:///home/michael/cwltool/tests/wf/inp1`
tests/wf/default-dir5.cwl:20:13: Warning: Field `location` contains undefined reference to
                                 `file:///home/michael/cwltool/tests/wf/inp1`
tests/wf/default-dir5.cwl is valid CWL.
$ cwltool --fast-validator --validate tests/wf/default-dir5.cwl
INFO /home/michael/cwltool/env3.10/bin/cwltool 3.1.20220902151754
INFO Resolved 'tests/wf/default-dir5.cwl' to 'file:///home/michael/cwltool/tests/wf/default-dir5.cwl'
tests/wf/default-dir5.cwl is valid CWL.

And extensions don't work either. We should error on combining --fast-validator and --enable-ext

$ cwltool  --enable-ext --validate tests/wf/generator/pytoolgen.cwl
INFO /home/michael/cwltool/env3.10/bin/cwltool 3.1.20220902151754
INFO Resolved 'tests/wf/generator/pytoolgen.cwl' to 'file:///home/michael/cwltool/tests/wf/generator/pytoolgen.cwl'
tests/wf/generator/pytoolgen.cwl is valid CWL.

$ cwltool --fast-validator --enable-ext --validate tests/wf/generator/pytoolgen.cwl
INFO /home/michael/cwltool/env3.10/bin/cwltool 3.1.20220902151754
INFO Resolved 'tests/wf/generator/pytoolgen.cwl' to 'file:///home/michael/cwltool/tests/wf/generator/pytoolgen.cwl'
ERROR Tool definition failed validation:
- tried CommandLineTool but
  Not a CommandLineTool
- tried ExpressionTool but
  Not a ExpressionTool
- tried Workflow but
  Not a Workflow
- tried Operation but
  Not a Operation
- tried array<CommandLineTool | ExpressionTool | Workflow | Operation> but
  Expected a list, was <class 'dict'>

It is also intolerant of extra id fields, like in https://www.commonwl.org/v1.2/CommandLineTool.html#CommandInputRecordSchema found in tests/wf/schemadef-bug-1473.cwl

mr-c

odd that we are missing test coverage at https://github.com/common-workflow-language/cwltool/pull/1720/files#diff-7bc82a2b4dbfbc386a1c1d22dac00140d782b959e858cd99978fc3ee193d15f2R273 and beyond ; can you look into that? Is test coverage not being collected in the --fast-parser github action run of the conformance tests?

README.rst

Co-authored-by: Michael R. Crusoe <[email protected]>

tetron · 2022-09-13T16:19:58Z

I don't actually know how we enable coverage data to be collected?

mr-c · 2022-09-13T16:28:37Z

I don't actually know how we enable coverage data to be collected?

cwltool/conformance-test.sh

Line 86 in 1e150ab

cat > "${CWLTOOL_WITH_COV}" <<EOF

tetron · 2022-09-13T16:36:06Z

Well now it's failing on "pinging codecov" 🤦

mr-c · 2022-09-13T17:08:39Z

Well now it's failing on "pinging codecov" facepalm

Do you have a link? I'm not seeing that..

tetron · 2022-09-13T17:15:28Z

Do you have a link? I'm not seeing that..

It was one of the earlier test iterations (py36 I think), but it seems to have just been a transient failure.

mr-c · 2022-09-13T17:22:15Z

Coverage is being recorded for cwltool/cwltool/*.py and cwltool/*.py https://app.codecov.io/gh/common-workflow-language/cwltool/pull/1720 ; this is probably a source of confusion

mr-c · 2022-09-13T18:03:48Z

The diff coverage is 91.01%.

Much better! (it was previously 33%)

On a very large workflow I was testing with, the validation time went 120 seconds to 20 seconds. * conformance testing: use CWLTOOL_OPTIONS * CWLTOOL_OPTIONS: ignore an empty string * Add loadingContext.skip_resolve_all with note * conformance tests: report CWLTOOL_OPTIONS * CI: include extra options in coverge classname * conformance coverage: normalize paths * Bump cwl-utils version requirement Co-authored-by: Michael R. Crusoe <[email protected]>

Experimental fast validator code path, using cwl-utils

f6d8198

Requires schema salad codegen improvements, these are pending On a very large workflow I was testing with, the validation time went 120 seconds to 20 seconds. This is a work in progress.

Load the schema graph

b145e3c

mr-c added 2 commits August 31, 2022 19:47

DO NOT MERGE: use cwl-utils from draft PR

3cb3d42

improve type hints

a5a1b85

Fix load_tool typing

b00b17e

Merge branch 'main' into pa/cwlutil-validator

cf846bc

GA: run the conformance tests with the fast validator

3adbcbc

conformance testing: use CWLTOOL_OPTIONS

772a11c

mr-c force-pushed the pa/cwlutil-validator branch from 8574126 to 38203f3 Compare September 2, 2022 15:29

mr-c added 2 commits September 2, 2022 17:37

CWLTOOL_OPTIONS: ignore an empty string

d140151

DO NOT MERGE: container build needs our cwl-util branch

80271c4

mr-c force-pushed the pa/cwlutil-validator branch from 38203f3 to 80271c4 Compare September 2, 2022 15:37

Merge branch 'main' into pa/cwlutil-validator

8c1c82d

tetron added 3 commits September 9, 2022 14:07

Add section to README about --fast-parser

833b0d6

Fast parser expects comment data

d64be75

format

1113260

tetron marked this pull request as ready for review September 9, 2022 20:13

tetron marked this pull request as draft September 10, 2022 16:23

tetron added 6 commits September 10, 2022 12:27

Update document_loader index to match fast parser

efa5c44

Fix format

acaf81c

Make sure everything in $graph gets into the index

0584fe8

Don't crash update if filename field is missing

46f53bc

Add loadingContext.skip_resolve_all with note

03b2f02

Fix format

7630a51

tetron marked this pull request as ready for review September 12, 2022 13:27

mr-c requested changes Sep 13, 2022

View reviewed changes

README.rst Outdated Show resolved Hide resolved

tetron and others added 2 commits September 13, 2022 12:13

Update README.rst

db08a47

Co-authored-by: Michael R. Crusoe <[email protected]>

conformance tests: report CWLTOOL_OPTIONS

1e150ab

CI: include extra options in coverge classname

f3e14e1

Bump cwl-utils version requirement

746033b

conformance coverage: normalize paths

2aa44d0

mr-c approved these changes Sep 13, 2022

View reviewed changes

mr-c enabled auto-merge (squash) September 13, 2022 18:05

mr-c merged commit cc3b260 into main Sep 13, 2022

mr-c deleted the pa/cwlutil-validator branch September 13, 2022 18:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experimental fast validator code path, using cwl-utils #1720

Experimental fast validator code path, using cwl-utils #1720

tetron commented Aug 26, 2022 •

edited

Loading

tetron commented Aug 26, 2022 •

edited

Loading

codecov bot commented Aug 26, 2022 •

edited

Loading

lgtm-com bot commented Aug 26, 2022

tetron commented Aug 27, 2022

lgtm-com bot commented Aug 27, 2022

mr-c commented Aug 27, 2022 •

edited

Loading

tetron commented Aug 29, 2022

tetron commented Aug 31, 2022

lgtm-com bot commented Sep 1, 2022

lgtm-com bot commented Sep 1, 2022

lgtm-com bot commented Sep 2, 2022

mr-c commented Sep 2, 2022

tetron commented Sep 2, 2022

mr-c commented Sep 2, 2022

lgtm-com bot commented Sep 2, 2022

lgtm-com bot commented Sep 2, 2022

mr-c commented Sep 2, 2022

mr-c left a comment

tetron commented Sep 13, 2022

mr-c commented Sep 13, 2022

tetron commented Sep 13, 2022

mr-c commented Sep 13, 2022

tetron commented Sep 13, 2022 •

edited

Loading

mr-c commented Sep 13, 2022 •

edited

Loading

mr-c commented Sep 13, 2022

Experimental fast validator code path, using cwl-utils #1720

Experimental fast validator code path, using cwl-utils #1720

Conversation

tetron commented Aug 26, 2022 • edited Loading

tetron commented Aug 26, 2022 • edited Loading

codecov bot commented Aug 26, 2022 • edited Loading

Codecov Report

lgtm-com bot commented Aug 26, 2022

tetron commented Aug 27, 2022

lgtm-com bot commented Aug 27, 2022

mr-c commented Aug 27, 2022 • edited Loading

tetron commented Aug 29, 2022

tetron commented Aug 31, 2022

lgtm-com bot commented Sep 1, 2022

lgtm-com bot commented Sep 1, 2022

lgtm-com bot commented Sep 2, 2022

mr-c commented Sep 2, 2022

tetron commented Sep 2, 2022

mr-c commented Sep 2, 2022

lgtm-com bot commented Sep 2, 2022

lgtm-com bot commented Sep 2, 2022

mr-c commented Sep 2, 2022

mr-c left a comment

Choose a reason for hiding this comment

tetron commented Sep 13, 2022

mr-c commented Sep 13, 2022

tetron commented Sep 13, 2022

mr-c commented Sep 13, 2022

tetron commented Sep 13, 2022 • edited Loading

mr-c commented Sep 13, 2022 • edited Loading

mr-c commented Sep 13, 2022

tetron commented Aug 26, 2022 •

edited

Loading

tetron commented Aug 26, 2022 •

edited

Loading

codecov bot commented Aug 26, 2022 •

edited

Loading

mr-c commented Aug 27, 2022 •

edited

Loading

tetron commented Sep 13, 2022 •

edited

Loading

mr-c commented Sep 13, 2022 •

edited

Loading