Skip to content
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

Csse layout 550 error #458

Merged
merged 11 commits into from
Nov 26, 2024
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
9 changes: 8 additions & 1 deletion .github/workflows/CI.yml
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,13 @@ jobs:
runs-on: windows-latest
pytest: "-k 'not (hes2 or qchem)'"

# 3.13
#- conda-env: disp-win
# python-version: 3.12
# label: disp-win
# runs-on: windows-latest
# pytest: ""

- conda-env: mace
python-version: "3.10"
label: MACE
Expand Down Expand Up @@ -158,7 +165,7 @@ jobs:
#if: false
run: |
conda remove qcelemental --force
python -m pip install 'git+https://github.com/loriab/QCElemental.git@csse_pyd2_warnings' --no-deps
python -m pip install 'git+https://github.com/loriab/QCElemental.git@csse_layout_536a' --no-deps

# note: conda remove --force, not mamba remove --force b/c https://github.com/mamba-org/mamba/issues/412
# alt. is micromamba but not yet ready for setup-miniconda https://github.com/conda-incubator/setup-miniconda/issues/75
Expand Down
26 changes: 26 additions & 0 deletions devtools/conda-envs/disp-win.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
name: test
channels:
- conda-forge
- nodefaults
dependencies:
- dftd3-python =1.2.0
- gcp-correction
- geometric
- optking
- pymdi

# Core
- python
- pyyaml
- py-cpuinfo
- psutil
- qcelemental
- pydantic=2
- pydantic-settings
- msgpack-python

# Testing
- pytest
- pytest-cov
- codecov

5 changes: 5 additions & 0 deletions docs/source/changelog.rst
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,11 @@ Misc.

MUST (Unmerged)
+++++++++++++++
- When qcengine.compute() fails and forms a fop = FailedOperation (raise_error=T), with v2, `fop.input_data` will be an <>Input model (when possible; if the error was in forming the model, it'll still be a dict), not always a dict like v1.
- When <executor>.compute() fails and collects the input for processing, with v2 it now uses the <>Input model passed to the executor, not the model-or-dict passed into compute().
- The net result of the two above is that whereas fop.input_data in v1 was reliably a dict and its contents would reflect whether a model or dict was passed to qcengine.compute(), now in v2, fop.input_data is a model whenever possible (to mirror <>Result.input_data) regardless of model or dict passed to qcengine.compute(); the only case where it's a dict is if the error was in forming the model.
- DFTD3 & DFTD4 (new intf) - intercept ``v1.AtomicResult`` with ``success=False`` and ``error`` fields set from QCSchema interfaces and return ``FailedOperation``s. Someday when upstream switches to v2, request packages return FaileOp directly and use ``input_error`` rather than ``input error``.
- ``qcengine run`` learned new argument ``--return-version`` analogous to ``qcengine.compute(..., return_version=1|2)`` so CLI matches API capabilities. Note *not* ported to phasing-out ``qcengine run-procedure``.

WIP (Unmerged)
++++++++++++++
Expand Down
10 changes: 9 additions & 1 deletion qcengine/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ def parse_args():
"(ii) A file name, "
"(iii) '-', indicating data will be read from STDIN.",
)
run.add_argument(
"--return-version", default=-1, type=int, help="Schema version to return, if not the input version"
)

run_procedure = subparsers.add_parser(
"run-procedure",
Expand Down Expand Up @@ -214,7 +217,12 @@ def main(args=None):
if command == "info":
info_cli(args)
elif command == "run":
ret = compute(data_arg_helper(args["data"]), args["program"], task_config=task_config)
ret = compute(
data_arg_helper(args["data"]),
args["program"],
task_config=task_config,
return_version=args["return_version"],
)
print(ret.json())
elif command == "run-procedure":
ret = compute_procedure(data_arg_helper(args["data"]), args["procedure"], task_config=task_config)
Expand Down
26 changes: 26 additions & 0 deletions qcengine/compute.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,28 @@ def compute(
AtomicResult, OptimizationResult, FailedOperation, etc., or Dict representation of any object type
A QCSchema representation of the requested output, type depends on return_dict key.

.. _`table:compute_result`:

+-----------+-------------+-------------+--------------------------------------------------+
| good_calc | raise_error | return_dict | output |
+===========+=============+=============+==================================================+
| T | F (def) | F (def) | ``AtomicResult`` object |
+-----------+-------------+-------------+--------------------------------------------------+
| T | T | F (def) | ``AtomicResult`` object |
+-----------+-------------+-------------+--------------------------------------------------+
| T | T | T | dict of ``AtomicResult`` |
+-----------+-------------+-------------+--------------------------------------------------+
| T | F (def) | T | dict of ``AtomicResult`` |
+-----------+-------------+-------------+--------------------------------------------------+
| F | F (def) | F (def) | ``FailedOperation`` object |
+-----------+-------------+-------------+--------------------------------------------------+
| F | T | F (def) | raises ``InputError`` (type encoded in FailedOp) |
+-----------+-------------+-------------+--------------------------------------------------+
| F | T | T | raises ``InputError`` (type encoded in FailedOp) |
+-----------+-------------+-------------+--------------------------------------------------+
| F | F (def) | T | dict of ``FailedOperation`` |
+-----------+-------------+-------------+--------------------------------------------------+

.. versionadded:: 0.50.0
input_data can newly be QCSchema v2 as well as longstanding v1.
The compute_procedure is newly incorporated into compute.
Expand Down Expand Up @@ -113,12 +135,16 @@ def compute(
output_data = executor.compute(input_data, config)
break
except RandomError as e:
if return_version >= 2:
output_data = input_data

if x == config.retries:
raise e
else:
metadata["retries"] += 1
except:
if return_version >= 2:
output_data = input_data
raise

return handle_output_metadata(
Expand Down
6 changes: 4 additions & 2 deletions qcengine/procedures/berny.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
from typing import Any, ClassVar, Dict, Union

import numpy as np
from qcelemental.models.v2 import FailedOperation, OptimizationInput, OptimizationResult
from qcelemental.models.v1 import OptimizationResult
from qcelemental.models.v2 import FailedOperation, OptimizationInput
from qcelemental.util import which_import

import qcengine
Expand Down Expand Up @@ -94,5 +95,6 @@ def compute(
"stdout": log_stream.getvalue(), # collect logged messages
}
)
return OptimizationResult(**output_data)
output_data = OptimizationResult(**output_data)
return output_data.convert_v(2)
return FailedOperation(input_data=input_data, error=error)
4 changes: 3 additions & 1 deletion qcengine/procedures/geometric.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from typing import Any, ClassVar, Dict, Union

from qcelemental.models.v2 import OptimizationInput, OptimizationResult
from qcelemental.models.v1 import OptimizationResult
from qcelemental.models.v2 import OptimizationInput
from qcelemental.util import safe_version, which_import

from .model import ProcedureHarness
Expand Down Expand Up @@ -65,5 +66,6 @@ def compute(self, input_model: "OptimizationInput", config: "TaskConfig") -> "Op
output_data["input_specification"]["extras"].pop("_qcengine_local_config", None)
if output_data["success"]:
output_data = OptimizationResult(**output_data)
output_data = output_data.convert_v(2)

return output_data
12 changes: 9 additions & 3 deletions qcengine/procedures/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,15 @@ def _build_model(
# remember these are user-provided dictionaries, so they'll have the mandatory fields,
# like driver, not the helpful discriminator fields like schema_version.

# for now, the two dictionaries look the same, so cast to the one we want
# note that this prevents correctly identifying the user schema version when dict passed in, so either as_v1/None or as_v2 will fail
mdl = model_wrapper(data, v1_model)
schver = data.get("schema_version")
if schver == 1:
mdl = model_wrapper(data, v1_model)
elif schver == 2:
mdl = model_wrapper(data, v2_model)
else:
# for now, the two dictionaries look the same, so cast to the one we want
# note that this prevents correctly identifying the user schema version when dict passed in, so either as_v1/None or as_v2 will fail
mdl = model_wrapper(data, v1_model)

input_schema_version = mdl.schema_version
if return_input_schema_version:
Expand Down
4 changes: 3 additions & 1 deletion qcengine/procedures/optking.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from typing import Any, ClassVar, Dict, Union

from qcelemental.models.v2 import OptimizationInput, OptimizationResult
from qcelemental.models.v1 import OptimizationResult
from qcelemental.models.v2 import OptimizationInput
from qcelemental.util import safe_version, which_import

from .model import ProcedureHarness
Expand Down Expand Up @@ -55,5 +56,6 @@ def compute(self, input_model: "OptimizationInput", config: "TaskConfig") -> "Op
output_data["input_specification"]["extras"].pop("_qcengine_local_config", None)
if output_data["success"]:
output_data = OptimizationResult(**output_data)
output_data = output_data.convert_v(2)

return output_data
12 changes: 11 additions & 1 deletion qcengine/programs/dftd_ng.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
from typing import Any, ClassVar, Dict

import qcelemental
from qcelemental.models.v2 import AtomicInput, AtomicResult
from qcelemental.models.v2 import AtomicInput, AtomicResult, FailedOperation
from qcelemental.util import parse_version, safe_version, which_import

from ..config import TaskConfig
Expand Down Expand Up @@ -109,6 +109,11 @@ def compute(self, input_model: AtomicInput, config: TaskConfig) -> AtomicResult:

# Run the Harness
output = run_qcschema(input_model)

# d4 qcschema interface stores error in Result model
if not output.success:
return FailedOperation(input_data=input_data, error=output.error.model_dump())

output = output.convert_v(2)

if "info" in output.extras:
Expand Down Expand Up @@ -278,6 +283,11 @@ def compute(self, input_model: AtomicInput, config: TaskConfig) -> AtomicResult:

# Run the Harness
output = run_qcschema(input_model)

# d3 qcschema interface stores error in Result model
if not output.success:
return FailedOperation(input_data=input_data, error=output.error.model_dump())

output = output.convert_v(2)

if "info" in output.extras:
Expand Down
26 changes: 17 additions & 9 deletions qcengine/programs/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,19 +76,27 @@ def build_input_model(
# Note: Someday when the multiple QCSchema versions QCEngine supports are all within the
# Pydantic v2 API base class, this can use discriminated unions instead of logic.

if isinstance(data, qcelemental.models.v1.AtomicInput):
mdl = model_wrapper(data, qcelemental.models.v1.AtomicInput)
elif isinstance(data, qcelemental.models.v2.AtomicInput):
mdl = model_wrapper(data, qcelemental.models.v2.AtomicInput)
v1_model = getattr(qcelemental.models.v1, "AtomicInput")

Choose a reason for hiding this comment

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

Is there a reason to access AtomicInput this way rather than though an import? If so, it would be helpful to add a comment to explain why that is necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not a particularly good reason -- I switched in here in programs/model.py (where it's always AtomicInput) so the analog was clearer with procedures/model.py https://github.com/MolSSI/QCEngine/pull/458/files#diff-2e40dbc50862d2bd8716a45e9dafab44cb3be4499b972dc1d58b8144ef1ab046R63 where the flavor or Input isn't fixed.

v2_model = getattr(qcelemental.models.v2, "AtomicInput")

if isinstance(data, v1_model):
mdl = model_wrapper(data, v1_model)
elif isinstance(data, v2_model):
mdl = model_wrapper(data, v2_model)
elif isinstance(data, dict):
# remember these are user-provided dictionaries, so they'll have the mandatory fields,
# like driver, not the helpful discriminator fields like schema_version.

# for now, the two dictionaries look the same, so cast to the one we want
# note that this prevents correctly identifying the user schema version when dict passed in, so either as_v1/None or as_v2 will fail
mdl = model_wrapper(
data, qcelemental.models.v1.AtomicInput
) # TODO v2 # TODO kill off excuse_as_v2, now fix 2->-1 in schema_versions
schver = data.get("schema_version")
if schver == 1:
mdl = model_wrapper(data, v1_model)
elif schver == 2:
mdl = model_wrapper(data, v2_model)
else:
# for now, the two dictionaries look the same, so cast to the one we want
# note that this prevents correctly identifying the user schema version when dict passed in, so either as_v1/None or as_v2 will fail
# TODO v2 # TODO kill off excuse_as_v2, now fix 2->-1 in schema_versions
mdl = model_wrapper(data, v1_model)

input_schema_version = mdl.schema_version
if return_input_schema_version:
Expand Down
6 changes: 4 additions & 2 deletions qcengine/programs/psi4.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
from pathlib import Path
from typing import TYPE_CHECKING, Any, ClassVar, Dict

from qcelemental.models.v2 import AtomicResult, BasisSet
from qcelemental.models.v1 import AtomicResult as AtomicResult

Choose a reason for hiding this comment

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

It looks like as AtomicResult can be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, that's the second time I've messed that line up. First time was something like ... import AtomicInput as AtomicResult, which was confusing. I've fixed this and added a comment for your previous comment in the next PR.

from qcelemental.models.v2 import BasisSet

Choose a reason for hiding this comment

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

Is BasisSet being used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

from qcelemental.util import deserialize, parse_version, safe_version, which, which_import

from ..exceptions import InputError, RandomError, ResourceError, UnknownError
Expand Down Expand Up @@ -306,4 +307,5 @@ def compute(self, input_model: "AtomicInput", config: "TaskConfig") -> "AtomicRe
# Delete keys
output_data.pop("return_output", None)

return AtomicResult(**output_data)
atres = AtomicResult(**output_data)
return atres.convert_v(2)
4 changes: 3 additions & 1 deletion qcengine/programs/tests/test_molpro.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ def test_molpro_output_parser(test_case):
data = molpro_info.get_test_data(test_case)
inp = qcel.models.v1.AtomicInput.parse_raw(data["input.json"])

output = qcng.get_program("molpro", check=False).parse_output(data, inp).dict()
output = qcng.get_program("molpro", check=False).parse_output(data, inp)
# only qcng.compute() handles schema versions. above returns v2, so need to convert
output = output.convert_v(1).dict()
output.pop("provenance", None)
output.pop("schema_version", None)

Expand Down
30 changes: 25 additions & 5 deletions qcengine/programs/tests/test_programs.py
Original file line number Diff line number Diff line change
Expand Up @@ -255,22 +255,36 @@ def test_mopac_task(schema_versions, request):
assert "== MOPAC DONE ==" in ret.stdout


def test_random_failure_no_retries(failure_engine, schema_versions, request):
def test_random_failure_no_retries_input(failure_engine, schema_versions, request):
_, retver, _ = schema_versions

failure_engine.iter_modes = ["input_error"]
ret = qcng.compute(failure_engine.get_job(), failure_engine.name, raise_error=False, return_version=retver)
ret = checkver_and_convert(ret, request.node.name, "post", vercheck=False)

assert ret.error.error_type == "input_error"
assert "retries" not in ret.input_data["provenance"].keys()
provenance_keys = (
ret.input_data.provenance.model_dump().keys()
if ("_v2" in request.node.name)
else ret.input_data["provenance"].keys()
)
assert "retries" not in provenance_keys


def test_random_failure_no_retries_random(failure_engine, schema_versions, request):
_, retver, _ = schema_versions

failure_engine.iter_modes = ["random_error"]
ret = qcng.compute(failure_engine.get_job(), failure_engine.name, raise_error=False, return_version=retver)
ret = checkver_and_convert(ret, request.node.name, "post", vercheck=False)

assert ret.error.error_type == "random_error"
assert "retries" not in ret.input_data["provenance"].keys()
provenance_keys = (
ret.input_data.provenance.model_dump().keys()
if ("_v2" in request.node.name)
else ret.input_data["provenance"].keys()
)
assert "retries" not in provenance_keys


def test_random_failure_with_retries(failure_engine, schema_versions, request):
Expand All @@ -286,7 +300,10 @@ def test_random_failure_with_retries(failure_engine, schema_versions, request):
)
ret = checkver_and_convert(ret, request.node.name, "post", vercheck=False)

assert ret.input_data["provenance"]["retries"] == 2
retries = (
ret.input_data.provenance.retries if ("_v2" in request.node.name) else ret.input_data["provenance"]["retries"]
)
assert retries == 2
assert ret.error.error_type == "random_error"

failure_engine.iter_modes = ["random_error", "input_error"]
Expand All @@ -299,7 +316,10 @@ def test_random_failure_with_retries(failure_engine, schema_versions, request):
)
ret = checkver_and_convert(ret, request.node.name, "post", vercheck=False)

assert ret.input_data["provenance"]["retries"] == 1
retries = (
ret.input_data.provenance.retries if ("_v2" in request.node.name) else ret.input_data["provenance"]["retries"]
)
assert retries == 1
assert ret.error.error_type == "input_error"


Expand Down
12 changes: 9 additions & 3 deletions qcengine/programs/tests/test_qchem.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ def test_qchem_output_parser(test_case):
inp = qcel.models.v1.AtomicInput.parse_raw(data["input.json"])

outfiles = qcel.util.deserialize(data["outfiles.msgpack"], "msgpack-ext")
output = qcng.get_program("qchem", check=False).parse_output(outfiles, inp).dict()
output = qcng.get_program("qchem", check=False).parse_output(outfiles, inp)
# only qcng.compute() handles schema versions. above returns v2, so need to convert
output = output.convert_v(1).dict()
output.pop("provenance", None)

output_ref = qcel.models.v1.AtomicResult.parse_raw(data["output.json"]).dict()
Expand Down Expand Up @@ -129,7 +131,9 @@ def test_qchem_logfile_parser(test_case):
data = qchem_logonly_info.get_test_data(test_case)
outfiles = {"dispatch.out": data["qchem.out"]}
with pytest.warns(Warning):
output = qcng.get_program("qchem", check=False).parse_logfile(outfiles).dict()
output = qcng.get_program("qchem", check=False).parse_logfile(outfiles)
# only qcng.compute() handles schema versions. above returns v2, so need to convert
output = output.convert_v(1).dict()
output["stdout"] = None

output_ref = qcel.models.v1.AtomicResult.parse_raw(data["output.json"]).dict()
Expand All @@ -152,7 +156,9 @@ def test_qchem_logfile_parser_qcscr(test_case):
outfiles = qcel.util.deserialize(data["outfiles.msgpack"], "msgpack-ext")

with pytest.warns(Warning):
output = qcng.get_program("qchem", check=False).parse_logfile(outfiles).dict()
output = qcng.get_program("qchem", check=False).parse_logfile(outfiles)
# only qcng.compute() handles schema versions. above returns v2, so need to convert
output = output.convert_v(1).dict()
output["stdout"] = None

output_ref = qcel.models.v1.AtomicResult.parse_raw(data["output.json"]).dict()
Expand Down
Loading
Loading