-
-
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2179 +/- ##
==========================================
+ Coverage 84.85% 84.92% +0.07%
==========================================
Files 46 46
Lines 8509 8523 +14
Branches 1984 1988 +4
==========================================
+ Hits 7220 7238 +18
- Misses 812 814 +2
+ Partials 477 471 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
… 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 |
|
Hello @mr-c, Thank you in advance |
|
Sorry for the delay, @Stellatsuu , I missed your original question; so thank you for the reminder 5f455a4 is where that variation was added, as part of implementing |
| for a in ("cores", "ram", "tmpdir", "outdir"): | ||
| mn = cast(Union[int, float, None], requirement.get(a + "Min")) | ||
| mx = cast(Union[int, float, None], requirement.get(a + "Max")) | ||
| if mn is not None and mx is not None and mx < mn: | ||
| raise ValidationException(f"{a}Min cannot be greater than {a}Max.") |
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.
This doesn't work in the case of a CWL Expression or Parameter Reference (type str).
Either
A. Skip strings here
or
B. move the call to this function to later, when a builder object is available to utilize builder.do_eval to get the actual 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.
Hello @mr-c,
I removed the cast here so that string values are skipped. This part of the code is only executed when running cwltool --validate. The actual runtime validation is performed later in builder.resources = self.evalResources(builder, runtime_context), so Expressions should work here.
My question is: if we want to --validate a requirement that contains a CWL expression, we first need to evaluate the expression to get an int or float before running the validation. But since evaluating expressions requires a Builder, how could this be done during validation? Validation does not use builder right?
Related to : #2163
--validate