[WIP] Detect proton transfer and add error handling logic to QM export

[j-wags]

Description

• [x] Address #38 and #60
• [x] Addresses #58 (by considering it an error and not trying to salvage the molecule).

Questions

• [x] Will this filter all QM exports, or just one route (server vs. execute?)
• [ ] If we run this on a large set of stored results, do we get any structures filtered that we don't expect?
• [ ] Will adding error_dir/ to the output break downstream steps?

Status

• [x] Ready for review

[j-wags]

Ahhh, I completely missed the scope of the different execution/export pathways. I probably should have glanced closer at the architecture before jumping into this PR.

@dotsdl Lets see if we can chat tomorrow on the questions above. I could see a few paths through these decisions.

[dotsdl]

@j-wags thank you again for starting this off!

I've finished my refactor, and added tests to hit all three export pathways optimize {export, execute-from-server, execute} for the proton transfer failure mode.

As for error_mols usage, we have a couple options. We could have molecules that fail to execute for optimize {execute-from-server, execute} or fail to export for all three pathways

1. deposit all outputs in <spec_name>/error_mols; we do not write an SDF file
2. deposit all outputs in <spec_name>/; we do not write an SDF file

(1) is what we implement in this PR. (2) is how we have operated so far for compute. Some thoughts:

• (1) is more consistent with the behavior of the preprocessing steps in the pipeline
• (1) makes it fairly obvious what molecules have errored; user doesn't have to check for SDF files that aren't present, or parse json files for error output
• (2) does away with error_mols entirely, which would reduce the complexity of the export logic

I am fine with either approach. What do you prefer?

[j-wags]

Reviewing now. I agree with your decision in the last post -- I think it's a much better UX for the errors to live in their own directory. That way, the user gets a clear view into the program's intention when they were getting written out.

[j-wags]

Well, I can't explicitly "approve" this PR because I'm an author, but I've:

• read over the code and found no major objections
• run local tests on a few different situations (all good, some bad, all bad) and seen the proper behavior
• tested downstream steps on the output

and this seems to be good to me.

I approve this merge.

There are a few situation where we might have additional benefit from outputting SDFs to error_mols/ where the connectivity had rearranged, but the benefit is marginal, and it shouldn't hold up this merge. If @dotsdl thinks this is important, we can open an issue to add it later.

Nice work @dotsdl. Reading over this PR, I apologize for not providing more context for my changes, and making you incur the overhead of starting with my diffs without context. This looks quite nice in the end :-)

(Let me know if you can't click merge due to permissions and I'm happy to do it)

[j-wags]

In fact, to resolve ambiguity, I'll just go ahead and merge directly 👍

[dotsdl]

Thank you @j-wags! Much appreciated!