Changes to `openff-qcsubmit` to use `next` QCFractal

[dotsdl]

Description

The next major iteration of QCFractal is coming soon, and these changes are those necessary for QCSubmit to be used with it.

• [x] Easy (straightforward/syntax) fixes in test_submissions
• [x] Figure out how to wire pcm back in in test_submissions.py::test_optimization_submissions_with_pcm
• [x] Easy (straightforward/syntax) fixes in test_datasets
• [x] Hard (info content/architecture) fixes in test_datasets
• [x] Easy (straightforward/syntax) fixes in tests/results/
  • Note: An example of quadrupole info is in records_and_molecules[0][0].properties['scf quadrupole'] after calling records_and_molecules = collection.to_records() on the COH single point set
• [x] ~Why is test_results.py::test_collection_from_server[BasicResultCollection-OpenFF BCC Refit Study COH v1.0-spec_1-91-191] returning 190 results when it used to return 191?~ The spec names were lost and one of the two specs had an errored calc
• [x] Figure out a new way for maxiter and scf keywords to get in submissions. Previously these were added by the QCSpec class from common_structures (which has values for those as defaults)
• [x] Figure out how to do torsiondrive mocking in test_results.py::test_to_records[collection2-record2]
• [ ] Re-add find_existing to _BaseDataset.submit (datasets.py around line 220) (or open an issue to do in a separate PR)
• [x] Un-ignore tests/results
• [x] Add pre-commit config back
• [ ] Rename specs in single point datasets (cc https://github.com/MolSSI/QCFractal/issues/739)
• [ ] Is s99f now unloadable via openmmforcefields?

Release notes

• Note that QCF record IDs now fail equality checks with str, but succeed with int.
• QCSubmit's dataset.get_qc_spec method is gone now, including its kwarg keyword_id (it's not clear to me what this means or where it's used)
• OptimizationDataset driver is now deferred (used to be gradient)
• pcm is currently unstable, so the test is marked as xfail
• Many tests now just run short jobs instead of mocking data from the QCA server
• cmiles is now found on all entries created in new submissions
• All to_records methods now warn about entries with invalid cmiles and then exclude them from the returned values.
• OptimizationDataset.to_basic_result_collection will now warn if it's unable to find results with the desired driver and will exclude them from the returned values.

Dev install instructions

mamba env create -n qcsubmit-dev -f basic.yaml
mamba install -c conda-forge -c openeye openeye-toolkits jupyterlab
mamba install -c conda-forge -c conda-forge/label/libint_dev psi4

In a different folder

git clone -b next git@github.com:MolSSI/QCFractal.git # really important to check out this branch directly, otherwise you'll get a bunch of empty folders preventing pip installation
cd QCFractal
pip install ./qcportal ./qcarchivetesting ./qcfractalcompute ./qcfractal

To prevent posgresql's "not enough shared memory" issue:

sudo sysctl -w kern.sysv.shmall=65536
sudo sysctl -w kern.sysv.shmmax=16777216

Finally in the openff-qcsubmit folder

pip install -e ./

When running pytest, sometimes I'll get a postgres issue about a temporary file path being too long. I can fix that by setting --basetemp in the pytest invocation to something short-ish:

pytest openff/qcsubmit/tests/test_submissions.py --basetemp="/Users/jeffreywagner/temp"  -vvvvv

QCF next docs: https://molssi.github.io/QCFractal/user_guide/records/base.html#qcportal.dataset_models.BaseDataset.submit

Status

• [ ] Ready to go

[lgtm-com[bot]]

This pull request introduces 1 alert when merging ce56294a1ae581a7e451f59e8560b9a208ab13ff into fff75903edd3c02ba5a57e0d0292323ae750cc85 - view on LGTM.com

new alerts:

• 1 for Unused import

[codecov[bot]]

Codecov Report

Merging #195 (73136c3) into main (c4d3313) will increase coverage by 0.20%. Report is 2 commits behind head on main. The diff coverage is 95.55%.

Additional details and impacted files

[dotsdl]

Live testing with this PR: https://github.com/openforcefield/qca-dataset-submission-next-test/pull/4

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 20ee05e1597d1676b46cb74fd1ad437b82780a52 into fff75903edd3c02ba5a57e0d0292323ae750cc85 - view on LGTM.com

new alerts:

• 1 for Unused import

[lgtm-com[bot]]

This pull request introduces 1 alert when merging c92634726d222f6fca7795ab274eb1dd80aedb63 into fff75903edd3c02ba5a57e0d0292323ae750cc85 - view on LGTM.com

new alerts:

• 1 for Unused import

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 64dbf1240db80447fb5f7cd0ee183dff3abfe509 into fff75903edd3c02ba5a57e0d0292323ae750cc85 - view on LGTM.com

new alerts:

• 1 for Unused import

[lgtm-com[bot]]

This pull request introduces 1 alert when merging e3b15e6d06fc598691e3a10d20a8e38409c626b7 into fff75903edd3c02ba5a57e0d0292323ae750cc85 - view on LGTM.com

new alerts:

• 1 for Unused import

[lgtm-com[bot]]

This pull request introduces 1 alert when merging f36c9e28701df7e6944f76c403f3f2fdd170ad8b into fff75903edd3c02ba5a57e0d0292323ae750cc85 - view on LGTM.com

new alerts:

• 1 for Unused import

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 667ed247477d8020f8a9ef45e2662ae35062fd57 into fff75903edd3c02ba5a57e0d0292323ae750cc85 - view on LGTM.com

new alerts:

• 1 for Unused import

[lgtm-com[bot]]

This pull request introduces 1 alert when merging e6147ab97608dc2fb96087eb264d21501ac61abe into fff75903edd3c02ba5a57e0d0292323ae750cc85 - view on LGTM.com

new alerts:

• 1 for Unused import

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 0956d90a92603b556bf1c8b11d0adf95232e4c23 into fff75903edd3c02ba5a57e0d0292323ae750cc85 - view on LGTM.com

new alerts:

• 1 for Unused import

[lgtm-com[bot]]

This pull request introduces 1 alert when merging e44eaa9c1beef0a41ad10f3d50cbd619cfd42315 into fff75903edd3c02ba5a57e0d0292323ae750cc85 - view on LGTM.com

new alerts:

• 1 for Unused import

[lgtm-com[bot]]

This pull request introduces 2 alerts when merging f1426586785b8307eedcf75b075ad6d2462c1089 into e4d5add8f169b7c358fb518358f12699923cee4d - view on LGTM.com

new alerts:

• 2 for Unused import

[lgtm-com[bot]]

This pull request introduces 3 alerts when merging e2eaf09a9ceb234bb622c3122a50468b283064ff into e4d5add8f169b7c358fb518358f12699923cee4d - view on LGTM.com

new alerts:

• 3 for Unused import

[lgtm-com[bot]]

This pull request introduces 3 alerts when merging 727c8f9279ba86c3a4b7c5b4371930dfba1dd795 into e4d5add8f169b7c358fb518358f12699923cee4d - view on LGTM.com

new alerts:

• 3 for Unused import

[lgtm-com[bot]]

This pull request introduces 3 alerts when merging a85d635790feb766c7cc99ed250537ccecd4eda0 into e4d5add8f169b7c358fb518358f12699923cee4d - view on LGTM.com

new alerts:

• 3 for Unused import

[dotsdl]

@bennybp please push your changes here when you get a chance. Thanks!

[j-wags]

Currently having trouble getting tests running locally (I arbitrarily started at test_submission) - the snowflake fixture is complaining that postgres isn't configured. Looking elsewhere, it looks like I don't need to run qcfractal-server init-db if the tests are just using snowflake, but I DO need to run it if I'm testing a full server (which I don't think we're doing here)

I've also tried running with --fractal-uri="snowflake", but this doesn't appear to make a difference.

===================================================================================== ERRORS ======================================================================================
___________________________________________________ ERROR at setup of test_basic_submissions_single_spec[PSI4 hf 3-21g energy] ____________________________________________________

tmp_path_factory = TempPathFactory(_given_basetemp=None, _trace=<pluggy._tracing.TagTracerSub object at 0x17516ce50>, _basetemp=PosixPath...s/v9/2_5kkyp10z37wvxz8b3rl6_h0000gn/T/pytest-of-jeffreywagner/pytest-11'), _retention_count=3, _retention_policy='all')

    @pytest.fixture(scope="session")
    def postgres_server(tmp_path_factory):
        """
        A postgres server instance

        This does not contain any databases (except the postgres defaults).

        This is built only once per session, and automatically deleted after the session. It uses
        a pytest-provided session-scoped temporary directory
        """

        db_path = str(tmp_path_factory.mktemp("db"))
>       test_pg = QCATestingPostgresServer(db_path)

../../../conda/envs/qcsubmit-test-basic/lib/python3.11/site-packages/qcarchivetesting/testing_fixtures.py:31: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../../../conda/envs/qcsubmit-test-basic/lib/python3.11/site-packages/qcarchivetesting/testing_classes.py:74: in __init__
    self.tmp_pg = TemporaryPostgres(data_dir=db_path)
../../../conda/envs/qcsubmit-test-basic/lib/python3.11/site-packages/qcfractal/postgres_harness.py:664: in __init__
    self._harness.initialize_postgres()
../../../conda/envs/qcsubmit-test-basic/lib/python3.11/site-packages/qcfractal/postgres_harness.py:544: in initialize_postgres
    self.start()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <qcfractal.postgres_harness.PostgresHarness object at 0x17516cc50>

    def start(self) -> None:
        """
        Starts a PostgreSQL server based off the current configuration parameters.

        The PostgreSQL server must be initialized and the configured port open. The database does not need to exist.
        """

        # We should only do this if we are in charge of the database itself
        assert self.config.own

        # Startup the server
        self._logger.info("Starting the PostgreSQL instance")

        # We should be in charge of this postgres process. If something is running, then that is a problem
        if is_port_inuse(self.config.host, self.config.port):
            raise RuntimeError(
                f"A process is already running on port {self.config.port} that is not associated with this QCFractal instance's database"
            )
        else:
            retcode, stdout, stderr = self.pg_ctl(["start"])

            err_msg = f"Error starting PostgreSQL. Did you remember to initialize it (qcfractal-server init)?\noutput:\n{stdout}\nstderr:\n{stderr}"

            if retcode != 0:
>               raise RuntimeError(err_msg)
E               RuntimeError: Error starting PostgreSQL. Did you remember to initialize it (qcfractal-server init)?
E               output:
E               waiting for server to start.... stopped waiting
E               
E               stderr:
E               pg_ctl: could not start server
E               Examine the log output.

../../../conda/envs/qcsubmit-test-basic/lib/python3.11/site-packages/qcfractal/postgres_harness.py:452: RuntimeError

[j-wags]

Hah, so my local snowflake failures were because of the following (from the postgres log):

E               log contents:
E               2023-07-05 11:24:07.864 PDT [47226] LOG:  starting PostgreSQL 15.3 on x86_64-apple-darwin13.4.0, compiled by clang version 15.0.7, 64-bit
E               2023-07-05 11:24:07.866 PDT [47226] LOG:  listening on IPv4 address "127.0.0.1", port 64387
E               2023-07-05 11:24:07.866 PDT [47226] LOG:  listening on IPv6 address "::1", port 64387
E               2023-07-05 11:24:07.866 PDT [47226] LOG:  Unix-domain socket path "/private/var/folders/v9/2_5kkyp10z37wvxz8b3rl6_h0000gn/T/pytest-of-jeffreywagner/pytest-19/db0/.s.PGSQL.64387" is too long (maximum 103 bytes)
E               2023-07-05 11:24:07.866 PDT [47226] WARNING:  could not create Unix-domain socket in directory "/private/var/folders/v9/2_5kkyp10z37wvxz8b3rl6_h0000gn/T/pytest-of-jeffreywagner/pytest-19/db0"
E               2023-07-05 11:24:07.866 PDT [47226] FATAL:  could not create any Unix-domain sockets
E               2023-07-05 11:24:07.866 PDT [47226] LOG:  database system is shut down

Adding --basetemp=/Users/jeffreywagner/temp to my pytest invocations solved this, now I get the same error as the GitHub CI.

[j-wags]

An evolving notes post (mostly for myself, will be editing this a lot)

• Big picture, I want to look into whether QCSubmit can drop its dataset classes and just use the new ones from QCFractal. QCSubmit's Factory classes could then just directly produce QCF datasets.
• I've been told that QCSubmit can now drop most/all of its caching functionality. I have yet to look into that/understand its current caching functionality.
• It'd be helpful if the QCATestingSnowflake class came with a worker or two. Right now it's hard-coded at 0
• I should update the tests to not load molecules from PDB

[bennybp]

* I've been told that QCSubmit can now drop most/all of its caching functionality. I have yet to look into that/understand its current caching functionality.

There's some caching in datasets, but not much in the client. I do have a few todo items around there with saving the cache for later use.

* It'd be helpful if the QCATestingSnowflake class came with a worker or two. Right now it's [hard-coded at 0](https://github.com/MolSSI/QCFractal/blob/next/qcarchivetesting/qcarchivetesting/testing_classes.py#L165)

For your testing you might be able to get away with a plain FractalSnowflake rather than the testing one. Although I guess it depends on what tests you want. I can always add workers to the QCATestingSnowflake

[j-wags]

Thanks for the pointers @bennybp!

[j-wags]

I spoke with @mattwthompson yesterday and he pointed out that we should keep the QCSubmit dataset classes, since they're the only available dataset class that holds OFFMols natively, and he plans to use them for the internal benchmarking project.

[ntBre]

My last commit got the basic test_optimization_submissions passing, but I'm a bit stuck on test_optimization_submissions_with_constraints. It really looks like the constraints are not being propagated to the QCPortal stuff. I can push what I have so far if anyone is interested in seeing it, but I both can't find constraints in any of the record.*.keywords fields and the final angle measurements are failing. Is anyone more familiar with how QCPortal is handling constraints? Probably that's not even the right question to ask. I can't find anything in their OptimizationRecord or OptimizationDataset code that even mentions constraints.

[jthorton]

Hi @ntBre, Thanks for taking this on and for all the changes so far. I need to dig into this a little more but I think that the constraints are being missed by qcfractal. Here is my thought thread and I'll cc: @bennybp to make sure I am on the right lines.

For optimisation datasets where we need unique constraints for each molecule we store the constraints in the additional_keywords field which should be combined with the specification keywords to create each molecules unique task. I see that in the next branch the OptimizationDatasetEntryORM model still has this field. The OptimizationRecordORM model however does not, and it is this second model which is used to create the optimization task here. To fix this I think the OptimizationRecordORM needs extending to include the additional_keywords and they then need to be combined with the opt_keywords here before creating the task. @bennybp it would be great if you could check this I might have missed something trying to follow the new code!

[j-wags]

(From a meeting today) BP -- In the new QCFractal (which may differ in design philosophy from QCSubmit) the additional_keywords should start in the entry in the dataset, then running dataset.submit should make OptimizationRecords where the additional_keywords get into each OptimizationRecord's specification.

In today's debugging, QCSubmit's datasets.py line 1032 seems like the place where additional keywords should be propagated, looks like examples of previous propagation are around line 1406.

[jthorton]

Thats great following that I notice that we also need to pass the attributes to each of the new dataset entry objects in each of the _get_entries methods.

[j-wags]

Temporarily unhooking find_existing kwarg from _BaseDataset.submit since it's not in a built QCFractal next package yet, so it's causing CI to fail. Added a to-do in PR body text so we don't forget.

[ntBre]

I tried debugging test_submissions.py::test_optimization_submissions_with_pcm today, and I can't figure out how to get Psi4 to calculate the quadrupole. I've never used Psi4 before, but I'm assuming older versions always printed the quadrupole, and now they only print the dipole. Although that doesn't line up with the fact that the main branch tests are passing with psi4 1.8.1. Anyway, this input:

memory 600 mb

molecule h2o {
  O
  H 1 0.96
  H 1 0.96 2 104.5
}

set basis cc-pVDZ
energy('hf')

Produces this multipole section:

 Multipole Moments:

 ------------------------------------------------------------------------------------
     Multipole            Electronic (a.u.)      Nuclear  (a.u.)        Total (a.u.)
 ------------------------------------------------------------------------------------

 L = 1.  Multiply by 2.5417464519 to convert [e a0] to [Debye]
 Dipole X            :          0.0000000            0.0000000            0.0000000
 Dipole Y            :          0.0000000            0.0000000            0.0000000
 Dipole Z            :         -0.1680325            0.9783140            0.8102815
 Magnitude           :                                                    0.8102815

 ------------------------------------------------------------------------------------

Changing the energy line to either properties('hf', properties=['dipole', 'quadrupole']) or

e, wfn = energy('hf', return_wfn=True)
oeprop(wfn, 'QUADRUPOLE')

produces the quadrupole moments.

I tried setting keywords on this line (it should be propagated from somewhere else, but this is the most direct place to test), but none of them worked. In particular, I tried dict(properties=['quadrupole']), which psi4 actually seems to accept, as well as dict(oeprop="quadrupole"), which didn't work, and the more exotic {"not a real keyword": "quadrupole"}, which helped me confirm that Psi4 was actually getting these keywords by throwing an error from the psi4 driver.

I was referencing this page in the documentation, so hopefully I didn't miss anything too obvious.

This assertion is easy to fix by making SCF DIPOLE scf dipole, but I didn't think that was even worth pushing by itself. In my debugging, I also noticed that result.extras is empty, so that assertion will fail even once the quadrupole issue is resolved.

[j-wags]

Gonna go ahead and merge and make an 0.50.0rc1 release, then will build a non-main label package to make sure things don't go sideways!

[jthorton]

Thanks so much, everyone, fantastic to qcsumit back online!