Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions cwl_utils/parser/cwl_v1_2.py
Original file line number Diff line number Diff line change
Expand Up @@ -18131,6 +18131,14 @@ def fromDoc(
"is not valid because:",
)
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for prototyping the validation. This file is autogenerated, so any changes here would get overwritten.

https://github.com/common-workflow-language/cwl-utils/blob/4a8390d897913c3776a0d2b2a01e1e6134b968bc/cwl_utils/parser/cwl_v1_2.py#L2

cwl-utils/Makefile

Lines 191 to 209 in 4a8390d

cwl_utils/parser/cwl_v1_0.py: FORCE
schema-salad-tool --codegen python \
--codegen-parser-info "org.w3id.cwl.v1_0" \
https://github.com/common-workflow-language/common-workflow-language/raw/codegen/v1.0/extensions.yml \
> $@
cwl_utils/parser/cwl_v1_1.py: FORCE
schema-salad-tool --codegen python \
--codegen-parser-info "org.w3id.cwl.v1_1" \
https://github.com/common-workflow-language/cwl-v1.1/raw/codegen/extensions.yml \
> $@
cwl_utils/parser/cwl_v1_2.py: FORCE
schema-salad-tool --codegen python \
--codegen-parser-info "org.w3id.cwl.v1_2" \
https://github.com/common-workflow-language/cwl-v1.2/raw/codegen/extensions.yml \
> $@
regen_parsers: cwl_utils/parser/cwl_v1_*.py

Additional validation logic should be added elsewhere, perhaps cwl_utils/parser/utils.py

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, my bad on that! I didn't see it was autogenerated, those comments were hidden on my IDE.
I will look to add this validation in cwltool only then, since you mentioned that there isn't much validation code to add in cwl-utils 🙂

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any documentation to onboard developers?
I am also trying to understand the workflow to see where we should add that piece of logic. From what I understand:

  • You define the CWL specifications in these repos: cwl 1.3, cwl 1.2, ...
  • You are using schema_salad to convert the CWL specifications into python code in cwl-utils
  • So cwl-utils is the repository we should use to manipulate CWL workflows in Python
  • And then on top of that there is cwltool, used for the execution of CWL workflows

IIUC, checks should generally be included in the CWL specifications in JSON/YAML; But the structure would not enable the comparison of 2 attributes like minCores and maxCores for instance.
So this would need to be added in the cwl_utils/parser/utils.py module.

From here - and if my understanding has been correct so far - I am a bit confused. I don't see where these functions are used. They don't seem to appear in the generated parser files, and in cwltool I see functions with the same names but they are redefined. Example:

Ideally I think we would love to have this min-max validation when we call load_document.

Thank you very much for you guidance

if coresMin is not None and coresMax is not None and coresMin > coresMax:
_errors__.append(
ValidationException(
"the `coresMin` field is greater than the `coresMax` field",
None
)
)

ramMin = None
if "ramMin" in _doc:
try:
Expand Down Expand Up @@ -18225,6 +18233,14 @@ def fromDoc(
"is not valid because:",
)
)
if ramMin is not None and ramMax is not None and ramMin > ramMax:
_errors__.append(
ValidationException(
"the `ramMin` field is greater than the `ramMax` field",
None
)
)

tmpdirMin = None
if "tmpdirMin" in _doc:
try:
Expand Down Expand Up @@ -18319,6 +18335,14 @@ def fromDoc(
"is not valid because:",
)
)
if tmpdirMin is not None and tmpdirMax is not None and tmpdirMin > tmpdirMax:
_errors__.append(
ValidationException(
"the `tmpdirMin` field is greater than the `tmpdirMax` field",
None
)
)

outdirMin = None
if "outdirMin" in _doc:
try:
Expand Down Expand Up @@ -18413,6 +18437,14 @@ def fromDoc(
"is not valid because:",
)
)
if outdirMin is not None and outdirMax is not None and outdirMin > outdirMax:
_errors__.append(
ValidationException(
"the `outdirMin` field is greater than the `outdirMax` field",
None
)
)

extension_fields: dict[str, Any] = {}
for k in _doc.keys():
if k not in cls.attrs:
Expand Down Expand Up @@ -18478,6 +18510,11 @@ def save(
r["coresMax"] = save(
self.coresMax, top=False, base_url=base_url, relative_uris=relative_uris
)
if r.get("coresMax") and r.get("coresMin") and r["coresMax"] < r["coresMin"]:
raise ValidationException(
"the `coresMax` field is less than the `coresMin` field"
)

if self.ramMin is not None:
r["ramMin"] = save(
self.ramMin, top=False, base_url=base_url, relative_uris=relative_uris
Expand All @@ -18486,6 +18523,11 @@ def save(
r["ramMax"] = save(
self.ramMax, top=False, base_url=base_url, relative_uris=relative_uris
)
if r.get("ramMax") and r.get("ramMin") and r["ramMax"] < r["ramMin"]:
raise ValidationException(
"the `ramMax` field is less than the `ramMin` field"
)

if self.tmpdirMin is not None:
r["tmpdirMin"] = save(
self.tmpdirMin,
Expand All @@ -18500,6 +18542,11 @@ def save(
base_url=base_url,
relative_uris=relative_uris,
)
if r.get("tmpdirMax") and r.get("tmpdirMin") and r["tmpdirMax"] < r["tmpdirMin"]:
raise ValidationException(
"the `tmpdirMax` field is less than the `tmpdirMin` field"
)

if self.outdirMin is not None:
r["outdirMin"] = save(
self.outdirMin,
Expand All @@ -18514,6 +18561,10 @@ def save(
base_url=base_url,
relative_uris=relative_uris,
)
if r.get("outdirMax") and r.get("outdirMin") and r["outdirMax"] < r["outdirMin"]:
raise ValidationException(
"the `outdirMax` field is less than the `outdirMin` field"
)

# top refers to the directory level
if top:
Expand Down
108 changes: 108 additions & 0 deletions tests/test_resreq_minmax.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
from typing import Optional, List, Any

from cwl_utils.parser.cwl_v1_2 import (
ResourceRequirement,
WorkflowStep,
Workflow,
CommandLineTool,
save,
)
import pytest
from schema_salad.exceptions import ValidationException


# Helper functions
def create_commandlinetool(
requirements: Optional[List[Any]] = None,
inputs: Optional[List[Any]] = None,
outputs: Optional[List[Any]] = None,
) -> CommandLineTool:
return CommandLineTool(
requirements=requirements or [],
inputs=inputs or [],
outputs=outputs or [],
)


def create_workflow(
requirements: Optional[List[Any]] = None,
steps: Optional[List[Any]] = None,
inputs: Optional[List[Any]] = None,
outputs: Optional[List[Any]] = None,
) -> Workflow:
return Workflow(
requirements=requirements or [],
steps=steps or [],
inputs=inputs or [],
outputs=outputs or [],
)


def create_step(
requirements: Optional[List[Any]] = None,
run: Any = None,
in_: Optional[List[Any]] = None,
out: Optional[List[Any]] = None,
) -> WorkflowStep:
return WorkflowStep(
requirements=requirements or [],
run=run,
in_=in_ or [],
out=out or [],
)


@pytest.mark.parametrize(
"bad_min_max_reqs",
[
# cores
ResourceRequirement(coresMin=4, coresMax=2),
# ram
ResourceRequirement(ramMin=2048, ramMax=1024),
# tmpdir
ResourceRequirement(tmpdirMin=1024, tmpdirMax=512),
# outdir
ResourceRequirement(outdirMin=512, outdirMax=256),
],
)
def test_bad_min_max_resource_reqs(bad_min_max_reqs: ResourceRequirement) -> None:
# Test invalid min/max resource requirements in CWL objects.

# CommandlineTool with bad minmax reqs
clt = create_commandlinetool(requirements=[bad_min_max_reqs])
with pytest.raises(ValidationException):
save(clt)

# WorkflowStep.run with bad minmax reqs
step_bad_run = create_step(run=clt)
workflow = create_workflow(steps=[step_bad_run])
with pytest.raises(ValidationException):
save(workflow)

# WorkflowStep with bad minmax reqs
clt = create_commandlinetool()
step = create_step(run=clt, requirements=[bad_min_max_reqs])
workflow = create_workflow(steps=[step])
with pytest.raises(ValidationException):
save(workflow)

# Workflow with bad minmax reqs
workflow = create_workflow(requirements=[bad_min_max_reqs])
with pytest.raises(ValidationException):
save(workflow)

# NestedWorkflow with bad minmax reqs
nest_workflow = create_workflow(requirements=[bad_min_max_reqs])
step = create_step(run=nest_workflow)
workflow = create_workflow(steps=[step])
with pytest.raises(ValidationException):
save(workflow)

# DeepNestedWorkflow with bad minmax reqs
deep_workflow = create_workflow(requirements=[bad_min_max_reqs])
deep_step = create_step(run=deep_workflow)
nest_workflow = create_workflow(steps=[deep_step])
step = create_step(run=nest_workflow)
workflow = create_workflow(steps=[step])
with pytest.raises(ValidationException):
save(workflow)