WIP fix #1330 #1566

manabuishii · 2021-11-11T03:20:08Z

At least workflow-dyn-res.cwl does not create error.

But this PR fail some tests.

(venv) ➜  cwltool git:(fix_issue_1330) ✗ python3 -m cwltool workflow-dyn-res.cwl         
INFO /Users/manabu/ghq/github.com/common-workflow-language/cwltool/cwltool/__main__.py 3.1.20211107152837
INFO Resolved 'workflow-dyn-res.cwl' to 'file:///Users/manabu/ghq/github.com/common-workflow-language/cwltool/workflow-dyn-res.cwl'
INFO [workflow ] start
INFO [workflow ] starting step one
INFO [step one] start
INFO [job one] /private/tmp/docker_tmpduee6x2k$ echo \
    4
4
INFO [job one] completed success
INFO [step one] completed success
INFO [workflow ] completed success
{
    "file:///Users/manabu/ghq/github.com/common-workflow-language/cwltool/workflow-dyn-res.cwl#threads_max": 4
}
INFO Final process status is success

Strategy

( I think below, but need to know more about inside)

To resolve ResourceRequirement value,
First,
I bring Workflow inputs to steps inputs,( see TODO )

Second,
When evalResources is called, resource resolved.

Third,
After the call, CommandLineTool's inputs are rewrite to remove inputs for ResourceRequirement.

TODO:

Add workflow-dyn-res.cwl to tests
Discuss about adding workflow-dyn-res.cwl related tests to at least unit test or mypy test or both?
Check other path ways call evalResourcesare exists or not .
- I think the self.tool['class'] is not 'Workflow' are called evalResources .
- cwltool/cwltool/process.py
  
  Lines 973 to 974 in 898fddf
  
  if self.tool["class"] != "Workflow":
  
  builder.resources = self.evalResources(builder, runtime_context)
- I put the code just below this line, but some tests result are failed. But sometimes my test environment is strange behavior, I should check again.
Check other behavior which I do not know now. All tests are passed but I think need to care.
Check nested nested (n nested) Workflow behavior.

kinow

@manabuishii impressive work tracking down where you had to add the state/job info 👏

I think what this change is doing is making the job info from the parent Job available to children Jobs. Not sure if there's any downsides to this.

Maybe another approach could be to modify Process#evalResources and rewrite the Resource value, evaluating the expression to 4. This way when children Job are created they wouldn't have to try to resolve the variables/values.

cwltool/process.py

kinow · 2022-03-29T01:52:37Z

cwltool/workflow_job.py

@@ -386,6 +386,8 @@ def object_from_state(
    incomplete: bool = False,
 ) -> Optional[CWLObjectType]:
    inputobj = {}  # type: CWLObjectType
+    for s in state.keys():
+        inputobj[s] = state[s].value


I think we can avoid accessing keys & values separately. Something like:

for k, v in state.items(): inputobj[k] = v

Or start inputobj by deep-cloning state.

Current commited version is
https://github.com/common-workflow-language/cwltool/pull/1566/files#diff-ed87d61e2237460638e7f3ddb21bafc9969d8c142c804d79f94801142342f3d6R389-R391

My local version is following .
It handles 1330 well and pass py39-unit
But I can not pass py39-mypy

for s in state.keys(): if hasattr(state[s], "value"): inputobj[s] = state[s].value

Now I try to replace it to your code, because it looks good.
But I can not pass py39-mypy

for k,v in state.items(): if type(v) is WorkflowStateItem: if hasattr(v, "value"): inputobj[k] = v.value

currently

kinow · 2022-03-29T05:57:27Z

Maybe another approach could be to modify Process#evalResources and rewrite the Resource value, evaluating the expression to 4. This way when children Job are created they wouldn't have to try to resolve the variables/values.

Hmm, interesting. Tried that, but realized that process.py intentionally skips evaluating the resources for the top level job (the Workflow itself is the parent job).

🤷‍♂️ there must be a good reason for doing so, so I think your approach should be the right fix @manabuishii 👍 let's see what's wrong with the build...

mr-c · 2022-03-29T16:57:01Z

@manabuishii Can you fix the merge conflict so that the CI can run?

cwltool/workflow_job.py

codecov · 2022-03-30T23:13:13Z

Codecov Report

Merging #1566 (14a2ded) into main (7a058fe) will increase coverage by 0.02%.
The diff coverage is 82.69%.

@@            Coverage Diff             @@
##             main    #1566      +/-   ##
==========================================
+ Coverage   66.82%   66.84%   +0.02%     
==========================================
  Files          93       93              
  Lines       16578    16784     +206     
  Branches     4404     4447      +43     
==========================================
+ Hits        11078    11220     +142     
- Misses       4361     4396      +35     
- Partials     1139     1168      +29

Impacted Files	Coverage Δ
cwltool/command_line_tool.py	`77.90% <81.48%> (+0.43%)`	⬆️
cwltool/workflow_job.py	`60.80% <81.81%> (+1.02%)`	⬆️
cwltool/workflow.py	`85.82% <100.00%> (+0.16%)`	⬆️
cwltool/task_queue.py	`73.77% <0.00%> (-3.28%)`	⬇️
cwltool/cwltool/argparser.py	`72.11% <0.00%> (-2.68%)`	⬇️
cwltool/sandboxjs.py	`72.63% <0.00%> (-1.97%)`	⬇️
cwltool/cwltool/sandboxjs.py	`54.73% <0.00%> (-1.48%)`	⬇️
cwltool/cwltool/cwlviewer.py	`21.97% <0.00%> (-1.02%)`	⬇️
cwltool/utils.py	`79.33% <0.00%> (-0.37%)`	⬇️
cwltool/main.py	`71.71% <0.00%> (-0.27%)`	⬇️
... and 16 more

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 7a058fe...14a2ded. Read the comment docs.

kinow · 2022-03-30T23:21:49Z

Getting closer to a green build! 🚀

manabuishii · 2022-03-31T03:55:45Z

@kinow @mr-c

This commit is done by me, but account is not current my account .

a7a66cf

Is there any good way to rewrite my commit ?
Or Can I leave it as it is?

kinow · 2022-03-31T04:36:06Z

@manabuishii the commit will be squashed when we merge the PR (it has 6 commits, ideally a single commit is best, or grouped commits if they are helpful for future devs.)

Or if you prefer to edit the commit now and change the author, you can use interactive rebase or amend (if the last commit), e.g. https://stackoverflow.com/a/3042512

manabuishii · 2022-03-31T08:33:12Z

@kinow Thanks !!
I choose to squash when sending pull requests .

manabuishii · 2022-03-31T09:23:04Z

@kinow @mr-c
How do I execute following test ( that fails GitHub Actions)

Continuous integration tests / CWL spec conformance tests (v1.0, podman) (pull_request)

Is this correct ?

source venv/bin/activate
container=podman spec_branch=main version=v1.0 ./conformance-test.sh

In my linux machine, above command is take long time.

I want to fix the error which GitHub Actions says.
Best case is execute only failed test first, and if it is passed, execute all.

mr-c · 2022-03-31T09:42:33Z

You can run just the test that failed

https://github.com/common-workflow-language/cwltool/runs/5765165834?check_suite_focus=true#step:5:490

cwltool --parallel --podman --debug https://github.com/common-workflow-language/common-workflow-language/raw/main/v1.0/v1.0/fail-unconnected.cwl

Note that this test should fail: https://github.com/common-workflow-language/common-workflow-language/blob/a62f91e3d75036e7d89dbd454fc4e1f72caf901f/v1.0/conformance_test_v1.0.yaml#L1863

So if this PR causes this to not fail, then that is a problem 🙂

manabuishii · 2022-03-31T13:36:45Z

My current strategy is such that I make everything a global variable, so
It appears that the scope of the variable is not properly controlled.

Therefore, the

workflow-dyn-res.cwl also does not deal with input.cores.
It is looking at inputs.threads_max, which it was forced to look at as a global variable.

Essentialy , inputs.threads_max is first set to coresMax
And the coresMax is reflected in runtime.cores

I think runtime.cores is necessary to put it in arguments of echo.

So this is the reason why cwltool --parallel --podman --debug https://github.com/common-workflow-language/common-workflow-language/raw/main/v1.0/v1.0/fail-unconnected.cwl is not failed, I think.

I need to change strategy.

manabuishii · 2022-04-01T09:17:36Z

Memo:
threads_max is required at evalResources in process.py to make coresMax and runtime.core
After using threads_max in evalResources of process.py, I test builder.job(in command_line_tool.py) which is copy of job_order , workflow-dyn-res.cwl and fail-unconnected.cwl is behaved expected.
Of course other unit tests are broken.

manabuishii · 2022-04-03T08:19:06Z

@mr-c I think this PullRequest fix #1330.
All tests are passed.

Now I want to check that there is no unwanted behavior .

I want to ask questions related to fail-unconnected.cwl

Question 1: What kind of output is expected for ExpressionTool instead of CommandLineTool ?

fail-unconnected-expressiontool.cwl is following

class: Workflow
cwlVersion: v1.1
inputs:
  inp1:
    type: string
    default: hello inp1
  inp2:
    type: string
    default: hello inp2
outputs:
  out:
    type: string
    outputSource: step1/out
steps:
  step1:
    in:
      in: inp1
      in2: inp2
    out: [out]
    run: fail-unspecified-input-expressiontool.cwl

and

fail-unspecified-input-expressiontool.cwl is following

cwlVersion: v1.0
class: ExpressionTool

requirements:
  InlineJavascriptRequirement: {} 

inputs:
  in:
    type: string

outputs:
  out:
    type: string

expression: |
  ${ 
    return {"out": inputs.in +" "+inputs.in2};
  }

Both
current HEAD (7a058fe)
and this PullRequest return same

Is this result correct ?

python3 -m cwltool fail-unconnected-expressiontool.cwl
INFO /home/manabu/ghq/github.com/common-workflow-language/cwltool/cwltool/__main__.py 3.1.20220325085616
INFO Resolved 'fail-unconnected-expressiontool.cwl' to 'file:///home/manabu/ghq/github.com/common-workflow-language/cwltool/fail-unconnected-expressiontool.cwl'
WARNING Workflow checker warning:
fail-unconnected-expressiontool.cwl:18:7: 'in2' is not an input parameter of
                                          file:///home/manabu/ghq/github.com/common-workflow-language/cwltool/fail-unspecified-input-expressiontool.cwl,
                                          expected in
INFO [workflow ] start
INFO [workflow ] starting step step1
INFO [step step1] start
INFO [step step1] completed success
INFO [workflow ] completed success
{
    "out": "hello inp1 undefined"
}
INFO Final process status is success

As WARNING messages indicates , fail-unconnected-expressiontool.cwl does not have in2 as input parameter.
Then result indiates inputs.in2 is evaluated to undefined.

When CommandLineTool case is error, but ExpressionTool case is warning .
So I want to ask correct behavior.

Question2: What kind of output is expected for Operation instead of CommandLineTool ?
I think CWL version 1.2 has Operation .
But I do not know how to write it for tests.

tom-tan · 2022-04-03T09:17:30Z

Question 1: What kind of output is expected for ExpressionTool instead of CommandLineTool ?
Question2: What kind of output is expected for Operation instead of CommandLineTool ?

I guess the both cases must fail.

The spec of WorkflowStep.in says:

Input parameters include a schema for each parameter which is used to validate the input object.

But in2 has no corresponding schema to be used for validation.

mr-c · 2022-04-05T13:48:45Z

cwltool/schemas/v1.1/conformance_tests.yaml

+
+- job: tests/empty.json
+  tool: tests/fail-unconnected-expressiontool.cwl
+  label: wf_step_access_undeclared_param_expressiontool
+  id: 198
+  doc: >-
+    Test that parameters that don't appear in the `run` process
+    inputs are not present in the input object used to run the expressiontool.
+  should_fail: true
+  tags: [ required, workflow ]
+


The CI doesn't run this copy of the tests, it makes a new checkout from GitHub

cwltool/conformance-test.sh

Line 41 in 7a058fe

wget "https://github.com/common-workflow-language/${repo}/archive/${spec_branch}.tar.gz"

@mr-c Thanks

I will create a PullRequest about this test case (tests/fail-unconnected-expressiontool.cwl),

Repo:
https://github.com/common-workflow-language/cwl-v1.2
Branch:
1.2.1_proposed

Thanks, you can also keep it in this PR, just move the test scripts to the the test directory and add an entry to test_examples.py

Can you move this file to the tests directory, or a sub-directory?

Original PullRequest common-workflow-language/cwltool#1566 Original Issue common-workflow-language/cwltool#1330

It is not passed parameters which is not pass to ExpressionTool

manabuishii · 2022-06-30T08:49:21Z

I improve ExpressionTool inputs handling.

This does not cause error.
It becomes undefined.

First, currently

in is resolved as inp1
in2 is not resolved.

I think this is expected behavior for following class ExpressionTool

https://github.com/common-workflow-language/cwltool/pull/1566/files#diff-3d8236842572888947227e8925a930cebe33d4af9c6bb8abc61282eec6d105c1R17

Just before exec_js_process

fn is following.
inputs member has only one in.

'"use strict";
var inputs = {
    "in": "hello inp1"
};
var self = null;
var runtime = {
    "cores": 1,
    "ram": 1024,
    "tmpdirSize": 1024,
    "outdirSize": 1024,
    "tmpdir": null,
    "outdir": null
};
(function(){ 
  return {"out": inputs.in +" "+inputs.in2};
})()'

function executed,
inputs.in is hello inp1
inputs.in2 is undefined because inputs.in2 is undefined.

Output is here

$ python3 -m cwltool tests/fail-unconnected-expressiontool.cwl 
INFO /Users/manabu/ghq/github.com/common-workflow-language/cwltool/cwltool/__main__.py 3.1.20211111031309
INFO Resolved 'tests/fail-unconnected-expressiontool.cwl' to 'file:///Users/manabu/ghq/github.com/common-workflow-language/cwltool/tests/fail-unconnected-expressiontool.cwl'
WARNING Workflow checker warning:
tests/fail-unconnected-expressiontool.cwl:18:7: 'in2' is not an input parameter of
                                                file:///Users/manabu/ghq/github.com/common-workflow-language/cwltool/tests/fail-unspecified-input-expressiontool.cwl,
                                                expected in
INFO [workflow ] start
INFO [workflow ] starting step step1
INFO [step step1] start
INFO [step step1] completed success
INFO [workflow ] completed success
{
    "out": "hello inp1 undefined"
}
INFO Final process status is success

I investigate related JS behavior.

When inputs is declared as dictionary.
inputs.in2 returns undefined.
Just in2 is cause error because in2 is not defined.

> inputs={}
> inputs.in = "hello inp1"
> inputs.in2
undefined
> in2
VM1960:1 Uncaught ReferenceError: in2 is not defined
    at <anonymous>:1:1

I think that in2 is resolved as undefined and it is correct behavior at least some Javascript Implementation.

manabuishii · 2022-06-30T08:53:11Z

Until previous commit 14a2ded
in2 is resolved as hello inp2

so output is following.
At least this result is wrong.

$ python3 -m cwltool tests/fail-unconnected-expressiontool.cwl
INFO /Users/manabu/ghq/github.com/common-workflow-language/cwltool/cwltool/__main__.py 3.1.20211111031309
INFO Resolved 'tests/fail-unconnected-expressiontool.cwl' to 'file:///Users/manabu/ghq/github.com/common-workflow-language/cwltool/tests/fail-unconnected-expressiontool.cwl'
WARNING Workflow checker warning:
tests/fail-unconnected-expressiontool.cwl:18:7: 'in2' is not an input parameter of
                                                file:///Users/manabu/ghq/github.com/common-workflow-language/cwltool/tests/fail-unspecified-input-expressiontool.cwl,
                                                expected in
INFO [workflow ] start
INFO [workflow ] starting step step1
INFO [step step1] start
INFO [step step1] completed success
INFO [workflow ] completed success
{
    "out": "hello inp1 hello inp2"
}
INFO Final process status is success

manabuishii · 2022-07-06T07:42:34Z

I think current this PR approach is Lazy evaluation.
But implementation is difficult.
In my opinion, Eager evaluation will be more clear and easy to understand and easy to implement.

How do you think ?

kinow · 2022-07-11T08:09:03Z

In my opinion, Eager evaluation will be more clear and easy to understand and easy to implement.

I haven't dug really deep in the issue as @manabuishii , but from what I could see, I also think that eager evaluation in this case would be better for cwltool.

As for specification, I think we can leave it as-is, or maybe add a comment that eager or lazy evaluation is up to implementers, but not enforced by the specification.

manabuishii · 2022-07-11T12:26:32Z

@kinow Thanks !!

As for specification, I think we can leave it as-is, or maybe add a comment that eager or lazy evaluation is up to implementers, but not enforced by the specification.

I consider about multi cloud situation (including on-premise cloud and some on-premise cluster) sometimes lazy evaluations require variable or file which is not accessible from some environment.
Maybe it is related to acquire for the machine resources to distributed multi computing environments (multi-cloud).

To consider such a situation , I think we keep discuss and investigate about pros, cons and reasonable assumption.
But during the discussion I agree that "we can leave it as-is".
Mainly I care about the difference between implementations if the specification is said nothing.(Sorry not so much deeply about "as-is" but I feel it's okay because it is current state.)

One more option is that spec said nothing but implementation is accept variable about eager or lazy evaluation.

But I want to fix #1330 😄

tom-tan · 2022-07-12T07:18:21Z

I agree with @kinow that we leave the spec as is (at least in v1.2.0) because clarifying the evaluation strategy may break existing platforms.
IMO it is better to fix it in the later release of CWL by clarifying the default evaluation strategy and by extending the spec to support use cases for eager and lazy strategies.

I prefer the eager strategy by default because it makes much easier to implement platforms.
I proposed a tentative extension to cover the use cases for lazy strategy in another issue.

Original PullRequest common-workflow-language/cwltool#1566 Original Issue common-workflow-language/cwltool#1330

WIP fix #1330

fbed7db

kinow reviewed Mar 29, 2022

View reviewed changes

manabuishii added 2 commits March 30, 2022 01:40

Revert cwltool/process.py

2414df9

Update to fix #1330

1862895

manabuishii added 2 commits March 30, 2022 10:09

Merge branch 'main' into fix_issue_1330

40a22d8

Fix lint problem

59c97cf

mr-c reviewed Mar 30, 2022

View reviewed changes

cwltool/workflow_job.py Outdated Show resolved Hide resolved

cwltool/workflow_job.py Outdated Show resolved Hide resolved

Fix for mypy

a7a66cf

Fix podman v1.0 error

8031b3d

Remove items for evalResources

a6caace

mr-c requested a review from tetron April 3, 2022 10:09

Add expressiontool conformance test case

8c08318

mr-c reviewed Apr 5, 2022

View reviewed changes

manabuishii pushed a commit to common-workflow-language/cwl-v1.2 that referenced this pull request Apr 6, 2022

Add test case for fail uncconected ExpressionTool

94638bf

Original PullRequest common-workflow-language/cwltool#1566 Original Issue common-workflow-language/cwltool#1330

manabuishii mentioned this pull request Apr 6, 2022

Add test case for fail uncconected ExpressionTool common-workflow-language/cwl-v1.2#156

Closed

manabuishii added a commit to common-workflow-language/cwl-v1.2 that referenced this pull request Apr 6, 2022

Add test case for fail uncconected ExpressionTool

e5c23e6

Original PullRequest common-workflow-language/cwltool#1566 Original Issue common-workflow-language/cwltool#1330

Improve ExpressionTool inputs

14a2ded

It is not passed parameters which is not pass to ExpressionTool

kinow mentioned this pull request Aug 6, 2022

Evaluate requirements and hints using the correct input reference #1703

Draft

mr-c pushed a commit to common-workflow-language/cwl-v1.2 that referenced this pull request Aug 17, 2023

Add test case for fail uncconected ExpressionTool

36ec018

Original PullRequest common-workflow-language/cwltool#1566 Original Issue common-workflow-language/cwltool#1330

mr-c pushed a commit to common-workflow-language/cwl-v1.2 that referenced this pull request Aug 17, 2023

Add test case for fail uncconected ExpressionTool

4d8442a

Original PullRequest common-workflow-language/cwltool#1566 Original Issue common-workflow-language/cwltool#1330

mr-c pushed a commit to common-workflow-language/cwl-v1.2 that referenced this pull request Aug 17, 2023

Add test case for fail uncconected ExpressionTool

f56e4d7

Original PullRequest common-workflow-language/cwltool#1566 Original Issue common-workflow-language/cwltool#1330

mr-c pushed a commit to common-workflow-language/cwl-v1.2 that referenced this pull request Aug 17, 2023

Add test case for fail uncconected ExpressionTool

fa30365

Original PullRequest common-workflow-language/cwltool#1566 Original Issue common-workflow-language/cwltool#1330

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP fix #1330 #1566

WIP fix #1330 #1566

manabuishii commented Nov 11, 2021 •

edited

Loading

kinow left a comment

kinow Mar 29, 2022

manabuishii Mar 30, 2022 •

edited

Loading

kinow commented Mar 29, 2022 •

edited

Loading

mr-c commented Mar 29, 2022

codecov bot commented Mar 30, 2022 •

edited

Loading

kinow commented Mar 30, 2022

manabuishii commented Mar 31, 2022

kinow commented Mar 31, 2022

manabuishii commented Mar 31, 2022

manabuishii commented Mar 31, 2022 •

edited

Loading

mr-c commented Mar 31, 2022 •

edited

Loading

manabuishii commented Mar 31, 2022

manabuishii commented Apr 1, 2022

manabuishii commented Apr 3, 2022

tom-tan commented Apr 3, 2022 •

edited

Loading

mr-c Apr 5, 2022

manabuishii Apr 6, 2022 •

edited

Loading

mr-c Apr 6, 2022

mr-c Jun 2, 2022

manabuishii commented Jun 30, 2022

manabuishii commented Jun 30, 2022 •

edited

Loading

manabuishii commented Jul 6, 2022

kinow commented Jul 11, 2022

manabuishii commented Jul 11, 2022

tom-tan commented Jul 12, 2022

	if self.tool["class"] != "Workflow":
	builder.resources = self.evalResources(builder, runtime_context)

WIP fix #1330 #1566

Are you sure you want to change the base?

WIP fix #1330 #1566

Conversation

manabuishii commented Nov 11, 2021 • edited Loading

Strategy

TODO:

kinow left a comment

Choose a reason for hiding this comment

kinow Mar 29, 2022

Choose a reason for hiding this comment

manabuishii Mar 30, 2022 • edited Loading

Choose a reason for hiding this comment

kinow commented Mar 29, 2022 • edited Loading

mr-c commented Mar 29, 2022

codecov bot commented Mar 30, 2022 • edited Loading

Codecov Report

kinow commented Mar 30, 2022

manabuishii commented Mar 31, 2022

kinow commented Mar 31, 2022

manabuishii commented Mar 31, 2022

manabuishii commented Mar 31, 2022 • edited Loading

mr-c commented Mar 31, 2022 • edited Loading

manabuishii commented Mar 31, 2022

manabuishii commented Apr 1, 2022

manabuishii commented Apr 3, 2022

tom-tan commented Apr 3, 2022 • edited Loading

mr-c Apr 5, 2022

Choose a reason for hiding this comment

manabuishii Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

mr-c Apr 6, 2022

Choose a reason for hiding this comment

mr-c Jun 2, 2022

Choose a reason for hiding this comment

manabuishii commented Jun 30, 2022

manabuishii commented Jun 30, 2022 • edited Loading

manabuishii commented Jul 6, 2022

kinow commented Jul 11, 2022

manabuishii commented Jul 11, 2022

tom-tan commented Jul 12, 2022

manabuishii commented Nov 11, 2021 •

edited

Loading

manabuishii Mar 30, 2022 •

edited

Loading

kinow commented Mar 29, 2022 •

edited

Loading

codecov bot commented Mar 30, 2022 •

edited

Loading

manabuishii commented Mar 31, 2022 •

edited

Loading

mr-c commented Mar 31, 2022 •

edited

Loading

tom-tan commented Apr 3, 2022 •

edited

Loading

manabuishii Apr 6, 2022 •

edited

Loading

manabuishii commented Jun 30, 2022 •

edited

Loading