Skip to content
Merged
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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -173,3 +173,4 @@ tests/data/minimal_data/iteration_1/t3_pdep_qm_budget.yml
tests/data/minimal_data/iteration_1/t3_pdep_network_assessments.yml
tests/data/pdep_network/iteration_1/t3_pdep_qm_budget.yml
tests/data/pdep_network/iteration_1/t3_pdep_network_assessments.yml
docs/contracts/
24 changes: 24 additions & 0 deletions t3/pdep/explorer/input_file.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
from dataclasses import dataclass, field

from t3.pdep.hashing import hash_bytes
from t3.pdep.hybrid import _MODEL_CHEMISTRY_CALL_NAMES, _validate_model_chemistry_expression
from t3.utils.writer import METHOD_LINE_CANDIDATE_RE, METHOD_MAP, rewrite_arkane_method_line

# The only top-level Arkane DSL calls this module needs to recognize while walking the source's
Expand Down Expand Up @@ -1533,6 +1534,29 @@ def _validate_source_statements(tree: ast.Module, text: str, source_path: str) -
f"Arkane itself defines in the namespace it loads an input file in. Rebinding it shadows "
f"the real one for every statement that follows -- including the directives this module "
f"generates and appends -- so a source may not assign to it, however harmless the value.")
# ``modelChemistry`` is the one directive whose real ARC/hybrid value is a bare
# ``LevelOfTheory(...)``/``CompositeLevelOfTheory(...)`` call (which Arkane execs at load
# time), not an ``ast.literal_eval``-able literal. Rather than grow a second model-chemistry
# allowlist here, route it through the SAME structural checker T3 uses when it EMITS this
# directive (``t3.pdep.hybrid._validate_model_chemistry_expression``); the AST is what
# decides exec semantics and the verbatim splice reproduces it, so validating the
# ``ast.unparse``-round-tripped node -- which that string-taking checker re-parses -- is
# equivalent to validating the spliced text. The gate is deliberately on a genuine call
# node to one of the two known names: the checker accepts any NON-call string as a plain
# label (injection-char check only), so a computed ``modelChemistry`` value must keep
# falling through to the refusal below rather than being handed over as a bare label.
if (target == 'modelChemistry' and isinstance(node.value, ast.Call)
and isinstance(node.value.func, ast.Name)
and node.value.func.id in _MODEL_CHEMISTRY_CALL_NAMES):
try:
_validate_model_chemistry_expression(target, ast.unparse(node.value))
except ValueError as e:
raise ValueError(
f"Refusing to use '{source_path}' as an Arkane explorer/network source: line "
f"{node.lineno} assigns a 'modelChemistry' value that fails structural validation "
f"({_source_snippet(node, text)!r}): {e}. This source's text is spliced verbatim into "
Comment thread
alongd marked this conversation as resolved.
f"a NEW file Arkane will exec, so a malformed model-chemistry call is refused.") from e
continue
try:
ast.literal_eval(node.value)
except (ValueError, TypeError, SyntaxError, MemoryError, RecursionError) as e:
Expand Down
44 changes: 27 additions & 17 deletions t3/pdep/hybrid.py
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ def _model_chemistry_ast_call(value: str) -> ast.Call | None:
return None


def _validate_level_of_theory_call(call: ast.Call, value: str) -> None:
def _validate_level_of_theory_call(call: ast.Call, value: str, field_name: str) -> None:
"""
Structurally validate a ``LevelOfTheory(...)`` call node against the real Arkane
``LevelOfTheory`` constructor schema (see ``_LEVEL_OF_THEORY_FIELD_TYPES``): only keyword
Expand All @@ -151,6 +151,11 @@ def _validate_level_of_theory_call(call: ast.Call, value: str) -> None:
Args:
call (ast.Call): The ``LevelOfTheory(...)`` call node to validate.
value (str): The original ``model_chemistry`` string (used in error messages).
field_name (str): The name of the field ``value`` came from, as the caller knows it
(e.g. ``QMEnergySettings.model_chemistry`` when validating settings, or
``modelChemistry`` when validating a directive in an Arkane source).
Reported verbatim in error messages, so a failure names the field the
caller actually has rather than one from an unrelated context.

Raises:
ValueError: If ``call`` has positional args, ``**kwargs``, a duplicate keyword, a keyword
Expand All @@ -159,33 +164,33 @@ def _validate_level_of_theory_call(call: ast.Call, value: str) -> None:
``method`` keyword.
"""
if call.args:
raise ValueError(f"QMEnergySettings.model_chemistry's LevelOfTheory(...) call must not have positional "
raise ValueError(f"{field_name}'s LevelOfTheory(...) call must not have positional "
f"arguments, got {value!r}.")
seen_keywords = set()
for kw in call.keywords:
if kw.arg is None:
raise ValueError(f"QMEnergySettings.model_chemistry's LevelOfTheory(...) call must not use "
raise ValueError(f"{field_name}'s LevelOfTheory(...) call must not use "
f"**kwargs, got {value!r}.")
if kw.arg in seen_keywords:
raise ValueError(f"QMEnergySettings.model_chemistry's LevelOfTheory(...) call must not repeat "
raise ValueError(f"{field_name}'s LevelOfTheory(...) call must not repeat "
f"keyword {kw.arg!r}, got {value!r}.")
seen_keywords.add(kw.arg)
allowed_types = _LEVEL_OF_THEORY_FIELD_TYPES.get(kw.arg)
if allowed_types is None:
raise ValueError(f"QMEnergySettings.model_chemistry's LevelOfTheory(...) call has unknown keyword "
raise ValueError(f"{field_name}'s LevelOfTheory(...) call has unknown keyword "
f"{kw.arg!r}, got {value!r}.")
# Compare exact type(), not isinstance(): isinstance(True, int) is True in Python's numeric
# tower, so an isinstance check would silently accept e.g. method=True as if it were a str.
if not isinstance(kw.value, ast.Constant) or type(kw.value.value) not in allowed_types:
raise ValueError(f"QMEnergySettings.model_chemistry's LevelOfTheory(...) keyword {kw.arg!r} must be a "
raise ValueError(f"{field_name}'s LevelOfTheory(...) keyword {kw.arg!r} must be a "
f"literal of type {allowed_types!r}, got {value!r}.")
missing = [field for field in _LEVEL_OF_THEORY_REQUIRED_FIELDS if field not in seen_keywords]
if missing:
raise ValueError(f"QMEnergySettings.model_chemistry's LevelOfTheory(...) call is missing required "
raise ValueError(f"{field_name}'s LevelOfTheory(...) call is missing required "
f"keyword(s) {missing!r}, got {value!r}.")


def _validate_composite_level_of_theory_call(call: ast.Call, value: str) -> None:
def _validate_composite_level_of_theory_call(call: ast.Call, value: str, field_name: str) -> None:
"""
Structurally validate a ``CompositeLevelOfTheory(...)`` call node against the real Arkane
``CompositeLevelOfTheory`` constructor schema (see
Expand All @@ -198,32 +203,37 @@ def _validate_composite_level_of_theory_call(call: ast.Call, value: str) -> None
Args:
call (ast.Call): The ``CompositeLevelOfTheory(...)`` call node to validate.
value (str): The original ``model_chemistry`` string (used in error messages).
field_name (str): The name of the field ``value`` came from, as the caller knows it
(e.g. ``QMEnergySettings.model_chemistry`` when validating settings, or
``modelChemistry`` when validating a directive in an Arkane source).
Reported verbatim in error messages, so a failure names the field the
caller actually has rather than one from an unrelated context.

Raises:
ValueError: If ``call`` has positional args, ``**kwargs``, a duplicate keyword, a keyword
other than ``freq``/``energy``, a keyword value that is not a valid
``LevelOfTheory(...)`` call, or is missing ``freq`` and/or ``energy``.
"""
if call.args:
raise ValueError(f"QMEnergySettings.model_chemistry's CompositeLevelOfTheory(...) call must not have "
raise ValueError(f"{field_name}'s CompositeLevelOfTheory(...) call must not have "
f"positional arguments, got {value!r}.")
seen_keywords = set()
for kw in call.keywords:
if kw.arg not in _COMPOSITE_LEVEL_OF_THEORY_REQUIRED_FIELDS:
raise ValueError(f"QMEnergySettings.model_chemistry's CompositeLevelOfTheory(...) call only accepts "
raise ValueError(f"{field_name}'s CompositeLevelOfTheory(...) call only accepts "
f"'freq'/'energy' keywords, got {kw.arg!r} in {value!r}.")
if kw.arg in seen_keywords:
raise ValueError(f"QMEnergySettings.model_chemistry's CompositeLevelOfTheory(...) call must not "
raise ValueError(f"{field_name}'s CompositeLevelOfTheory(...) call must not "
f"repeat keyword {kw.arg!r}, got {value!r}.")
seen_keywords.add(kw.arg)
if not (isinstance(kw.value, ast.Call) and isinstance(kw.value.func, ast.Name)
and kw.value.func.id == 'LevelOfTheory'):
raise ValueError(f"QMEnergySettings.model_chemistry's CompositeLevelOfTheory(...) keyword {kw.arg!r} "
raise ValueError(f"{field_name}'s CompositeLevelOfTheory(...) keyword {kw.arg!r} "
f"must be a LevelOfTheory(...) call, got {value!r}.")
_validate_level_of_theory_call(kw.value, value)
_validate_level_of_theory_call(kw.value, value, field_name)
missing = [field for field in _COMPOSITE_LEVEL_OF_THEORY_REQUIRED_FIELDS if field not in seen_keywords]
if missing:
raise ValueError(f"QMEnergySettings.model_chemistry's CompositeLevelOfTheory(...) call is missing "
raise ValueError(f"{field_name}'s CompositeLevelOfTheory(...) call is missing "
f"required keyword(s) {missing!r}, got {value!r}.")


Expand Down Expand Up @@ -251,9 +261,9 @@ def _validate_model_chemistry_expression(field_name: str, value: str) -> None:
_validate_no_injection_chars(field_name, value)
return
if call.func.id == 'LevelOfTheory':
_validate_level_of_theory_call(call, value)
_validate_level_of_theory_call(call, value, field_name)
else:
_validate_composite_level_of_theory_call(call, value)
_validate_composite_level_of_theory_call(call, value, field_name)


# Every QMEnergySettings field carries its own expected-type metadata (see the ``field(...)``
Expand Down Expand Up @@ -543,7 +553,7 @@ def write_hybrid_network_input_file(source_path: str,
raise ValueError("QMEnergySettings.model_chemistry is required and must not be blank: without it, Arkane "
"cannot apply atom energy corrections, so a QM'd transition state's E0 would not be on "
"the same energy reference scale as the RMG wells around it.")
_validate_model_chemistry_expression('model_chemistry', energy_settings.model_chemistry)
_validate_model_chemistry_expression('QMEnergySettings.model_chemistry', energy_settings.model_chemistry)
if not energy_settings.use_atom_corrections:
raise ValueError("QMEnergySettings.use_atom_corrections is False: this directive silently disables "
"Arkane's atom energy corrections, so a QM'd transition state's E0 would not be on the "
Expand Down
65 changes: 65 additions & 0 deletions tests/test_pdep/test_explorer_input_file.py
Original file line number Diff line number Diff line change
Expand Up @@ -1094,6 +1094,71 @@ def test_numeric_arithmetic_is_accepted(self, source):
_validate_source_statements(ast.parse(source), source, '/nonexistent/source.py')


class TestModelChemistryCallFormIsAccepted:
"""
A real ARC/hybrid network source expresses ``modelChemistry`` as a bare
``LevelOfTheory(...)``/``CompositeLevelOfTheory(...)`` call, which Arkane execs at load time.
That is not ``ast.literal_eval``-able, so the literal-assignment rule refused it. This branch is
now bridged to ``t3.pdep.hybrid._validate_model_chemistry_expression`` -- the exact checker T3
uses when it EMITS this directive -- so the two paths agree and no second allowlist is grown.
The exception is keyed strictly to the ``modelChemistry`` target and to a genuine call node; any
other target, and any non-call value, still falls through to the existing refusal.
"""

@staticmethod
def _refuses(source: str) -> str:
with pytest.raises(ValueError) as exc_info:
_validate_source_statements(ast.parse(source), source, '/nonexistent/source.py')
return str(exc_info.value)

@pytest.mark.parametrize('source', [
"modelChemistry = LevelOfTheory(method='wb97xd', basis='def2tzvp')",
"modelChemistry = LevelOfTheory(method='wb97xd', basis='def2tzvp', software='qchem')",
# Explicit `+`, not adjacent-literal concatenation: inside a list of parametrize cases
# the implicit form is indistinguishable from a missing comma, which would silently turn
# this one case into three malformed ones.
("modelChemistry = CompositeLevelOfTheory("
+ "freq=LevelOfTheory(method='wb97xd', basis='def2tzvp'), "
+ "energy=LevelOfTheory(method='dlpno-ccsd(t)', basis='cc-pvtz'))"),
])
def test_the_call_form_for_model_chemistry_is_accepted(self, source):
"""The bare-call form real ARC/hybrid sources use must load, not be refused as non-literal."""
_validate_source_statements(ast.parse(source), source, '/nonexistent/source.py')

@pytest.mark.parametrize('source', [
# A positional arg -- the checker refuses these; the refusal must surface as a source refusal.
"modelChemistry = LevelOfTheory('wb97xd')",
# An unknown keyword.
"modelChemistry = LevelOfTheory(method='wb97xd', bogus='x')",
# A CompositeLevelOfTheory missing a required field.
"modelChemistry = CompositeLevelOfTheory(freq=LevelOfTheory(method='wb97xd'))",
# A non-literal keyword value smuggled into the call.
"modelChemistry = LevelOfTheory(method=().__class__)",
])
def test_a_malformed_call_form_for_model_chemistry_is_still_refused(self, source):
assert 'source.py' in self._refuses(source)

def test_the_refusal_names_the_directive_this_source_actually_has(self):
"""The structural checker is shared with the settings path, whose messages name
``QMEnergySettings.model_chemistry`` -- a field an Arkane source file does not have. A
reader looking at a refused source must be told about ``modelChemistry``, the thing in
front of them, not about a settings attribute from an unrelated context."""
message = self._refuses("modelChemistry = LevelOfTheory(method='wb97xd', bogus='x')")
assert 'modelChemistry' in message
assert 'QMEnergySettings' not in message

@pytest.mark.parametrize('target', ['title', 'basis', 'level_of_theory'])
def test_the_call_form_under_any_other_target_is_still_refused(self, target):
"""The exception is keyed to ``modelChemistry`` alone; the call form elsewhere is non-literal."""
assert 'source.py' in self._refuses(f"{target} = LevelOfTheory(method='wb97xd')")

@pytest.mark.parametrize('value', ['1 + 2', '().__class__', 'foo("x")'])
def test_a_computed_non_call_model_chemistry_value_is_still_refused(self, value):
"""Guards the trap: the checker treats any non-call string as a plain label, so the branch
must gate on a real call node -- a computed ``modelChemistry`` value stays refused."""
assert 'source.py' in self._refuses(f'modelChemistry = {value}')


class TestSourceIsNarrowedToNetworkSourceSyntax:
"""
Codex's round-29 P1 B and P1/P2 D: being a name Arkane defines is not a reason to splice a call
Expand Down
Loading