QCElemental 0.23.0 update

[jthorton]

Description

This PR should make QCFractal compatible with qcelemental=0.23.0, we now use the json encoding on dicts when adding or updating results which should remove all non-json serializable objects.

Changelog description

Update QCFractal to work with QCElemental=0.23.0

Status

• Code base linted
• Ready to go

[codecov]

Codecov Report

Merging #694 (d0567b7) into master (867b1bc) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

[jthorton]

It looks like storing the results is working as expected but the dataset view tests seem to have broken, it looks like the results might be a list rather than numpy array? @bennybp any ideas on what's going on here, I would expect the get records to correctly convert the return_result to the correct type? I might be missing a hidden error though.

[bennybp]

Yes, if you can wait for the other features.

This might be a spurious error (that are getting to be a pain in this branch). I will try re-running

[bennybp]

It wasn't a spurious error. The change in qcelemental affects only the properties, not the entire ResultRecord/AtomicResult object.

Does that seem reasonable? Any other modifications?

[jthorton]

@bennybp Thanks for catching this. I am not sure I understand how the properties can be None though the model indicates it should always be a AtomicResultProperties object. Is there a test that checks the properties in the database of a result?

[bennybp]

I think you had the same confusion I did. At that point of the code, the result is a ResultRecord (not an AtomicResult), which has an optional properties. This makes sense because you create a result record before the computation has been run.

(well, it should be optional, but doesn't seem to be, and the default is None):

QCFractal/qcfractal/interface/models/records.py

Lines 286 to 287 in 867b1bc

	properties: qcel.models.ResultProperties = Field(
	None, description="Additional data and results computed as part of the ``return_result``."

[bennybp]

I think we should leave in psi4/label/dev and test against the current master version. Right @loriab ?
I suppose we can leave the pin below

[bennybp]

I have zero idea why this is failing now, and cannot reproduce locally

[bennybp]

Ok seems to be something with alembic on the conda defaults channel not pulling in importlib_resources. The reason for that is a conda-related mystery which I can't be bothered to figure out (since it does seem to be specified as a dependency).

If you install alembic from the default channel, and then do pip install -e . for qcfractal, it then gets installed because pip is smart enough to actually handle dependencies I guess. (but we do --no-deps in the CI)

[bennybp]

Ok ready to merge if you are ok with the psi4 version change

[jthorton]

@bennybp thanks for fixing this, LGTM!