Evaluate requirements and hints using the correct input reference #1703

kinow · 2022-08-06T03:48:46Z

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 · 2022-08-06T04:31:56Z

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 · 2022-08-06T06:18:08Z

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 · 2022-08-08T08:22:29Z

From today's APAC-EMEA meeting:

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

kinow · 2022-08-08T11:20:29Z

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 · 2022-08-18T22:29:25Z

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 !

tests/test_reqs_hints.py

kinow · 2022-08-21T23:27:36Z

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” 👍

… inputs.

kinow · 2022-08-22T00:06:02Z

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 · 2022-08-22T08:41:28Z

cwltool/workflow.py

+            for hint_or_requirement in hints_or_requirements:
+                for key, value in hint_or_requirement.items():
+                    if key in requirements_or_hints_to_evaluate:
+                        hint_or_requirement[key] = expression.do_eval(


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.

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 · 2022-08-22T09:10:49Z

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

InitialWorkDirRequirement.listing single entries
InitialWorkDirRequirement.listing entries of type Dirent, fields entry and entryname
EnvVarRequirement 's EnvDef.envValue
ResourceRequirement fieldscoresMin, coresMax, ramMin, ramMax, tmpdirMin, tmpdirMax, outdirMin, outdirMax
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

NetworkAccess.networkAccess
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.
ToolTimeLimit.timelimit
WordReuse.enableReuse

CWL v1.2

Same as v1.1 plus

InitialWorkDirRequirement.listing itself

kinow · 2022-08-22T09:44:46Z

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.

kinow force-pushed the try-1330 branch 2 times, most recently from 722e66d to 2e75cff Compare August 6, 2022 04:20

This comment was marked as outdated.

Sign in to view

kinow marked this pull request as ready for review August 6, 2022 07:06

kinow commented Aug 6, 2022

View reviewed changes

kinow changed the title ~~Testing evaluating requirements and hints eagerly~~ Evaluating requirements and hints eagerly Aug 6, 2022

kinow changed the title ~~Evaluating requirements and hints eagerly~~ Evaluate requirements and hints eagerly Aug 6, 2022

mr-c requested a review from tetron August 6, 2022 07:55

kinow force-pushed the try-1330 branch from 10ff798 to fc2f1b8 Compare August 17, 2022 05:04

kinow mentioned this pull request Aug 17, 2022

Add conformance tests for the cwltool issue 1330 (eager vs. lazy eval of reqs and hints) common-workflow-language/cwl-v1.2#195

Closed

mr-c requested changes Aug 17, 2022

View reviewed changes

kinow force-pushed the try-1330 branch 2 times, most recently from df93a68 to d89fd84 Compare August 18, 2022 22:34

This comment was marked as outdated.

Sign in to view

kinow mentioned this pull request Aug 18, 2022

Fix codespell issues #1715

Merged

mr-c reviewed Aug 19, 2022

View reviewed changes

tests/test_reqs_hints.py Outdated Show resolved Hide resolved

kinow changed the title ~~Evaluate requirements and hints eagerly~~ Evaluate requirements and hints using the correct input reference Aug 21, 2022

kinow force-pushed the try-1330 branch from 41905ae to 63cc5b3 Compare August 21, 2022 23:26

Evaluate hints and resources at the workflow level, using the correct…

710e414

… inputs.

kinow force-pushed the try-1330 branch from 63cc5b3 to 710e414 Compare August 21, 2022 23:34

kinow marked this pull request as draft August 22, 2022 00:02

mr-c reviewed Aug 22, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Evaluate requirements and hints using the correct input reference #1703

Evaluate requirements and hints using the correct input reference #1703

kinow commented Aug 6, 2022 •

edited

Loading

codecov bot commented Aug 6, 2022 •

edited

Loading

This comment was marked as outdated.

kinow commented Aug 6, 2022

kinow left a comment •

edited

Loading

kinow commented Aug 8, 2022 •

edited

Loading

kinow commented Aug 8, 2022 •

edited

Loading

mr-c left a comment

kinow commented Aug 18, 2022

This comment was marked as outdated.

kinow commented Aug 21, 2022 •

edited

Loading

kinow commented Aug 22, 2022 •

edited

Loading

mr-c Aug 22, 2022

kinow Aug 22, 2022

mr-c commented Aug 22, 2022

kinow commented Aug 22, 2022

Evaluate requirements and hints using the correct input reference #1703

Are you sure you want to change the base?

Evaluate requirements and hints using the correct input reference #1703

Conversation

kinow commented Aug 6, 2022 • edited Loading

codecov bot commented Aug 6, 2022 • edited Loading

Codecov Report

This comment was marked as outdated.

kinow commented Aug 6, 2022

kinow left a comment • edited Loading

Choose a reason for hiding this comment

kinow commented Aug 8, 2022 • edited Loading

kinow commented Aug 8, 2022 • edited Loading

mr-c left a comment

Choose a reason for hiding this comment

kinow commented Aug 18, 2022

This comment was marked as outdated.

kinow commented Aug 21, 2022 • edited Loading

kinow commented Aug 22, 2022 • edited Loading

mr-c Aug 22, 2022

Choose a reason for hiding this comment

kinow Aug 22, 2022

Choose a reason for hiding this comment

mr-c commented Aug 22, 2022

CWL v1.0

CWL v1.1

CWL v1.2

kinow commented Aug 22, 2022

kinow commented Aug 6, 2022 •

edited

Loading

codecov bot commented Aug 6, 2022 •

edited

Loading

kinow left a comment •

edited

Loading

kinow commented Aug 8, 2022 •

edited

Loading

kinow commented Aug 8, 2022 •

edited

Loading

kinow commented Aug 21, 2022 •

edited

Loading

kinow commented Aug 22, 2022 •

edited

Loading