-
Notifications
You must be signed in to change notification settings - Fork 64
Add workflow module ordering validation #1651
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
Changes from 5 commits
cc6f96d
993f0c4
dc3a6bc
6cf03b4
c24dc06
d5bfde5
50f2e5b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,155 @@ | ||
| """ | ||
| Validate the ordering of modules in a HADDOCK3 workflow. | ||
|
|
||
| Module- and parameter-level validation (see :mod:`haddock.gear.prepare_run`) | ||
| confirms that each requested module exists and that its parameters are valid, | ||
| but it does not enforce constraints on the *sequence* of modules. Some modules | ||
| only make sense when another module ran before them (e.g. ``clustrmsd`` needs | ||
| an RMSD matrix), some must not be chained to themselves (e.g. two consecutive | ||
| ``seletop``), and a workflow must always start by building topologies. | ||
|
|
||
| Those constraints are declared as data in ``workflow_rules.yaml`` and applied | ||
| here against the ordered list of modules taken from the user configuration | ||
| file. Keeping the rules in a data file means new constraints can be added | ||
| without editing this logic. | ||
|
|
||
| Rule types (see ``workflow_rules.yaml`` for the exact syntax): | ||
|
|
||
| - ``disallowed_sequences``: module B must not directly follow module A. | ||
| - ``required_preceding``: module B must be immediately preceded by one of a set. | ||
| - ``required_prior``: module B must be preceded anywhere earlier by one of a set. | ||
| - ``required_first``: the first module must be one of a set. | ||
| """ | ||
|
|
||
| from pathlib import Path | ||
|
|
||
| from haddock import log | ||
| from haddock.core.exceptions import ConfigurationError | ||
| from haddock.libs.libio import read_from_yaml | ||
|
|
||
|
|
||
| DEFAULT_RULES = Path(Path(__file__).resolve().parent, "workflow_rules.yaml") | ||
|
|
||
|
|
||
| def read_workflow_rules(rules_file=DEFAULT_RULES): | ||
| """ | ||
| Read the workflow ordering rules from a YAML file. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| rules_file : str or pathlib.Path | ||
| Path to the YAML file defining the ordering rules. | ||
| Defaults to the bundled ``workflow_rules.yaml``. | ||
|
|
||
| Returns | ||
| ------- | ||
| dict | ||
| The parsed rules, with every supported rule key present (empty | ||
| containers for the ones not defined in the file). | ||
| """ | ||
| rules = read_from_yaml(rules_file) | ||
| # guarantee all keys exist so callers need not test for their presence | ||
| rules.setdefault("disallowed_sequences", []) | ||
| rules.setdefault("required_preceding", {}) | ||
| rules.setdefault("required_prior", {}) | ||
| rules.setdefault("required_first", []) | ||
| return rules | ||
|
|
||
|
|
||
| def _check_required_first(modules, required_first): | ||
| """Collect a violation if the first module is not an allowed one.""" | ||
| errors = [] | ||
| if required_first and modules and modules[0] not in required_first: | ||
| errors.append( | ||
| f"The first module of a workflow must be one of " | ||
| f"{_join(required_first)}, but {modules[0]!r} was found." | ||
| ) | ||
| return errors | ||
|
|
||
|
|
||
| def _check_disallowed_sequences(modules, disallowed_sequences): | ||
| """Collect violations for modules directly following a disallowed one.""" | ||
| errors = [] | ||
| disallowed = {tuple(pair) for pair in disallowed_sequences} | ||
| for position, (before, after) in enumerate(zip(modules, modules[1:]), start=2): | ||
| if (before, after) in disallowed: | ||
| errors.append( | ||
| f"Module {after!r} (step {position}) cannot directly follow " | ||
| f"module {before!r}." | ||
| ) | ||
| return errors | ||
|
|
||
|
|
||
| def _check_required_preceding(modules, required_preceding): | ||
| """Collect violations for modules lacking a required direct predecessor.""" | ||
| errors = [] | ||
| for position, module in enumerate(modules, start=1): | ||
| allowed = required_preceding.get(module) | ||
| if allowed is None: | ||
| continue | ||
| preceding = modules[position - 2] if position >= 2 else None | ||
| if preceding not in allowed: | ||
| found = f"{preceding!r}" if preceding is not None else "nothing" | ||
| errors.append( | ||
| f"Module {module!r} (step {position}) must be directly preceded " | ||
| f"by one of {_join(allowed)}, but {found} was found." | ||
| ) | ||
| return errors | ||
|
|
||
|
|
||
| def _check_required_prior(modules, required_prior): | ||
| """Collect violations for modules lacking a required earlier module.""" | ||
| errors = [] | ||
| for position, module in enumerate(modules, start=1): | ||
| allowed = required_prior.get(module) | ||
| if allowed is None: | ||
| continue | ||
| earlier = modules[: position - 1] | ||
| if not any(mod in earlier for mod in allowed): | ||
| errors.append( | ||
| f"Module {module!r} (step {position}) requires one of " | ||
| f"{_join(allowed)} to run at some earlier step." | ||
| ) | ||
| return errors | ||
|
|
||
|
|
||
| def _join(modules): | ||
| """Render a collection of module names as a readable quoted list.""" | ||
| return ", ".join(repr(mod) for mod in modules) | ||
|
|
||
|
|
||
| def validate_workflow_order(modules, rules_file=DEFAULT_RULES): | ||
| """ | ||
| Validate the order of the modules of a workflow against the rules. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| modules : sequence of str | ||
| The module names in workflow order (without their ``.N`` suffix). | ||
|
|
||
| rules_file : str or pathlib.Path | ||
| Path to the YAML file defining the ordering rules. | ||
| Defaults to the bundled ``workflow_rules.yaml``. | ||
|
|
||
| Raises | ||
| ------ | ||
| haddock.core.exceptions.ConfigurationError | ||
| If the workflow violates one or more ordering rules. All detected | ||
| violations are reported together. | ||
| """ | ||
| modules = list(modules) | ||
| rules = read_workflow_rules(rules_file) | ||
|
|
||
| errors = [] | ||
| errors.extend(_check_required_first(modules, rules["required_first"])) | ||
| errors.extend(_check_disallowed_sequences(modules, rules["disallowed_sequences"])) | ||
| errors.extend(_check_required_preceding(modules, rules["required_preceding"])) | ||
| errors.extend(_check_required_prior(modules, rules["required_prior"])) | ||
|
|
||
| if errors: | ||
| msg = "Invalid workflow module order:" + "".join( | ||
| f"\n - {error}" for error in errors | ||
| ) | ||
| raise ConfigurationError(msg) | ||
|
|
||
| log.info("Workflow module order validated.") |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| # ============================================================================= | ||
| # HADDOCK3 workflow ordering rules | ||
| # ============================================================================= | ||
| # | ||
| # Individual module- and parameter-level validation (see gear/prepare_run.py) | ||
| # confirms that each requested module exists and that its parameters are valid. | ||
| # It does *not* enforce constraints on the *sequence* in which modules appear. | ||
| # | ||
| # This file declares those sequence constraints as data, so that they can be | ||
| # reviewed and extended without touching the validation logic in | ||
| # `gear/workflow_ordering.py`. Module names below are the module folder names | ||
| # (e.g. `topoaa`, `clustrmsd`), i.e. the keys used in a workflow config file | ||
| # without their `.N` occurrence suffix. | ||
| # | ||
| # Four rule types are supported: | ||
| # | ||
| # disallowed_sequences a pair [A, B]: module B must NOT come directly | ||
| # after module A (guards against meaningless repeats). | ||
| # | ||
| # required_preceding B: [A1, A2, ...]: module B must be immediately | ||
| # preceded by one of A1, A2, ... | ||
| # | ||
| # required_prior B: [A1, A2, ...]: module B must be preceded | ||
| # somewhere earlier in the workflow (not necessarily | ||
| # directly) by at least one of A1, A2, ... | ||
| # | ||
| # required_first [M1, M2, ...]: the very first module of a workflow | ||
| # must be one of M1, M2, ... | ||
| # | ||
| # All checks are skipped for entries whose "trigger" module is not present in | ||
| # the workflow, so adding a rule never breaks unrelated workflows. | ||
| # ----------------------------------------------------------------------------- | ||
|
|
||
| # Module B must not directly follow module A. Each entry is a [A, B] pair. | ||
| disallowed_sequences: | ||
| - [clustrmsd, clustrmsd] | ||
| - [clustfcc, clustfcc] | ||
| - [seletop, seletop] | ||
| - [seletopclusts, seletopclusts] | ||
|
|
||
| # Module (key) must be immediately preceded by one of the listed modules. | ||
| required_preceding: | ||
| clustrmsd: [rmsdmatrix, ilrmsdmatrix] | ||
| topocg: [topoaa] | ||
|
|
||
| # Module (key) must be preceded anywhere earlier by one of the listed modules. | ||
| required_prior: | ||
| cgtoaa: [topoaa] | ||
|
|
||
| # The first module of the workflow must be one of these. | ||
| required_first: | ||
| - topoaa | ||
|
Comment on lines
+51
to
+52
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. simple comment / open question (after 3 year only): Why don't we just run
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because there are some options that can be given to it.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One of which is to provide your own param/top files for a ligand. So it should remain exposed.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
this is not the reason, options could be passed to it without it being defined as a module - @VGPReys this module exist as an historical artifact, the very initial pre-alpha version had this logic of topology>rigidbody>selection>refinement (same as the legacy haddock version) and this was just propagated until now. so the reason it exists is because it always existed, but could indeed be absorbed into the initialization and not needed to be explicit.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No so simple. If you want to control the protonation state of say 10 histamine you will have to pass many options... Won't be user friendly. Same thing for providing custom param files. Now all this is defined in the config file (better for reproducibility). If you have to pass many options every time you call haddock3 it will be messy ("a la rosetta" with incredibly long command lines). But in the future we could lift the requirement a workflow starts with it. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| """Test the workflow module ordering validation.""" | ||
|
|
||
| import pytest | ||
|
|
||
| from haddock.core.exceptions import ConfigurationError | ||
| from haddock.gear.workflow_ordering import ( | ||
| read_workflow_rules, | ||
| validate_workflow_order, | ||
| ) | ||
|
|
||
|
|
||
| def test_read_workflow_rules_has_all_keys(): | ||
| """The bundled rules expose every supported rule key.""" | ||
| rules = read_workflow_rules() | ||
| assert set(rules) >= { | ||
| "disallowed_sequences", | ||
| "required_preceding", | ||
| "required_prior", | ||
| "required_first", | ||
| } | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "modules", | ||
| [ | ||
| ["topoaa", "rigidbody", "flexref", "emref"], | ||
| ["topoaa", "rigidbody", "rmsdmatrix", "clustrmsd", "seletopclusts"], | ||
| ["topoaa", "rigidbody", "ilrmsdmatrix", "clustrmsd"], | ||
| ["topoaa", "topocg", "rigidbody"], | ||
| ["topoaa", "topocg", "rigidbody", "cgtoaa", "flexref"], | ||
| ["topoaa", "rigidbody", "clustfcc", "seletopclusts", "clustfcc"], | ||
| ], | ||
| ) | ||
| def test_valid_workflows_pass(modules): | ||
| """Well-ordered workflows validate without error.""" | ||
| validate_workflow_order(modules) | ||
|
|
||
|
|
||
| def test_required_first_violation(): | ||
| """A workflow not starting with topoaa is rejected.""" | ||
| with pytest.raises(ConfigurationError, match="first module"): | ||
| validate_workflow_order(["rigidbody", "flexref"]) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "modules", | ||
| [ | ||
| ["topoaa", "rmsdmatrix", "clustrmsd", "clustrmsd"], | ||
| ["topoaa", "rigidbody", "clustfcc", "clustfcc"], | ||
| ["topoaa", "rigidbody", "seletop", "seletop"], | ||
| ["topoaa", "seletopclusts", "seletopclusts"], | ||
| ], | ||
| ) | ||
| def test_disallowed_sequence_violation(modules): | ||
| """Repeating a non-repeatable module directly is rejected.""" | ||
| with pytest.raises(ConfigurationError, match="cannot directly follow"): | ||
| validate_workflow_order(modules) | ||
|
|
||
|
|
||
| def test_required_preceding_violation_wrong_predecessor(): | ||
| """clustrmsd not directly preceded by an rmsd matrix is rejected.""" | ||
| with pytest.raises(ConfigurationError, match="directly preceded"): | ||
| validate_workflow_order(["topoaa", "rigidbody", "clustrmsd"]) | ||
|
|
||
|
|
||
| def test_required_preceding_violation_topocg(): | ||
| """topocg must directly follow topoaa.""" | ||
| with pytest.raises(ConfigurationError, match="directly preceded"): | ||
| validate_workflow_order(["topoaa", "rigidbody", "topocg"]) | ||
|
|
||
|
|
||
| def test_required_prior_violation(): | ||
| """cgtoaa requires topoaa somewhere earlier.""" | ||
| # required_first also fires here, but the prior-module message must appear | ||
| with pytest.raises(ConfigurationError, match="earlier step"): | ||
| validate_workflow_order(["topocg", "rigidbody", "cgtoaa"]) | ||
|
|
||
|
|
||
| def test_multiple_violations_reported_together(): | ||
| """All ordering violations are collected in a single error.""" | ||
| with pytest.raises(ConfigurationError) as exc: | ||
| validate_workflow_order(["rigidbody", "seletop", "seletop"]) | ||
| message = str(exc.value) | ||
| assert "first module" in message | ||
| assert "cannot directly follow" in message | ||
|
|
||
|
|
||
| def test_empty_workflow_does_not_raise(): | ||
| """An empty module list is not an ordering error on its own.""" | ||
| validate_workflow_order([]) | ||
|
|
||
|
|
||
| def test_custom_rules_file(tmp_path): | ||
| """Rules are read from the provided file.""" | ||
| rules_file = tmp_path / "rules.yaml" | ||
| rules_file.write_text("required_first:\n - rigidbody\n") | ||
| # topoaa first now violates the custom rule | ||
| with pytest.raises(ConfigurationError, match="first module"): | ||
| validate_workflow_order(["topoaa"], rules_file=rules_file) | ||
| # rigidbody first satisfies it | ||
| validate_workflow_order(["rigidbody"], rules_file=rules_file) |
Uh oh!
There was an error while loading. Please reload this page.