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

Fix for create_network in interface API, properly deserializing AlchemicalNetwork #276

Merged
merged 4 commits into from
Jun 5, 2024

Conversation

ianmkenney
Copy link
Collaborator

@ianmkenney ianmkenney commented May 30, 2024

Directly handle the request to create_network using the gufe JSON_HANDLER to properly decode the contained Settings objects.

Since these changes also work with gufe < 1.x.x, this PR shouldn't be blocked by #254.

dotsdl and others added 2 commits May 30, 2024 08:28
…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.
Since the "incomplete" network's edges are a subset of the full network,
we need to select transformations from the "incomplete" network. Without
this, there is a small chance that a Transformation is not part of the
incomplete network, which, in our tests we always assume the attached
TaskHub has three available tasks available to claim. Additionally, I've
added a couple more assertions checking these assumptions.
@ianmkenney
Copy link
Collaborator Author

@dotsdl There was a bug in the preloaded database fixture. At first it appeared random (like the supposed race conditions we have seen before). Upon further inspection I couldn't figure out how it could have been. Rerunning the tests many times locally, I finally caught one in the debugger. Long story short, we were trying to actions tasks for transformations that were not part of the alchemical network and our failing test relied on all tasks being actioned to pass.

image

@ianmkenney ianmkenney changed the title [WIP] Fix for create_network in interface API, properly deserializing AlchemicalNetwork Fix for create_network in interface API, properly deserializing AlchemicalNetwork May 30, 2024
@ianmkenney ianmkenney requested a review from dotsdl May 30, 2024 19:32
@dotsdl
Copy link
Member

dotsdl commented Jun 5, 2024

@dotsdl There was a bug in the preloaded database fixture. At first it appeared random (like the supposed race conditions we have seen before). Upon further inspection I couldn't figure out how it could have been. Rerunning the tests many times locally, I finally caught one in the debugger. Long story short, we were trying to actions tasks for transformations that were not part of the alchemical network and our failing test relied on all tasks being actioned to pass.

image

Great detective work on this @ianmkenney! Thanks for fixing this bug in the test suite!

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.

Looks excellent @ianmkenney! Thank you!

@dotsdl dotsdl merged commit 4f2a226 into main Jun 5, 2024
4 checks passed
@dotsdl dotsdl deleted the bugfix/settings_json_decoding branch June 5, 2024 01:09
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.

2 participants