openforcefield / openff-qcsubmit

Automated tools for submitting molecules to QCFractal
https://openff-qcsubmit.readthedocs.io/en/latest/index.html
MIT License
26 stars 4 forks source link

Add `verify` named argument to `to_records` #282

Closed j-wags closed 2 months ago

j-wags commented 3 months ago

Todos

Status

ntBre commented 3 months ago

What do you think about generalizing this to **client_kwargs or something similar? In messing with the caching section of the retrieving_results example, I think providing the cache_dir kwarg to PortalClient automates caching throughout qcsubmit. If you could pass this through to_records that would speed up those calls too!

I also noticed that OptimizationResultCollection.to_basic_result_collection, for example, calls to_records internally, so any kwargs (verify or the ** version) probably need to be propagated anywhere to_records is called.

ntBre commented 3 months ago

I might be getting ahead of myself, but if we decide to support these additional keywords, it might make more sense to have some kind of default_portal_client function that we call instead of PortalClient directly. That way the user can just override that function, and we don't have to worry about propagating kwargs through a huge fraction of the code base. Something like this?

from openff.qcsubmit import default_portal_client
from qcportal import PortalClient

def my_portal_client(addr):
    return PortalClient(addr, verify=False, cache_dir=".")

default_portal_client = my_portal_client
j-wags commented 3 months ago

Oh, I really like the client_kwargs idea, and good catch noticing to_records is called in more places internally. I need to think a little more about how to handle that.

The default_portal_client idea goes in the right direction - if we don't want to thread kwargs through everywhere we'll need to modify some sort of global state. But from what I recall, wrangling global state is a big pain. If we go this direction it might be best to offer users something like a context manager to give them a better handle on things.

j-wags commented 2 months ago

Superseded by #284