validation: make environment property mandatory in serial steps #415

giuseppe-steduto · 2023-10-26T14:20:35Z

codecov · 2023-10-26T14:23:55Z

Codecov Report

Merging #415 (120559a) into master (2ad95ce) will increase coverage by 0.16%.
The diff coverage is 88.88%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #415      +/-   ##
==========================================
+ Coverage   37.50%   37.66%   +0.16%     
==========================================
  Files          26       26              
  Lines        1557     1561       +4     
==========================================
+ Hits          584      588       +4     
  Misses        973      973

Files	Coverage Δ
reana_commons/validation/utils.py	`79.31% <100.00%> (+1.53%)`	⬆️
reana_commons/version.py	`0.00% <0.00%> (ø)`

mdonadoni

This seems to break some non-serial workflows, such as the Snakemake version of helloworld. Can you reproduce this?

$ reana-client run -f reana-snakemake.yaml
==> Creating a workflow...
Job stats:
job           count    min threads    max threads
----------  -------  -------------  -------------
all               1              1              1
helloworld        1              1              1
total             2              1              1

[WARNING] Job stats:
job           count    min threads    max threads
----------  -------  -------------  -------------
all               1              1              1
helloworld        1              1              1
total             2              1              1

==> Verifying REANA specification file... .../reana-demo-helloworld/reana-snakemake.yaml
[ERROR] Invalid REANA specification: None is not of type 'string'
==> ERROR: Cannot create workflow : 
None is not of type 'string'

Failed validating 'type' in schema['properties']['workflow']['properties']['specification']['properties']['steps']['items']['properties']['kubernetes_memory_limit']:
    {'id': '/properties/workflow/properties/specification/properties/steps/properties/kubernetes_memory_limit',
     'title': 'Memory limit for the step container (e.g. 256Mi).',
     'type': 'string'}

On instance['workflow']['specification']['steps'][0]['kubernetes_memory_limit']:
    None

Also note that the CWL version has a very different workflow.specification (even though it still works fine):

"workflow": {
  "file": "workflow/cwl/helloworld.cwl",
  "specification": {
    "$graph": [
      {
        "class": "Workflow",
        "id": "#main",
        "inputs": [
          {
            "id": "#main/helloworld",
            "type": "File"
...

Maybe it's possible to check the schema of workflow.specification only if workflow.type is serial?

giuseppe-steduto · 2023-10-26T15:44:01Z

This seems to break some non-serial workflows, such as the Snakemake version of helloworld

Apparently this happens because the Snakemake specification is loaded using None as a value for all the keys that were not explicitely specified in the Snakefile. Thus a solution could be to allow for both strings and null values for this and other properties.

Maybe it's possible to check the schema of workflow.specification only if workflow.type is serial?

AFAIK in JSON Schema draft-04 (the one we are using), one can't express conditional requirements directly. JSON Schema draft-07 introduced the if, then, and else structure, so it would be possible to do something like this:

{
  ...
  "workflow": {
    ...
    "properties": {
      "type": {
        "type": "string",
        "enum": [
          "cwl",
          "serial",
          "yadage",
          "snakemake"
        ]
      }
    },
    "if": {
      "properties": {
        "type": {
          "const": "serial"
        }
      }
    },
    "then": {
      "properties": {
        "specification": {
          "properties": {
            "steps": {
              "items": {
                "required": ["environment"]
              }
            }
          }
        }
      }
    },
    "else": {
      "properties": {
        "specification": {
          "id": ...
        }
      }
    }
  }
}

But upgrading to draft-07 might not be worth it, especially if it's only for this reason (draft-06 introduced a few big backwards-incompatible changes).

Another valid option at this point would be to add only the "required": ["environment"]" clause and forget about the rest of the "specification" schema. In this way the effect would be:

Serial workflows: make environment mandatory in the steps, as it should be
Snakemake workflows: make environment mandatory in the steps, as it should be (since the steps are loaded in a very similar way to the serial workflows)
CWL workflows: the steps are inside specification.$graph.steps, so the required clause does not apply
Yadage workflows: the steps are inside specification.stages.scheduler..., so the required clause does not apply

In this way, also, all the specifications covered by reana-dev run-example -w cwl -w snakemake -w serial -w yadage -c DEMO -b kubernetes --submit-only seem to pass the validation stage successfully.

Yet another alternative, mentioned in the JSONSchema documentation, could be to use a small boolean algebra trick to rewrite the A -> B condition as !A || B (therefore not needing conditionals). This has the advantage of allowing to specify more constraints on serial workflows (such as no inputs or outputs under each workflow step). Even though somewhat clever, it would complicate the schema a bit, especially because the conditions is on workflow.type, whereas the inquired properties are under workflow.specifications.steps.items.{property}, and the pattern is not very intuitive.

Finally, the last option I see is enforcing different JSON schemas for different workflow types at the r-commons level (maybe with a general one for every workflow engine, extended by the workflow-engine-specific ones.

To recap:
Solution 1️⃣: allow for string an null values on kubernetes_memory_limit and other properties.
Solution 2️⃣: upgrade to JSON schema draft-07 and use the if/then construct.
Solution 3️⃣: only keep the "required": ["environment"]" clause (I would personally go for this one).
Solution 4️⃣: use !A || B instead of A -> B to define the condition
Solution 5️⃣: enforce different JSON schemas at the REANA logic level for different workflow engines.

What do you think?

tiborsimko · 2023-11-08T10:01:51Z

What do you think?

Thanks for the nice analysis. IMO the soluttion 3 will do for the problem at hand, but we should be ready to improve the behaviour during sandboxing sprint.

For example, I've heard a wish from Snakemake users who typically use the same container for all the steps that they did not really expect having to specify the container: clause for each rule in the Snakefile, because it's not really required when running Snakemake "locally". The users wished to specify a "base container" for the workflow that would be used for the execution of all the rules unless overridden. The current PR is actually very good for catching these must-have-container-clause situations early, but there are chances we may amend this later to allow specifying a base container for the workflow aiming at having 1:1 correspondence between Snakefile-outside-REANA and Snakefile-inside-REANA for these kind of usages. If we do that, then should we revisit the environment validation anyway.

Ideally, it would be nice to have different schemas for different workflow engines, or at least conditionals, so that we can express truly all the constraints, and cover other fields, not only environments. But this would be a bigger change that could probably go together with the sandboxing sprint...

mdonadoni · 2023-11-27T08:53:12Z

CHANGES.rst

@@ -4,7 +4,10 @@ Changes
 Version 0.9.4 (UNRELEASED)
 --------------------------

+- Changes the REANA specification schema to use the ``draft 2020-12`` version of the JSON schema specification.


Suggested change

- Changes the REANA specification schema to use the ``draft 2020-12`` version of the JSON schema specification.

- Changes the REANA specification schema to use the ``draft-07`` version of the JSON schema specification.

mdonadoni · 2023-11-27T09:23:45Z

reana_commons/validation/schemas/reana_analysis_schema.json

-          "type": "object",
+          "type": [
+            "object",
+            "null"


In which cases resources can be null?

Never - restored it to just allowing objects

mdonadoni · 2023-11-27T09:30:55Z

reana_commons/validation/schemas/reana_analysis_schema.json

+                          "kubernetes",
+                          "htcondor",
+                          "htcondorcern",
+                          "slurm",
+                          "slurmcern"


I do not think slurm and htcondor are allowed here. Did you find any places where they are used?

I put them there to be less restrictive towards other REANA deployments that might want to integrate HTCondor or Slurm, but I don't know how the integration with those computational backends would work if external to CERN. Do you think it is better to remove them (and also avoid that CERN users mistakenly write "htcondor" instead of "htcondorcern")?

and also avoid that CERN users mistakenly write "htcondor" instead of "htcondorcern"

Yes, exactly, I think we should avoid this so let's remove them as currently we only support kubernetes, htcondorcern and slurmcern. When we will add support for other Slurm/HTCondor instances we will update the schema accordingly!

mdonadoni · 2023-11-27T09:44:25Z

tests/test_validation.py

+            for warning_value in value:
+                assert warning_value in warnings[key]
+        else:
+            assert operator.eq(value, warnings[key])


Just as a curiosity, why we call operator.eq here and not ==?

Relic of a previous version of the test in which I used map and passed operator.eq as a function. I've changed it to == to be more pythonic 😉

Closes reanahub#422.

mdonadoni

Looks good to me!

The only thing that needs to be changed is the new alpha version.

mdonadoni · 2023-11-28T14:07:40Z

reana_commons/version.py

@@ -14,4 +14,4 @@

 from __future__ import absolute_import, print_function

-__version__ = "0.9.4a2"
+__version__ = "0.9.4a4"


Let's regenerate the release commit so that the new version is 0.9.4a3 and not 0.9.4a4

Closes reanahub/reana-client#679.

mdonadoni reviewed Oct 26, 2023

View reviewed changes

giuseppe-steduto force-pushed the validate-fix-environment-misplaced branch from 5750fce to 3d5b5a6 Compare November 8, 2023 09:17

giuseppe-steduto mentioned this pull request Nov 9, 2023

JSON Schema: update to newer draft #422

Closed

giuseppe-steduto force-pushed the validate-fix-environment-misplaced branch 3 times, most recently from 269a6c5 to 4ea3b69 Compare November 14, 2023 23:48

giuseppe-steduto force-pushed the validate-fix-environment-misplaced branch 4 times, most recently from d804bcd to 827d309 Compare November 22, 2023 16:43

mdonadoni reviewed Nov 27, 2023

View reviewed changes

schemas: update JSON Schema version to draft 7

8205862

Closes reanahub#422.

giuseppe-steduto force-pushed the validate-fix-environment-misplaced branch from 827d309 to bbb85a3 Compare November 28, 2023 11:10

mdonadoni approved these changes Nov 28, 2023

View reviewed changes

mdonadoni reviewed Nov 28, 2023

View reviewed changes

giuseppe-steduto added 2 commits November 28, 2023 15:19

validation: add workflow-engine specific clauses to specification schema

6167c0c

Closes reanahub/reana-client#679.

release: 0.9.4a3

120559a

giuseppe-steduto force-pushed the validate-fix-environment-misplaced branch from bbb85a3 to 120559a Compare November 28, 2023 14:22

mdonadoni approved these changes Nov 29, 2023

View reviewed changes

mdonadoni merged commit 120559a into reanahub:master Nov 29, 2023
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

validation: make environment property mandatory in serial steps #415

validation: make environment property mandatory in serial steps #415

giuseppe-steduto commented Oct 26, 2023

codecov bot commented Oct 26, 2023 •

edited

Loading

mdonadoni left a comment

giuseppe-steduto commented Oct 26, 2023 •

edited

Loading

tiborsimko commented Nov 8, 2023

mdonadoni Nov 27, 2023

mdonadoni Nov 27, 2023

giuseppe-steduto Nov 28, 2023

mdonadoni Nov 27, 2023

giuseppe-steduto Nov 28, 2023

mdonadoni Nov 28, 2023

mdonadoni Nov 27, 2023

giuseppe-steduto Nov 28, 2023

mdonadoni left a comment

mdonadoni Nov 28, 2023

	- Changes the REANA specification schema to use the ``draft 2020-12`` version of the JSON schema specification.
	- Changes the REANA specification schema to use the ``draft-07`` version of the JSON schema specification.

validation: make environment property mandatory in serial steps #415

validation: make environment property mandatory in serial steps #415

Conversation

giuseppe-steduto commented Oct 26, 2023

codecov bot commented Oct 26, 2023 • edited Loading

Codecov Report

mdonadoni left a comment

Choose a reason for hiding this comment

giuseppe-steduto commented Oct 26, 2023 • edited Loading

tiborsimko commented Nov 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdonadoni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 26, 2023 •

edited

Loading

giuseppe-steduto commented Oct 26, 2023 •

edited

Loading