-
-
Notifications
You must be signed in to change notification settings - Fork 231
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
Evaluate requirements and hints using the correct input reference #1703
Draft
kinow
wants to merge
1
commit into
common-workflow-language:main
Choose a base branch
from
kinow:try-1330
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
"""Test for Requirements and Hints in cwltool.""" | ||
import json | ||
from io import StringIO | ||
|
||
from cwltool.main import main | ||
from .util import get_data | ||
|
||
|
||
def test_workflow_reqs_are_evaluated_earlier_default_args() -> None: | ||
"""Test that a Workflow process will evaluate the requirements earlier. | ||
|
||
Uses the default input values. | ||
|
||
This means that workflow steps, such as Expression and Command Line Tools | ||
can both use resources without re-evaluating expressions. This is useful | ||
when you have an expression that, for instance, dynamically decides | ||
how many threads/cpus to use. | ||
|
||
Issue: https://github.com/common-workflow-language/cwltool/issues/1330 | ||
""" | ||
stream = StringIO() | ||
|
||
assert ( | ||
main( | ||
[get_data("tests/wf/1330.cwl")], | ||
stdout=stream, | ||
) | ||
== 0 | ||
) | ||
|
||
out = json.loads(stream.getvalue()) | ||
assert out["out"] == "2\n" | ||
|
||
|
||
def test_workflow_reqs_are_evaluated_earlier_provided_inputs() -> None: | ||
"""Test that a Workflow process will evaluate the requirements earlier. | ||
|
||
Passes inputs via a job file. | ||
|
||
This means that workflow steps, such as Expression and Command Line Tools | ||
can both use resources without re-evaluating expressions. This is useful | ||
when you have an expression that, for instance, dynamically decides | ||
how many threads/cpus to use. | ||
|
||
Issue: https://github.com/common-workflow-language/cwltool/issues/1330 | ||
""" | ||
stream = StringIO() | ||
|
||
assert ( | ||
main( | ||
[get_data("tests/wf/1330.cwl"), get_data("tests/wf/1330.json")], | ||
stdout=stream, | ||
) | ||
== 0 | ||
) | ||
|
||
out = json.loads(stream.getvalue()) | ||
assert out["out"] == "1\n" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
# Original file: tests/eager-eval-reqs-hints/wf-reqs.cwl | ||
# From: https://github.com/common-workflow-language/cwl-v1.2/pull/195 | ||
cwlVersion: v1.2 | ||
class: Workflow | ||
|
||
requirements: | ||
ResourceRequirement: | ||
coresMax: $(inputs.threads_max) | ||
|
||
inputs: | ||
threads_max: | ||
type: int | ||
default: 2 | ||
|
||
steps: | ||
one: | ||
in: [] | ||
run: | ||
class: CommandLineTool | ||
inputs: | ||
other_input: | ||
type: int | ||
default: 8 | ||
baseCommand: echo | ||
arguments: [ $(runtime.cores) ] | ||
stdout: out.txt | ||
outputs: | ||
out: | ||
type: string | ||
outputBinding: | ||
glob: out.txt | ||
loadContents: true | ||
outputEval: $(self[0].contents) | ||
out: [out] | ||
|
||
outputs: | ||
out: | ||
type: string | ||
outputSource: one/out |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{ | ||
"threads_max": 1 | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.