-
Notifications
You must be signed in to change notification settings - Fork 13
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
Context still thinks it has an active session after Session.close() #214
Comments
The fix for GROMACS will not be in currently tagged 0.0.7 releases, so I will use the feature named |
Referemce kassonlab#214 * Add `gmx.core.has_feature` binding to `gmxapi::Version::hasFeature` * Check whether workaround is necessary for bug kassonlab#214 * Reassign a new `gmx.core.Context` to `gmx.context.Context._api_object` if bugfix not reported present in libgmxapi
Related: Somehow, there has been a circular reference between Context and WorkSpec since 7b17db2 in February. We never rely on Python garbage collection for anything, so this is a resource leak and could cause weird behavior when the interpreter exits, I don't think it causes any runtime problems that are noticeable or testable. However, |
Referemce kassonlab#214 * Add `gmx.core.has_feature` binding to `gmxapi::Version::hasFeature` * Check whether workaround is necessary for bug kassonlab#214 * Reassign a new `gmx.core.Context` to `gmx.context.Context._api_object` if bugfix not reported present in libgmxapi
Referemce kassonlab#214 * Add `gmx.core.has_feature` binding to `gmxapi::Version::hasFeature` * Check whether workaround is necessary for bug kassonlab#214 * Reassign a new `gmx.core.Context` to `gmx.context.Context._api_object` if bugfix not reported present in libgmxapi * Avoid circular reference by only allowing WorkSpec to hold weak ref to context. * Update and expand testing
Referemce kassonlab#214 * Add `gmx.core.has_feature` binding to `gmxapi::Version::hasFeature` * Check whether workaround is necessary for bug kassonlab#214 * Reassign a new `gmx.core.Context` to `gmx.context.Context._api_object` if bugfix not reported present in libgmxapi * Avoid circular reference by only allowing WorkSpec to hold weak ref to context. * Update and expand testing
Updates to the Python package appear to have resolved these issues. libgmxapi may or may not receive a patch to implement the fix upstream, but I'm leaving this issue open for now as a reminder. |
To do: check on the resolution of this before 0.0.7.4 patch release. |
Will be resolved with https://gitlab.com/gromacs/gromacs/-/issues/4422. As part of that update, the scope of the |
ContextImpl::launch() tries to avoid reentrance problems in libgromacs by keeping track of a session launched from it using a weak pointer, but this has increasingly led to false positives in ongoing development. We now require that a Session be
close()
d to force the shutdown of the mdrunner at a known point. After the call toclose()
, the Context should be able to launch a new session.Before simply updating the weak pointer check and reassignment, though, I need to make sure we don't have anything silly where keeping a handle to a closed session assumes that the ContextImpl can still find that session.
This bug probably wasn't more obvious because of inconsistent Python garbage collection, but ultimately the C++ API should handle the situation better. It is related to various implementation details of WorkSpec, Context, and Session, that are due for overhaul (the subject of a growing number of other issues) but this bug should be fixed for 0.0.7 ASAP.
The text was updated successfully, but these errors were encountered: