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

error handling in v2 #356

Open
loriab opened this issue Oct 30, 2024 · 1 comment
Open

error handling in v2 #356

loriab opened this issue Oct 30, 2024 · 1 comment

Comments

@loriab
Copy link
Collaborator

loriab commented Oct 30, 2024

For public visibility, I'm collecting here issues and tweaks relevant to error handling in QCElemental schema models and QCEngine harnesses. No changes for v1; these are for v2. See #323 for v2 plans.

Background

  1. The table below is what qcengine.compute(atomicinput) returns, based on keywords and runability (method b3lyp vs. nonsense):
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
  1. FailedOperation has a field input_data with type Any in which the exact submitted input is supposed to go to aid restart. In practice (through qcengine.compute), this is always a dict.
  2. For QCSchema v2, we're planning on an AtomicResult.input_data with type AtomicInput, in keeping with composition rather than inheritance. (Same for other <>Input/<>Result pairs.)

Issues & Changes

  1. type of FailedOperation.input_data by availability — right now (v1), this is always a dict. Sometimes the dict necessary, like when the <>Input model can't be constructed, so the pydantic error is the cause of the FailedOp. But sometimes it’s a dict even when the AtomicInput model is sitting there available (e.g., InputError b/c misspelled “ccsd”). And ResourceError and others presumably have valid Input models.
    • (4a) continue like v1 — AtomicResult.input_data = AtomicInput and FailedOperation.input_data = dict
    • (4b) model when possible for v2 — AtomicResult.input_data = AtomicInput and FailedOperations.input_data = AtomicInput when possible else dict
    • (4c) distinguish by field names for v2 — AtomicResult.input_model = AtomicInput and FailedOperations.input_data = dict
  2. type of FailedOperation.input_data by input type — right now (v1), FailedOp.input_data holds the exact input (dict of model or dict) that was passed to qcengine.compute. Access is uniform since always dict. If, from question above, we switch to model-when-available, then the FailedOp structure starts to differ depending on whether a dict or a model was passed into compute(). Should the field instead hold what was passed into the qcengine harness (after any input dict has already been cast to <>Input)?
    • (5a) stick with exact user input to qcengine.compute() thereby recording no more info than user supplied, at a cost of uneven types
    • (5b) switch to input to qcengine.<harness>.compute() thereby being a model as broadly as possible across <>Result.input_data, FailedOperation.input_data, and either (<>Input or dict) input mode, at a cost of losing the exact fields passed by user for dict input
  3. presence of <>Result.error field — right now (v1), the <>Result models have an error field. From what I can tell, this is left over from early days when a calc always returned an AtomicResult and the success field could be T or F. If the latter, this field is where one could stash a ComputeError until it got handled. This had the advantage of always returning the same model. The new dftd3 and dftd4 interfaces use it this way. In practice, others (qcengine itself and psi4) instead return AtomicResult when successful and FailedOp when unsuccessful.
    • (6a) continue like in v1 and keep the error field
    • (6b) for v2, remove the <>Result.error field to consolidate Result=success and FailedOp=failure
  4. (Added 3 Nov) Literal for FailedOperation.success and <>Result.success Right now these are booleans and convention says latter is False and former is True. It is possible for AtomicResult to set success=F and error=ComputeError (see (6) above).
    • (7a) continue allowing T/F like in v1
    • (7b) for v2, check and reject if Result.success set to F or FailedOp.success set to T
@loriab
Copy link
Collaborator Author

loriab commented Oct 30, 2024

If not obvious, I favor (4b), (5b), (6b) above, and have implemented them locally. (Again, only v2 changing.) But other views and decisions welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant