-
-
Notifications
You must be signed in to change notification settings - Fork 237
Resource requirement min max validation #2179
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
base: main
Are you sure you want to change the base?
Resource requirement min max validation #2179
Conversation
mr-c
left a comment
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.
Thank you @Stellatsuu !
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2179 +/- ##
===========================================
- Coverage 85.12% 55.48% -29.65%
===========================================
Files 46 46
Lines 8368 8383 +15
Branches 1956 1961 +5
===========================================
- Hits 7123 4651 -2472
- Misses 779 3230 +2451
- Partials 466 502 +36 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| "file", ["tests/wf/bad_resreq_mnmx_clt.cwl", "tests/wf/bad_resreq_mnmx_wf.cwl"] | ||
| ) |
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.
the CWL descriptions need inputs
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.
It was a bad copypaste of mine, I only took a workflow from the test folder and modified it, forgot about the inputs.
Since we want to focus on requirements validation, I changed both workflows to be only helloworld without inputs or outputs but with invalid requirements.
Would it work like this? Or maybe I did not understand your review.
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.
I was explaining why the tests failed :-)
tests/wf/bad_resreq_mnmx_wf.cwl still requires inputs; you need to either provide them as a separate file, or also modify that CWL description to not need inputs provided (perhaps via default values).
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.
Default inputs are required even in a workflow like this one where inputs and outputs are empty and not used?
#!/usr/bin/env cwl-runner
class: Workflow
cwlVersion: v1.2
requirements:
ResourceRequirement:
ramMin: 128
ramMax: 64
inputs: []
outputs: []
steps:
hello_world:
requirements:
ResourceRequirement:
ramMin: 64
ramMax: 128
run:
class: CommandLineTool
baseCommand: [ "echo", "Hello World" ]
inputs: [ ]
outputs: [ ]
out: [ ]
in: [ ]
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.
Would this be a correct workflow?
#!/usr/bin/env cwl-runner
class: Workflow
cwlVersion: v1.2
requirements:
ResourceRequirement:
ramMin: 128
ramMax: 64
inputs:
message: string?
outputs: []
steps:
hello_world:
requirements:
ResourceRequirement:
ramMin: 64
ramMax: 128
run:
class: CommandLineTool
baseCommand: echo
inputs:
message:
type: string?
default: "Hello World"
outputs: []
arguments:
- $(inputs.message)
in:
message: message
out: []
… only on requirements
… ResourceRequirement-MinMax
|
I looked at the code, and it seems that the main reason is that, def Process(...):
def _init_job(...):
...
if self.tool["class"] != "Workflow":
builder.resources = self.evalResources(builder, runtime_context)
return builder
def evalResources(...):
-validation is here-How should I manage to check if the resource requirements of a Workflow are correct when calling |
Related to : #2163
--validate