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

Add interpreter lock release to POMDP check function #158

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AlexBork
Copy link
Contributor

@AlexBork AlexBork commented Feb 14, 2024

The POMDP check function binding is lacking the release of the global interpreter lock that allows other processes to take control of a stopped belief exploration model checker.

Thanks to @TheGreatfpmK for pointing this out.

@volkm
Copy link
Contributor

volkm commented Feb 19, 2024

Can you elaborate a bit more on the change? As far as I understand, py::gil_scoped_release>() releases the Global Interpreter Lock and therefore potentially allows other threads to access the model checker. From your description you want to achieve the opposite though?

@AlexBork AlexBork changed the title Add guard to POMDP check function Add interpreter lock release to POMDP check function Feb 19, 2024
@AlexBork
Copy link
Contributor Author

Sorry, there was a misunderstanding of the problem on my side. The problem was indeed that the model checker was keeping the interpreter lock, so other processes were not able to restart it while it was suspended. I have changed the title and description to account for that.

@volkm
Copy link
Contributor

volkm commented Feb 19, 2024

Thanks for the clarification.
We have to be careful when releasing the GIL to avoid race conditions with multiple threads. I am okay with the change for now, but maybe we should think about general handling of parallelization in the future.
Any opinion, @sjunges?

@sjunges
Copy link
Contributor

sjunges commented Apr 2, 2024

Sorry, I was not aware of this issue. I am also concerned about this, in particular as it depends on the options whether or not this is actuallly code that is meant to run in parallel.

I would be more comfortable with a function run_check or something. We could then have a more general policy that such functions that are meant to run in parallel are always starting with run_*?

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.

None yet

3 participants