osqp / osqp-python

Python interface for OSQP
https://osqp.org/
Apache License 2.0
110 stars 41 forks source link

Release GIL #25

Closed sbarratt closed 5 years ago

sbarratt commented 5 years ago

Would it be possible to release the GIL before calling into osqp, with the macro Py_BEGIN_ALLOW_THREADS, or are there possible complications?

Similar to how this is done in scs.

bstellato commented 5 years ago

It should be very easy to implement since it is literally two lines of code per API function.

However, the OSQP interface is a bit more complicated than the SCS one since we divide the python commands exactly as in the C API with setup, solve, cleanup, see osqpobjectpy.h. In this way, we store the workspace internally and the OSQP Python object structure contains a pointer to the C workspace. I believe scs-python does not do so. I would probably disable GIL for all the Python functions corresponding to the OSQP C API functions.

It might be useful to write some unittests to make sure the internal OSQP memory does not get corrupted in multi-threading if we disable the GIL. Just a curiosity, for which application do you need to disable the GIL?

EDIT: After thinking about it, I am not 100% sure simply disabling the GIL would work since we are using the workspace structure which is shared between different functions calls.

sbarratt commented 5 years ago

I think releasing the GIL would be most important for the solve function.

The application for disabling the GIL is solving multiple OSQP in parallel, in python, using threads. If the GIL is never released, then the calls will actually be concurrent. Using threads is preferred over processes because you don't have all the overhead of forking a new process, copying memory, etc...

bstellato commented 5 years ago

The problem is that if we release the GIL, the workspace is shared between threads that will probably end up overwriting the ADMM iterates and other variables.

OSQP Python wrapper defines the workspace inside a python object and stores it so that we can reuse it in subsequent solves. SCS does not do that. Instead, it wraps the function scs which calls init, solve, finish all at once allocating and deallocating the whole the workspace.

One solution is to write a similar function for OSQP that executes the setup, solve and cleanup directly without creating an object. This would allocate the workspace, solve the problem and cleanup the allocation in one function call. Releasing the GIL would be easy and safe with it since all the variables, including the workspace, would be local to this new function.

However, if you called OSQP with many threads in this way, I suspect you would find similar bottlenecks as with multiprocessing. This is because when we run setup to create a new workspace, we perform all the initial solver steps (scaling, first factorization, etc.) including copying the problem data into memory. This would happen for all the threads in your program.

Let me know this solution would work for you anyway.

sbarratt commented 5 years ago

The main motivation would be for here: https://github.com/oxfordcontrol/osqpth/blob/master/osqpth/osqpth.py#L93 That is, where you are solving a bunch of (independent) OSQP problems, and you want to do it with multiple threads to take advantage of all your cores.

It seems like setup and solve are called together here.

bstellato commented 5 years ago

PR #29 should fix this issue. It adds a function osqp.solve that does not need any object. It performs setup solve and cleanup immediately and it disables the GIL. @sbarratt could you please try that branch?

In the osqpth repo you mentioned setup and solve are called together. However, I added some comments like this one where I believe it would be better to split setup and solve.

I am not sure what would be the most efficient way to do so (using setup + solve separately VS having a unique function that performs them together disabling the GIL).

sbarratt commented 5 years ago

This looks great to me. I will give it a try.

I do believe that calling setup and solve separately could definitely be beneficial. Maybe we can release the GIL for each separately?

On Fri, Aug 16, 2019 at 11:26 AM Bartolomeo Stellato < notifications@github.com> wrote:

PR #29 https://github.com/oxfordcontrol/osqp-python/pull/29 should fix this issue. It adds a function osqp.solve that does not need any object. It performs setup solve and cleanup immediately and it disables the GIL.

@sbarratt https://github.com/sbarratt could you please try that branch? In the osqpth repo you mentioned setup and solve are called together. However, I added some comments like this one https://github.com/oxfordcontrol/osqpth/blob/c001ec76857af60027337969f9c0e62c7f0a41e0/osqpth/osqpth.py#L25 where I believe it would be better to split setup and solve.

I am not sure what would be the most efficient way to do so (using setup + solve separately VS having a unique function that performs them together disabling the GIL).

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/oxfordcontrol/osqp-python/issues/25?email_source=notifications&email_token=AB7LUGNCT55OB76GU3YGJH3QE3WMTA5CNFSM4H4LGVJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4PLNWQ#issuecomment-522106586, or mute the thread https://github.com/notifications/unsubscribe-auth/AB7LUGI3OVJAGNNN4O5FDYLQE3WMTANCNFSM4H4LGVJA .

bstellato commented 5 years ago

Unfortunately not. If you run setup once and then solve in parallel using threads with no GIL, they will change the workspace variables at the same time. This can create many troubles. The exact same issue applies to the SCS C library by the way.

sbarratt commented 5 years ago

The use case im referring to is for entirely separate problems, with their own workspaces. It would presumably work here, right?

I’m assuming the user won’t be multithreading multiple calls using the same OSQP object, since well warn them that it isn’t thread safe.

On Mon, Aug 19, 2019 at 10:53 AM Bartolomeo Stellato < notifications@github.com> wrote:

Unfortunately not. If you run setup once and then solve in parallel using threads with no GIL, they will change the workspace variables at the same time. This can create many troubles. The exact same issue applies to the SCS C library by the way.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/oxfordcontrol/osqp-python/issues/25?email_source=notifications&email_token=AB7LUGL7CS76ZJ42Q4VAFG3QFLM25A5CNFSM4H4LGVJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4TY4NY#issuecomment-522686007, or mute the thread https://github.com/notifications/unsubscribe-auth/AB7LUGLYTDOLDQFR7LJ73MTQFLM25ANCNFSM4H4LGVJA .

sbarratt commented 5 years ago

I need to point the osqp source to the develop branch, right?

On Mon, Aug 19, 2019 at 11:28 AM Shane Barratt stbarratt@gmail.com wrote:

The use case im referring to is for entirely separate problems, with their own workspaces. It would presumably work here, right?

I’m assuming the user won’t be multithreading multiple calls using the same OSQP object, since well warn them that it isn’t thread safe.

On Mon, Aug 19, 2019 at 10:53 AM Bartolomeo Stellato < notifications@github.com> wrote:

Unfortunately not. If you run setup once and then solve in parallel using threads with no GIL, they will change the workspace variables at the same time. This can create many troubles. The exact same issue applies to the SCS C library by the way.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/oxfordcontrol/osqp-python/issues/25?email_source=notifications&email_token=AB7LUGL7CS76ZJ42Q4VAFG3QFLM25A5CNFSM4H4LGVJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4TY4NY#issuecomment-522686007, or mute the thread https://github.com/notifications/unsubscribe-auth/AB7LUGLYTDOLDQFR7LJ73MTQFLM25ANCNFSM4H4LGVJA .

sbarratt commented 5 years ago

Tried this a few days ago and got a seg fault. I’ll try again this weekend.

Shane

On Mon, Aug 19, 2019 at 1:35 PM Shane Barratt stbarratt@gmail.com wrote:

I need to point the osqp source to the develop branch, right?

On Mon, Aug 19, 2019 at 11:28 AM Shane Barratt stbarratt@gmail.com wrote:

The use case im referring to is for entirely separate problems, with their own workspaces. It would presumably work here, right?

I’m assuming the user won’t be multithreading multiple calls using the same OSQP object, since well warn them that it isn’t thread safe.

On Mon, Aug 19, 2019 at 10:53 AM Bartolomeo Stellato < notifications@github.com> wrote:

Unfortunately not. If you run setup once and then solve in parallel using threads with no GIL, they will change the workspace variables at the same time. This can create many troubles. The exact same issue applies to the SCS C library by the way.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/oxfordcontrol/osqp-python/issues/25?email_source=notifications&email_token=AB7LUGL7CS76ZJ42Q4VAFG3QFLM25A5CNFSM4H4LGVJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4TY4NY#issuecomment-522686007, or mute the thread https://github.com/notifications/unsubscribe-auth/AB7LUGLYTDOLDQFR7LJ73MTQFLM25ANCNFSM4H4LGVJA .

sbarratt commented 5 years ago

Still getting a seg fault. Here's the code I'm running:


import osqp
import numpy as np
from scipy import sparse

# Define problem data
P = sparse.csc_matrix([[4, 1], [1, 2]])
q = np.array([1, 1])
A = sparse.csc_matrix([[1, 1], [1, 0], [0, 1]])
l = np.array([1, 0, 0])
u = np.array([1, 0.7, 0.7])

osqp.solve(P, q, A, l, u)
bstellato commented 5 years ago

There is an issue with printing enabled if that function is called. Let's continue in https://github.com/oxfordcontrol/osqp-python/issues/33 to avoid duplicates.

Without printing this function should be fine.