Resource requirements validation

[Stellatsuu]

I tried to run "bad" jobs with cwltool and it seems that it does not check for conflicts or higher requirements than expected in ResourceRequirement.

So, I've come to this code to validate ResourceRequirements in workflows, related to #27. I tried to make something generic in case we need to implement other Requirements in the future?

However, I'm not entirely sure if this matches with how we want to implement things in the proto, so I'd appreciate any feedback or suggestions.

[mr-c]

@Stellatsuu PRs and/or issues to cwltool are welcome. I'm curious as to how it didn't meet your expectations.

[Stellatsuu]

Hi @mr-c,
I just created a discussion on CWL Forum explaining what I'm trying to do: https://cwl.discourse.group/t/how-does-resourcerequirement-work/1019

[natthan-pigoux]

Thank you @Stellatsuu ! The overall looks good to me.

I have a remark on the testing part. We can probably do something more concise here without adding that many cwl files.

Here is a suggestion which could be extended:

from cwl_utils.parser.cwl_v1_2 import CommandLineTool, Workflow, WorkflowStep, ResourceRequirement
import pytest

from dirac_cwl_proto.execution_hooks.requirement_validator import RequirementError, ResourceRequirementValidator

@pytest.mark.parametrize(
    ("bad_min_max_reqs"),
    [
        ResourceRequirement(coresMin=4, coresMax=2),
        ResourceRequirement(ramMin=2048, ramMax=1024)
    ],
)
def test_bad_min_max_resource_reqs(bad_min_max_reqs):
    """Test wrong min max resource requirements."""
    # test clt:
    clt = CommandLineTool(
        requirements=[bad_min_max_reqs],
        inputs=[],
        outputs=[]
    )
    with pytest.raises(RequirementError):
        ResourceRequirementValidator(cwl_object=clt).validate_requirements()

    # test workflow
    step = WorkflowStep(
        in_=[],
        out=[],
        run=clt
    )
    workflow = Workflow(
        requirements=[],
        steps=[step],
        inputs=[],
        outputs=[]
    )
    with pytest.raises(RequirementError):
        ResourceRequirementValidator(cwl_object=workflow).validate_requirements()

    workflow = Workflow(
        requirements=[bad_min_max_reqs],
        steps=[],
        inputs=[],
        outputs=[]
    )
    with pytest.raises(RequirementError):
        ResourceRequirementValidator(cwl_object=workflow).validate_requirements()

@pytest.mark.parametrize(
    ("global_cores_requirements", "step_cores_requirements"),
    [
        (
            ResourceRequirement(coresMax=2),
            ResourceRequirement(coresMin=4),
        ),
        (
            ResourceRequirement(ramMax=512),
            ResourceRequirement(ramMin=1024),
        ),
    ],
)
def test_bad_global_requirements(global_cores_requirements, step_cores_requirements):
    """Test global requirements conflicts."""
    clt = CommandLineTool(
        inputs=[],
        outputs=[]
    )
    step = WorkflowStep(
        in_=[],
        out=[],
        run=clt,
        requirements=[step_cores_requirements]
    )
    workflow = Workflow(
        requirements=[global_cores_requirements],
        steps=[step],
        inputs=[],
        outputs=[]
    )
    with pytest.raises(RequirementError):
        ResourceRequirementValidator(cwl_object=workflow).validate_requirements()

This is more easily extendable, and it also allows to test the RequirementValidator separately of the job submission.

What do you think?

[Stellatsuu]

Thank you @Stellatsuu ! The overall looks good to me.

I have a remark on the testing part. We can probably do something more concise here without adding that many cwl files.

This is more easily extendable, and it also allows to test the RequirementValidator separately of the job submission.

What do you think?

@natthan-pigoux Thanks, I hadn't thought of that, but it definitely seems like a better approach! I was already realizing that there were a lot of files... I forgot I could test it this way, so I'll give it a try and see if we can apply these tests to cover all cases 😄

[aldbr]

We now know that this should be part of cwl itself.
By the way, now that I am seeing that, I think we should include it to cwl-utils and not cwltool (or may be both).
In this way, this minmax test would be performed when we use save in the Production/Transformation/JobSubmissionModel.
Do you see what I mean?

[Stellatsuu]

I added a minmax check in cwl-utils : common-workflow-language/cwl-utils@addfebb
When saving a Workflow or CLT with save(), it triggers an error if min > max, I also added a test for that.

But, this will only check the minmax values of a same ResourceRequirement (ex: Workflow RR, Step RR, CLT RR, etc.). I don't think we can check that a Step RR values are not higher than a 'global' Workflow RR values using cwl-utils, I don't really see how to do it right now.

Also, I don't really know how to test the fromDoc() method, so I added the check without any test..

Do you think I should also look to add it to cwltool ? @aldbr

[mr-c]

I don't think we can check that a Step RR values are not higher than a 'global' Workflow RR values using cwl-utils

If there is a step-level ResourceRequirement in requirements, then any ResourceRequirement under the workflow-level hints or requirements would be ignored in CWL v1.0-v1.3; so such a check is unnecessarily and non-sensical

[Stellatsuu]

@aldbr Here's an update for now :

I removed my Validator classes and added everything into SubmissionsModels as asked :

• Every type of WF (Job, Transformation, Production) goes through the same validation before the model is created.
  - I don't know if I should've included the methods in the SubmissionModel class or if I let them as module helpers outside of it.
  - Once we add every possible check in cwl-utils/cwltool, we'll be able to reduce the validation cases/code and maybe even remove all of it.
  - Should I also add this validation into TransformationSubmissionModel and ProductionSubmissionModel ? I saw we always end up calling JobSubmissionModel, that's why I only implemented it there.
• In case of a Production, there's a special validation alongside the steps-metadata validation.

As for the implementation in cwl-utils/cwltool, I'm working on it (see: #40 (comment)). I think it would be great to add the minmax check in both codes, what do you think? Do you think I should ask the devs?

[Stellatsuu]

Open cwl-utils PR: common-workflow-language/cwl-utils#384 --> might be closed due to unnecessary validation (it's mostly auto-generated code)
Open cwl-tool PR: common-workflow-language/cwltool#2179