-
Notifications
You must be signed in to change notification settings - Fork 32
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
Python linsys solver. #11
Conversation
The tests on windows are currently giving NaNs so I've disabled the test and added windows support to the issue list above |
The travis tests are failing since cvxgrp/scs#110 hasn't yet been merged, I'll re-run them once that's been merged in. This may also fix the appveyor tests. |
Slight consistency update to setup.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing to check is for memory leaks. Try running many solves sequentially with new data each time and make sure that the memory usage doesn't climb over time.
/* Note, Python3.x may require special handling for the scs_int and scs_float | ||
* types. */ | ||
static int get_int_type(void) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a problem with these being static?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using them as extern
in my linsys solver, although I could just copy them over there or share them in a cleaner way if you prefer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok that's fine, probably good to prefix them with something like 'scs_' just so they don't collide with any other possible functions in the namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added bamos@67aa92e
test/test_scs_python_linsys.py
Outdated
import sys | ||
import gen_random_cone_prob as tools | ||
|
||
if platform.system() == 'Windows': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll figure out how to fix the windows tests
#endif | ||
|
||
|
||
#ifndef PYTHON_LINSYS | ||
/* release the GIL */ | ||
Py_BEGIN_ALLOW_THREADS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could release the GIL here and then reacquire once we enter each callback (then release at the end of each callback), that would probably be better for multi-threading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I thought about this earlier. This would require sharing the python thread state from this part with the linsys calls. I think the easiest way of adding this would be to have a global _save
variable in scs-module.c
that private.c
modifies as it's acquiring/releasing the GIL around the Python callbacks. What do you think?
Ref: https://docs.python.org/3/c-api/init.html#c.Py_BEGIN_ALLOW_THREADS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I just tried doing this and am getting a segfault and am not sure what else to try. I put this around the main scs
call:
/* release the GIL */
scs_thread_state = PyEval_SaveThread();
/* Solve! */
scs(d, k, &sol, &info);
/* reacquire the GIL */
PyEval_RestoreThread(scs_thread_state);
And this around all of the callbacks:
PyEval_RestoreThread(scs_thread_state);
PyObject_CallObject(scs_solve_lin_sys_cb, arglist);
scs_thread_state = PyEval_SaveThread();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird, ok let's just go with what you have for now then.
Once you resolve the minor comments I have (only major question being to check for memory leakage) I will merge this and then we can iterate on it some more. |
First I tracked the memory for multiple solves with the same data and there aren't any apparent leaks here: print('--- vanilla scs ---')
gc.collect(); print(' + {} bytes'.format(process.memory_info().rss))
for _ in range(10):
sol = scs.solve(
data, cones, verbose=False,
use_indirect=False, normalize=False,
max_iters=10,
)
gc.collect(); print(' + {} bytes'.format(process.memory_info().rss))
...
print('\n\n--- scs with python cbs---')
gc.collect(); print(' + {} bytes'.format(process.memory_info().rss))
for _ in range(10):
sol = scs.solve(
data, cones, verbose=False, use_indirect=False,
normalize=False, max_iters=10,
linsys_cbs=(solve_lin_sys_cb,accum_by_a_cb,accum_by_atrans_cb)
)
gc.collect(); print(' + {} bytes'.format(process.memory_info().rss))
Next I tracked the memory for multiple solves with the different data every time and there aren't any apparent leaks here: print('--- vanilla scs ---')
init_m = process.memory_info().rss
gc.collect(); print(' + {} bytes'.format(init_m))
for _ in range(20):
_G.value = npr.randn(nineq, nz)
data, chain, inv_data = prob.get_problem_data('SCS')
sol = scs.solve(
data, cones, verbose=False,
use_indirect=False, normalize=False,
max_iters=10,
)
gc.collect()
m = process.memory_info().rss
print(' + {} bytes (+ {} bytes)'.format(m, m-init_m))
init_m = m
...
print('\n\n--- scs with python cbs---')
init_m = process.memory_info().rss
gc.collect(); print(' + {} bytes'.format(init_m))
for _ in range(20):
_G.value = npr.randn(nineq, nz)
data, chain, inv_data = prob.get_problem_data('SCS')
sol = scs.solve(
data, cones, verbose=False, use_indirect=False,
normalize=False, max_iters=10,
linsys_cbs=(solve_lin_sys_cb,accum_by_a_cb,accum_by_atrans_cb)
)
gc.collect()
m = process.memory_info().rss
print(' + {} bytes (+ {} bytes)'.format(m, m-init_m))
init_m = m
|
Cool, is this ready to be merged then? |
Almost -- Python=3.6 on OSX is passing the tests for this PR now but the earlier versions of python on OSX aren't. I just tried changing the |
Hmm, the OSX Python builds can now at least compile this without errors but are now failing with Seems weird since the master branch is currently running/passing https://travis-ci.org/bodono/scs-python/jobs/515100158 |
Hmm, I've tried to fix the Python install part of the OSX travis build but it's still failing for reasons that appear to be beyond this PR |
Yeah, the tests can be a real pain, I'll merge this in then and we can try and fix them later. |
This is a nearly-finished PR for a linear system solver that calls back up into Python. It passes an empty
A
matrix into SCS and defers all operations involvingA
to Python. Here is some of the remaining work (if you're fine with the core content of this PR, it may make sense to merge it in sooner and move some of these to the issue tracker)I've added a simple test in the same format as the other tests in
test/test_scs_python_linsys.py
and here's an example use of the interface: