Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move logging messages to exceptions #296

Closed
wants to merge 2 commits into from
Closed

Move logging messages to exceptions #296

wants to merge 2 commits into from

Conversation

tom-tan
Copy link
Member

@tom-tan tom-tan commented Oct 30, 2019

Some errors are treated as warning that cannot be caught as exceptions.
This request fixes this issue by rethrowing the exception instead of just printing.

Example input:

#!/usr/bin/env cwl-runner

cwlVersion: v1.0a

class: CommandLineTool

hints:
  - class: ResourceRequirement
    coresMin: 2
    ramMin: 8
  - class: DockerRequirementXX
    dockerPull: python:2-slim

inputs:
  - id: reference
    type: File
    inputBinding: { position: 2 }

  - id: reads
    type:
      type: array
      items: File
    inputBinding: { position: 3 }

  - id: minimum_seed_length
    type: int
    inputBinding: { position: 1, prefix: -m }

  - id: min_std_max_min
    type: { type: array, items: int }
    inputBinding:
      position: 1
      prefix: -I
      itemSeparator: ","

  - id: args.py
    type: File
    default:
      class: File
      location: args.py
    inputBinding:
      position: -1

outputs:
  - id: sam
    type: ["null", File]
    outputBinding: { glob: output.sam }
  - id: args
    type:
      type: array
      items: string

baseCommand: python

arguments:
  - bwa
  - mem
  - valueFrom: $(runtime.cores)
    position: 1
    prefix: -t

stdout: output.sam
  • Before merging this request (The following is 4.3.20190604170443 but I obtained the same result in latest, i.e., 4.5.20191023134839)
$ schema-salad-tool --version
/Users/tom-tan/.pyenv/versions/3.8.0/bin/schema-salad-tool Current version: 4.3.20190604170443
$ schema-salad-tool ../common-workflow-language/v1.0/CommonWorkflowLanguage.yml error.cwl
/Users/tom-tan/.pyenv/versions/3.8.0/bin/schema-salad-tool Current version: 4.3.20190604170443
../../error.cwl:11:5:   checking item
../../error.cwl:11:5:     Field `class` contains undefined reference to `file:///Users/tom-tan/DockerRequirementXX`
../../error.cwl:40:7:   Field `location` contains undefined reference to `file:///Users/tom-tan/args.py`
Document `/Users/tom-tan/error.cwl` failed validation:
../../error.cwl:3:1: Field `cwlVersion` contains undefined reference to `file:///Users/tom-tan/v1.0a`
  • After merging this request
$ python -m schema_salad ../common-workflow-language/v1.0/CommonWorkflowLanguage.yml ~/error.cwl
/Users/tanjo/repos/schema_salad/schema_salad/__main__.py Current version: 4.5.20191029103748
Document `/Users/tanjo/error.cwl` failed validation:
../../error.cwl:3:1:  Field `cwlVersion` contains undefined reference to `file:///Users/tanjo/v1.0a`
../../error.cwl:11:5: checking item
                        Field `class` contains undefined reference to
                        `file:///Users/tanjo/DockerRequirementXX`
../../error.cwl:14:1: checking field `inputs`
../../error.cwl:36:5:   checking object `../../error.cwl#args.py`
../../error.cwl:40:7:     Field `location` contains undefined reference to
                          `file:///Users/tanjo/args.py`

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@codecov
Copy link

codecov bot commented Oct 30, 2019

Codecov Report

Merging #296 into master will increase coverage by 0.07%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #296      +/-   ##
==========================================
+ Coverage   69.53%   69.61%   +0.07%     
==========================================
  Files          15       15              
  Lines        2797     2797              
  Branches      764      764              
==========================================
+ Hits         1945     1947       +2     
  Misses        674      674              
+ Partials      178      176       -2     
Impacted Files Coverage Δ
schema_salad/ref_resolver.py 82.73% <0.00%> (+0.24%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbe70a3...8b318b8. Read the comment docs.

@mr-c
Copy link
Member

mr-c commented Oct 30, 2019

Thank you @tom-tan for this, lets wait for #295 to get merged first, as some of the logic in 2 of the cwltool tests will need to be updated

@mr-c
Copy link
Member

mr-c commented Apr 22, 2020

This PR results in 3 cwltool tests failing:

test_default_path, test_pack_fragment, and test_pack_rewrites

https://gist.github.com/mr-c/f2fd2a9cb60700176fe23a370d7e5edb

@mr-c
Copy link
Member

mr-c commented Apr 22, 2020

In addition, CWL v1.0/v1.1 conformance test # 54 fails: "hints_unknown_ignored: Test unknown hints are ignored"

@tom-tan
Copy link
Member Author

tom-tan commented May 1, 2020

Thank you for your comment but I close this issue because the current behavior is correct.
Sorry for my misunderstanding...

In the above example, DockerRequirementXX and args.py should be treated as warning rather than errors because hints and default fields have an attribute noLinkCheck: true in the CWL schema.
On the other hand, v1.0a should be an error because cwlVersion does not have noLinkCheck: true (I guess false is default).

The above output shows: warnings about DockerRequirementXX and args.py and an error about v1.0a and this is a correct behavior, I guess.


By the way, the current output gets us confusing a little because it seems to be a mis-ordering of errors.
It would be nice if the warnings have a prefix such as Warning: . For example:

/Users/tom-tan/.pyenv/versions/3.8.0/bin/schema-salad-tool Current version: 4.3.20190604170443
Warning: ../../error.cwl:11:5:   checking item
         ../../error.cwl:11:5:     Field `class` contains undefined reference to `file:///Users/tom-tan/DockerRequirementXX`
Warning: ../../error.cwl:40:7:   Field `location` contains undefined reference to `file:///Users/tom-tan/args.py`
Document `/Users/tom-tan/error.cwl` failed validation:
../../error.cwl:3:1: Field `cwlVersion` contains undefined reference to `file:///Users/tom-tan/v1.0a`

Any thoughts?

@tom-tan tom-tan closed this May 1, 2020
@mr-c
Copy link
Member

mr-c commented May 1, 2020

In the above example, DockerRequirementXX and args.py should be treated as warning rather than errors because hints and default fields have an attribute noLinkCheck: true in the CWL schema.

Correct

On the other hand, v1.0a should be an error because cwlVersion does not have noLinkCheck: true (I guess false is default).

Where are you seeing v1.0a ?

By the way, the current output gets us confusing a little because it seems to be a mis-ordering of errors.
It would be nice if the warnings have a prefix such as Warning: . For example:

/Users/tom-tan/.pyenv/versions/3.8.0/bin/schema-salad-tool Current version: 4.3.20190604170443
Warning: ../../error.cwl:11:5:   checking item
         ../../error.cwl:11:5:     Field `class` contains undefined reference to `file:///Users/tom-tan/DockerRequirementXX`
Warning: ../../error.cwl:40:7:   Field `location` contains undefined reference to `file:///Users/tom-tan/args.py`
Document `/Users/tom-tan/error.cwl` failed validation:
../../error.cwl:3:1: Field `cwlVersion` contains undefined reference to `file:///Users/tom-tan/v1.0a`

Any thoughts?

I like the "Warning" prefix, can you send a PR?

@tom-tan
Copy link
Member Author

tom-tan commented May 1, 2020

Where are you seeing v1.0a ?

#!/usr/bin/env cwl-runner

cwlVersion: v1.0a # Here! It will be an error because `v1.0a` is not valid

I like the "Warning" prefix, can you send a PR?

OK, I will make it!

@tom-tan tom-tan deleted the fix-warning branch May 1, 2020 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants