Evaluate requirements and hints using the correct input reference

[kinow]

Similar to #1566, with a tentative fix for the infamous #1330 🤠

Letting CI run to see what breaks after the fix, before adding tests or making further changes. If CI looks OK, and tests (new/old) pass, then it:

• Closes workflow level ResourceRequirements unable to access workflow level inputs #1330 🤞
• Supersedes and closes WIP fix #1330 #1566 too, cc @manabuishii

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.68%. Comparing base (0e2ced5) to head (710e414).
Report is 313 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (0e2ced5) and HEAD (710e414). Click for more details.

HEAD has 4 uploads less than BASE

Flag	BASE (0e2ced5)	HEAD (710e414)
	13	9

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1703       +/-   ##
===========================================
- Coverage   66.58%   54.68%   -11.90%     
===========================================
  Files          89       45       -44     
  Lines       15850     7965     -7885     
  Branches     4188     2066     -2122     
===========================================
- Hits        10553     4356     -6197     
+ Misses       4207     3086     -1121     
+ Partials     1090      523      -567     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kinow]

Conformance tests fixed locally, mypy and linter passing too. Pushed a new commit, waiting for CI. I think mypy failed on GH, but I'm hoping it's just a glitch (otherwise my environment might be using an older version?)

[kinow]

Unit & conformance tests passing 🎉

Mypy passes locally, but for some reason is failing on GH (maybe some usage limits on the server side?)

    File "/opt/hostedtoolcache/Python/3.6.15/x64/lib/python3.6/subprocess.py", line 311, in check_call
      raise CalledProcessError(retcode, cmd)
  subprocess.CalledProcessError: Command '['docker', 'pull', 'docker.io/ubuntu']' returned non-zero exit status 1.
  ERROR    cwltool:test_relocate.py:17 Workflow error, try again with --debug for more information:
  Docker is not available for this tool, try --no-container to disable Docker, or install a user space Docker replacement like uDocker with --user-space-docker-cmd.: Command '['docker', 'pull', 'docker.io/ubuntu']' returned non-zero exit status 1.

Will try to add the tests from #1566, but I believe it's ready for a review—even though I'm not sure if this is the best fix, but we can iterate and improve it as needed 👍

I tested it with a workflow 1330.cwl:

cwlVersion: v1.2
class: Workflow

hints:
  InlineJavascriptRequirement: {}
  ResourceRequirement:
    coresMax: $(inputs.threads_max)

inputs:
  threads_max:
    type: int
    default: 4

steps:
  one:
    in: []
    run:
      class: CommandLineTool
      inputs:
        other_input:
          type: int
          default: 8
      baseCommand: echo
      arguments: [ $(runtime.cores) ]
      outputs: []
    out: []

outputs: []

-Bruno

[kinow]

From today's APAC-EMEA meeting:

1. Does it work with command-line tool, expression tool? Operation is OK too?
2. Need to add to conformance tests.
3. Need to update specification (perhaps).
4. @tom-tan & @manabuishii think it's going in the right direction too 🎉

[kinow]

Does it work with command-line tool, expression tool? Operation is OK too?

It does! 🎉

I thought I would have to modify default_make_tool in other modules (for commandline tool and expression tool), but looks like that function is called for making the tool in the main.py for any of the process types we have.

I tested with the following (possible good candidates for conformance tests):

1330-wf.cwl

cwlVersion: v1.2
class: Workflow

hints:
  ResourceRequirement:
    coresMax: $(inputs.threads_max)

inputs:
  threads_max:
    type: int
    default: 4

steps:
  one:
    in: []
    run:
      class: CommandLineTool
      inputs:
        other_input:
          type: int
          default: 8
      baseCommand: echo
      arguments: [ $(runtime.cores) ]
      outputs: []
    out: []

outputs: []

1330-cmdtool.cwl

cwlVersion: v1.2
class: CommandLineTool

hints:
  ResourceRequirement:
    coresMax: $(inputs.threads_max)

inputs:
  threads_max:
    type: int
    default: 4

outputs: []

arguments: ['echo', $(runtime.cores)]

1330-expr.cwl

cwlVersion: v1.2
class: ExpressionTool

requirements:
  InlineJavascriptRequirement: {}

hints:
  ResourceRequirement:
    coresMax: $(inputs.threads_max)

inputs:
  threads_max:
    type: int
    default: 4

outputs:
  out: string

expression: |
  ${ return {"out": runtime.cores }}

Using the default job order, and providing --threads_max 1. It worked as expected 🍻

-Bruno

[mr-c]

Great to see CI passing!

Please add local copies of the suggested conformance tests that did not previously pass, and use pytest to confirm that they now pass.

I would like @tetron to review this before merging

[kinow]

Great to see CI passing!

🎉

Please add local copies of the suggested conformance tests that did not previously pass, and use pytest to confirm that they now pass.

Done!

I would like @tetron to review this before merging

👍 thanks @mr-c !

[kinow]

Rebased, and re-worded the commit that said eager, to say: “Evaluate hints and resources at the workflow level, using the correct inputs.”. The docs and functions were updated in code and tests, to say “earlier” rather than “eagerly” 👍

[kinow]

I was fixing the cwl-v1.2 repository conformance tests, following @mr-c feedback.

@mr-c, your suggestion to use EnvVarRequirement looked interesting! I thought that would simplify things, but it actually pointed to a flaw in the current fix PR.

I am evaluating the value of key: value of hints & requirements. However, that's not how cwltool handles for command line tool. Instead, it handles each requirement in a different way. For EnvVarRequirement, it will evaluate—if needed—the values of envDef's keys. For ToolTimeLimit it will evaluate the timelimit attribute, and so it goes.

I think the proper fix would have to allow the correct inputs object to be referenced in any requirement, regardless whether it's a ResourceRequirement or any other type.

The only blocker to making this change, is that the logic to evaluate requirements resides in command_line_tool.py,

cwltool/cwltool/command_line_tool.py

Line 1196 in 0e2ced5

env_value_eval = builder.do_eval(env_value_field)

. I couldn't figure out an easy way to re-use the same logic in workflow.py or workflow_job.py without duplicating code. Maybe these could be moved to a separate function.

Moving it back to draft.

[mr-c]

Ah, now I see the challenge you uncovered. This is evaluating CWL expressions in every key (but not sub-key) of every requirement/hint. Not only is it missing nested fields that should be evaluated, it is evaluating fields that should NOT be evaluated! Only fields where Expression appears in the type definition are to be evaluated.

[kinow]

Exactly. I was happy and sad at the same time when I realized that 🤣 Happy we found it before it was merged, sad (in a good nerdy-way!) that it doesn't seem to be an easy fix.

[mr-c]

Review of CWL requirement/hint fields that can contain CWL expressions (inspired by the incomplete list at https://www.commonwl.org/user_guide/13-expressions/index.html )

CWL v1.0

1. InitialWorkDirRequirement.listing single entries
2. InitialWorkDirRequirement.listing entries of type Dirent, fields entry and entryname
3. EnvVarRequirement 's EnvDef.envValue
4. ResourceRequirement fieldscoresMin, coresMax, ramMin, ramMax, tmpdirMin, tmpdirMax, outdirMin, outdirMax
5. SchemaDefRequirement.types can can type definitions with inputBindings (defined in CommandLineBinding) that themselves can have CWL expressions in the valueFrom field. Unclear if a workflow-level SchemaDefRequirement is used, then which inputs object is presented to these expressions? Likewise for the use of format fields that use CWL expressions.

CWL v1.1

Same as v1.0 plus

6. NetworkAccess.networkAccess
7. SchemaDefRequirement.types can can type definitions with inputBindings (defined in CommandLineBinding) that themselves can have CWL expressions also in the position field in addition to the valueFrom field. The type definitions could also use a CWL expression in the secondaryFiles field.
8. ToolTimeLimit.timelimit
9. WordReuse.enableReuse

CWL v1.2

Same as v1.1 plus

10. InitialWorkDirRequirement.listing itself

[kinow]

Review of CWL requirement/hint fields that can contain CWL expressions (inspired by the incomplete list at https://www.commonwl.org/user_guide/13-expressions/index.html )

Thanks!

It's not hard to review the existing code and confirm these rules/cases match, or if there are any missing. I think the hard part is figuring out a way to re-use that in another context (when evaluating the workflow or workflow steps). Will have a go after the user guide, but happy if anyone beats me to it.