Skip to content

ENH: Add inline C function to import and cache Python functions. #5940

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

Merged
merged 1 commit into from
Jun 4, 2015

Conversation

charris
Copy link
Member

@charris charris commented Jun 4, 2015

A new inline function 'npy_cache_pyfunc' is provided for the common
operation of inporting and caching a Python function in static
variables. The intended usage is as follows.

int myfunc()
{
static PyObject *cache = NULL:

npy_cache_pyfunc("the.module.name", "function", &cache);
...

}

Errors are not recoverable, so checked with assert for debugging.

@charris
Copy link
Member Author

charris commented Jun 4, 2015

There are a lot of places this function should be used, and more in the offing.

@jaimefrio
Copy link
Member

Why not do:

*cache = NULL;

if there is an error?

Use of this helper function would then typically look like:

static PyObject *cache = NULL;

npy_cache_pyfunc("the.module.name", "function", &cache);
if (cache == NULL) {
    goto fail;
}

That way you can handle errors more gracefully.

@charris
Copy link
Member Author

charris commented Jun 4, 2015

I had that in there originally, but in most (all?) cases numpy is unusable when there is a failure. The intent is to load python implementations of multiarray functions, and later have explicit loads of ufuncs (n_ops currently set on loading the ufunc module) as implementation of the ndarray operators becomes set. I'd actually prefer to call abort on failure to avoid debug dependency, but assert is probably adequate.

@charris
Copy link
Member Author

charris commented Jun 4, 2015

Note that cache will be NULL on failure as is.

@charris
Copy link
Member Author

charris commented Jun 4, 2015

I'm open to argument on this point, however.

@jaimefrio
Copy link
Member

I think on failure, if the assert statements are turned off, you will never make it outside of the function call, because you are going to either PyModule_GetDict on a NULL pointer, or going to Py_INCREF a NULL pointer, which is going to segfault big time. That's probably the least helpful of error messages we can provide...

You could also make it return an int, so that the standard use would be:

if (npy_cache_pyfunc("the.module.name", "function", &cache) < 0) {
    goto fail;
}

@charris
Copy link
Member Author

charris commented Jun 4, 2015

Yeah, I had that one too ;) I'm leaning towards your first solution.

A new inline function 'npy_cache_pyfunc' is provided for the common
operation of inporting and caching a Python function in static
variables. The intended usage is as follows.

int myfunc()
{
    static PyObject *cache = NULL:

    npy_cache_pyfunc("the.module.name", "function", &cache);
    ...
}

Errors are not recoverable, so checked with assert for debugging.
@charris charris force-pushed the add-python-func-import branch from c17bbb9 to 403d61d Compare June 4, 2015 22:23
@charris
Copy link
Member Author

charris commented Jun 4, 2015

OK, changed. Most of the current uses already check for NULL so that is an easy upgrade.

jaimefrio added a commit that referenced this pull request Jun 4, 2015
ENH: Add inline C function to import and cache Python functions.
@jaimefrio jaimefrio merged commit b1cfccd into numpy:master Jun 4, 2015
@jaimefrio
Copy link
Member

In it goes, thanks!

@seberg
Copy link
Member

seberg commented Jun 5, 2015

If I use it i.e. for the VisibleDeprecationWarning, do you mind if I rename it to _pyobject (honestly don't care much)? ;)

@charris
Copy link
Member Author

charris commented Jun 5, 2015

I'm thinking of renaming it npy_cache_attr since it uses the PyObject_GetAttr function ;)

@charris charris deleted the add-python-func-import branch June 5, 2015 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants