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

Test new openfe & gufe rc #254

Merged
merged 6 commits into from
Jun 4, 2024
Merged

Test new openfe & gufe rc #254

merged 6 commits into from
Jun 4, 2024

Conversation

mikemhenry
Copy link
Collaborator

No description provided.

@mikemhenry
Copy link
Collaborator Author

self = <[AttributeError("'DummyProtocol' object has no attribute '_settings'") raised in repr()] DummyProtocol object at 0x7f125c289e70>
settings = {':is_custom:': True, '__class__': 'DummySpecificSettings', '__module__': 'gufe.tests.test_protocol', 'forcefield_sett..._class__': 'OpenMMSystemGeneratorFFSettings', '__module__': 'gufe.settings.models', 'constraints': 'hbonds', ...}, ...}

    def __init__(self, settings: Settings):
        """
        Parameters
        ----------
        settings : Settings
          The parameters for this particular method.  This will be a specialised
          subclass for this particular Protocol.
    
        Note
        ----
        Once the Protocol object is created, the input Settings are frozen,
        so should be finalised before creating the Protocol instance.
        """
>       self._settings = settings.frozen_copy()
E       AttributeError: 'dict' object has no attribute 'frozen_copy'

Hmmm, getting an error in the gufe internals...

@richardjgowers I can look into this but if you want to take just a minute to skim the errors that would be great!

@richardjgowers
Copy link

@mikemhenry it's this PR: https://github.com/OpenFreeEnergy/gufe/pull/169/files

The settings variable there needs to be a Settings subclass, not a dict

@mikemhenry
Copy link
Collaborator Author

Yse I saw that but it looks like alchemiscale is importing the dummy protocol from gufe from gufe.tests.test_protocol import DummyProtocol and then getting the default settings from it protocol=DummyProtocol(settings=DummyProtocol.default_settings()) so it should be the right object, but I will dig to see where it is getting cast into a dict

@mikemhenry
Copy link
Collaborator Author

self = <[AttributeError("'DummyProtocol' object has no attribute '_settings'") raised in repr()] DummyProtocol object at 0x7f1d05f33190> probably just need to check the changes in DummyProtocol

@mikemhenry
Copy link
Collaborator Author

Okay so the actual problem is that the settings object is not getting parsed back into a gufe object, this is the bit https://github.com/OpenFreeEnergy/gufe/blob/main/gufe/tokenization.py#L66 where we re-create the object, but instead of a settings object, we have a serialized settings dictionary:

cls= <class 'gufe.tests.test_protocol.DummyProtocol'> 
args= () 
kwargs= {
    "settings": {
        "__class__": "DummySpecificSettings",
        "__module__": "gufe.tests.test_protocol",
        ":is_custom:": True,
        "forcefield_settings": {
            "__class__": "OpenMMSystemGeneratorFFSettings",
            "__module__": "gufe.settings.models",
            ":is_custom:": True,
            "constraints": "hbonds",
            "rigid_water": True,
            "hydrogen_mass": 3.0,
            "forcefields": [
                "amber/ff14SB.xml",
                "amber/tip3p_standard.xml",
                "amber/tip3p_HFE_multivalent.xml",
                "amber/phosaa10.xml",
            ],
            "small_molecule_forcefield": "openff-2.0.0",
            "nonbonded_cutoff": {
                "magnitude": 1.0,
                "unit": "nanometer",
                ":is_custom:": True,
                "pint_unit_registry": "openff_units",
            },
            "nonbonded_method": "PME",
        },
        "thermo_settings": {
            "__class__": "ThermoSettings",
            "__module__": "gufe.settings.models",
            ":is_custom:": True,
            "temperature": {
                "magnitude": 298.0,
                "unit": "kelvin",
                ":is_custom:": True,
                "pint_unit_registry": "openff_units",
            },
            "pressure": None,
            "ph": None,
            "redox_potential": None,
        },
        "n_repeats": 21,
    }
}

You can see the difference in what the SmallMoleculeComponent looks like here:

cls= <class 'gufe.components.smallmoleculecomponent.SmallMoleculeComponent'> 
args= () 
kwargs= {'rdkit': <rdkit.Chem.rdchem.Mol object at 0x73aeb48518c0>}

@mikemhenry
Copy link
Collaborator Author

This is what is getting "sent" to GUFE:

[
    "DummyProtocol-140583b69a04f875265977f9e5f114d8",
    {
        "settings": {
            "__class__": "DummySpecificSettings",
            "__module__": "gufe.tests.test_protocol",
            ":is_custom:": true,
            "forcefield_settings": {
                "__class__": "OpenMMSystemGeneratorFFSettings",
                "__module__": "gufe.settings.models",
                ":is_custom:": true,
                "constraints": "hbonds",
                "rigid_water": true,
                "hydrogen_mass": 3.0,
                "forcefields": [
                    "amber/ff14SB.xml",
                    "amber/tip3p_standard.xml",
                    "amber/tip3p_HFE_multivalent.xml",
                    "amber/phosaa10.xml",
                ],
                "small_molecule_forcefield": "openff-2.0.0",
                "nonbonded_cutoff": {
                    "magnitude": 1.0,
                    "unit": "nanometer",
                    ":is_custom:": true,
                    "pint_unit_registry": "openff_units",
                },
                "nonbonded_method": "PME",
            },
            "thermo_settings": {
                "__class__": "ThermoSettings",
                "__module__": "gufe.settings.models",
                ":is_custom:": true,
                "temperature": {
                    "magnitude": 298.0,
                    "unit": "kelvin",
                    ":is_custom:": true,
                    "pint_unit_registry": "openff_units",
                },
                "pressure": null,
                "ph": null,
                "redox_potential": null,
            },
            "n_repeats": 21,
        },
        "__qualname__": "DummyProtocol",
        "__module__": "gufe.tests.test_protocol",
        ":version:": 1,
    },
]

@mikemhenry
Copy link
Collaborator Author

mikemhenry commented Mar 7, 2024

@dotsdl @ianmkenney Could I get you guys to take a look at this? I think the issue might be here: https://github.com/openforcefield/alchemiscale/blob/main/alchemiscale/keyedchain.py
Settings isn't a gufe tokenizable, so I think that might be where the breakdown is in this encode/decode loop

This is what I have been using to test:

pytest -v "alchemiscale/tests/integration/interface/test_api.py::TestAPI::test_get_network[Neo4j/5.16 (CE) over 'bolt']" alchemiscale/tests

@dotsdl
Copy link
Member

dotsdl commented Mar 11, 2024

@ianmkenney and I are looking into these failures now @mikemhenry, @richardjgowers; the good news is that I think we encountered something like this before, so will report back once we've characterized it further.

…micalNetwork

Previously, we were relying on `fastapi` to decode the JSON form of the
request, which was not using `gufe.tokenization.JSON_HANDLER.decoder`.
This resulted in `Settings` objects not being turned into `pydantic`
models from `dict` form, which silently worked until now before changes
in `gufe` for 1.0, in which `Protocol` now requires `settings` to be an
actual `Settings` object on init.
@dotsdl
Copy link
Member

dotsdl commented Mar 13, 2024

Think I may have fixed the problem you encountered @mikemhenry; see notes in 328aa66.

@codecov-commenter
Copy link

codecov-commenter commented Mar 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.29%. Comparing base (7d80068) to head (4a84194).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #254   +/-   ##
=======================================
  Coverage   82.28%   82.29%           
=======================================
  Files          26       26           
  Lines        3066     3079   +13     
=======================================
+ Hits         2523     2534   +11     
- Misses        543      545    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mikemhenry
Copy link
Collaborator Author

Awesome! I see how that used to work just fine by accident as well 🙃

@dotsdl
Copy link
Member

dotsdl commented May 1, 2024

@ianmkenney will lead the charge here on getting openfe and gufe 1.0 through QA on alchemiscale.org. Thanks @mikemhenry for getting us started here!

* devtools/conda-envs/alchemiscale-client.yml
* devtools/conda-envs/alchemiscale-compute.yml
* devtools/conda-envs/alchemiscale-server.yml
@dotsdl dotsdl changed the title [DNM] test new openfe & gufe rc Test new openfe & gufe rc Jun 4, 2024
@dotsdl
Copy link
Member

dotsdl commented Jun 4, 2024

Reviewing this next!

Copy link
Member

@dotsdl dotsdl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is finally ready to go! Thanks @mikemhenry and @ianmkenney for your work on this!

@dotsdl dotsdl merged commit b7d67d5 into main Jun 4, 2024
4 checks passed
@dotsdl dotsdl deleted the test/openfe-gufe-1.0.0rc0 branch June 4, 2024 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants