add to_object to the vtable and fix numpy "linking" #43

llllllllll · 2019-04-09T21:28:56Z

No description provided.

include/libpy/numpy_utils.h

ssanderson

Change looks reasonable assuming we're confident in the current and future behavior of PyArray_Scalar. Is there a quick test we can add for this functionality, especially in the any_ref<scoped_ref> case?

ssanderson

LGTM

… we are not on a different version of numpy

…o_object(any_ref)

ssanderson

Changes look good to me. I had some suggestions on the numpy linking description.

ssanderson · 2019-04-11T17:35:28Z

tests/main.cc

@@ -1,6 +1,16 @@
 #include "gtest/gtest.h"
+#include <Python.h>
+
+#define LIBPY_TEST_MAIN


Can we add a comment here linking the user to the comment that explains what this is about?

include/libpy/numpy_utils.h

ssanderson · 2019-04-11T17:46:32Z

include/libpy/numpy_utils.h

@@ -7,6 +7,34 @@
 #include <vector>

 #include <Python.h>
+
+// numpy expects code is being used in a way that has a 1:1 correspondence from source


It might be helpful for a reader to remind what numpy expects a normal c extension to look like, e.g.:

// numpy expects that users will write C extensions that look like this: // my_extension.c #include arrayobject.h ... PyMODINIT_FUNC PyInit_myextension() { import_array(); // Rest of module setup. }

ssanderson · 2019-04-11T18:14:15Z

include/libpy/numpy_utils.h

+// external linkage. Then, because the tests are declaring the same symbol as extern, they
+// will all resolve to the same `PyArray_API` instance and we only need to call
+// `import_array` once in `main.cc`.
+#if defined(LIBPY_COMPILING_FOR_TESTS) && !defined(LIBPY_TEST_MAIN)


Could this instead be:

#if defined(LIBPY_COMPILING_FOR_TESTS) #define PY_ARRAY_UNIQUE_SYMBOL PyArray_API_libpy #if !defined(LIBPY_TEST_MAIN) #define NO_IMPORT_ARRAY #endif #endif

That would remove the need for setting PY_ARRAY_UNIQUE_SYMBOL separately (to the same value) in main.cc, right?

Though, reading this now, I don't actually understand why we need to set PY_ARRAY_UNIQUE_SYMBOL. Why does it matter which symbol gets used for looking up the API functions?

We need to set that macro at all to get it to have external linkage. The name doesn't matter, just defining it does.

Gotcha. So just defining NO_IMPORT_ARRAY by itself doesn't do anything?

ssanderson · 2019-04-11T18:25:51Z

include/libpy/numpy_utils.h

+// numpy expects code is being used in a way that has a 1:1 correspondence from source
+// file to Python extension (as a shared object). Numpy wants to be able to use the Python
+// import system to find the numpy shared object(s), therefore numpy uses it's own linking
+// system where the `numpy/arrayobject.h` will put a `static void** PyArray_API = nullptr`


I think a reader who doesn't already understand this topic might have a hard time following the logic of this paragraph. Here, for example, when we write "numpy uses its own linking system where numpy/arrayobject.h will put a static void** PyArray_API = nullptr name into each TU that includes it.", I'm not sure it's clear how the second half of that sentence follows from the first half, because really the first half of the sentence is introducing the topic of the next 2-3 sentences.

It seems like the goal of this paragraph wants to be to explain to the user how numpy expects things to normally work, so that the next paragraph can explain why the usual way doesn't work for us, so that the paragraph after that can explain why we're doing something unusual to fix the problem.

Partly for checking my understanding, and partly as a suggestion for the structuring this explanation, here's my high-level understanding of the problem being solved here:

Numpy wants callers to be able to use its API functions without having to dynamically link against numpy.

To make that work, numpy expects you to define a static variable named PyArray_API that holds the address of an array of function pointers. Most of numpy's "API functions" are actually macros that resolve to calls through one of these pointers. By default, numpy will define PyArray_API for you when you #include numpy/arrayobject.h, and numpy provides an import_array macro that you're expected to "call" at runtime in your extension setup. import_array uses the Python import machinery to import numpy, find the correct address of its array of function pointers and sets the value of PyArray_API to that address.

The fact that numpy defines PyArray_API for you (as opposed to merely declaring it) is unusual. Headers generally only provide declarations of variables, not definitions, because if you include a header in multiple files that get linked together, you end up with multiple definitions, which is bad. Numpy defines PyArray_API for you anyway, because it assumes by default that you're writing a single-file C extension, which means you're only going to #include numpy/arrayobject.h once.

In our case, however, we're linking lots of test files together into a single executable, so having multiple definitions of PyArray_API is bad. Numpy provides a way to disable its default behavior, which is to define the symbol NO_IMPORT_ARRAY. Defining NO_IMPORT_ARRAY causes numpy to declare PyArray_API as extern "C", which tells the compiler/linker to find the definition of PyArray_API in another ~~castle~~ translation unit.

We want NO_IMPORT_ARRAY to be defined for every #include of numpy/arrayobject.h except one. We do that with some include shenanigans to make it so that main.cc is the one place where that happens.

ssanderson · 2019-04-11T18:34:24Z

include/libpy/numpy_utils.h

+// (`LIBPY_COMPILING_FOR_TESTS`) which will control the `NO_IMPORT_ARRAY` flag. This flag
+// tells numpy to declare the `PyArray_API` flag as an `extern "C" void** PyArray_API`,
+// meaning we expect to have this symbol defined by another object we are to be linked
+// with. In `main.cc` we also set `LIBPY_TEST_MAIN` to disable this feature, but instead


Why is this "instead"? It seems like we're defining PY_ARRAY_UNIQUE_SYMBOL in both places.

ssanderson · 2019-04-11T18:35:19Z

include/libpy/numpy_utils.h

+// single program. This has made it very hard to figure out when and how to initialize the
+// `PyArray_API` value. Instead, we now set a macro when compiling for the tests
+// (`LIBPY_COMPILING_FOR_TESTS`) which will control the `NO_IMPORT_ARRAY` flag. This flag
+// tells numpy to declare the `PyArray_API` flag as an `extern "C" void** PyArray_API`,


Assuming my understanding from above is correct, I might remove the void ** here and below.

The important difference between the two cases here is that one of them is extern "C" and the other one isn't, but that isn't obvious to the reader when they're on this sentence, so they might focus on the void ** rather than the extern "C".

ssanderson · 2019-04-11T18:37:58Z

include/libpy/any.h

@@ -165,10 +180,14 @@ class any_vtable {
        return m_impl->ne(lhs, rhs);
    }

-    inline bool eq(const void* lhs, const void* rhs) {
+    inline bool eq(const void* lhs, const void* rhs) const {


Was this just an oversight? I wonder if we could set up a linter of some sort to suggest obvious missing consts like this.

this just means we don't have a test for this, idk if a linter is the correct tool. These qualifications aren't "obvious" or not, they are part of the API

ssanderson · 2019-04-11T18:40:15Z

include/libpy/numpy_utils.h

-        return ob;
-    }
-};
+struct new_dtype<std::int8_t> : new_dtype_from_typecode<NPY_INT8> {};


This seems like a nice simplification. Not sure this matters, but could this also be a template using to avoid the need for subclassing?

This is an explicit specialization, not just an alias so I don't think a using works here.

ssanderson · 2019-04-11T18:43:45Z

include/libpy/numpy_utils.h

@@ -695,7 +601,7 @@ scoped_ref<> move_to_numpy_array(std::vector<T>&& values) {
        return nullptr;
    }
    return move_to_numpy_array<std::vector<T>, 1>(std::move(values),
-                                                  descr,
+                                                  std::move(descr),


Is there a functional difference here, or is this just to avoid an unnecessary ref inc/dec?

just moving into the function to transfer the reference, no functional difference.

Co-Authored-By: llllllllll <[email protected]>

Joe Jevnik added 2 commits April 9, 2019 17:27

ENH: use PyArray_Scalar to convert anyref to object

deb4269

ENH: add static assertions about sizeof scopedref

2b50f30

ssanderson reviewed Apr 9, 2019

View reviewed changes

include/libpy/numpy_utils.h Outdated Show resolved Hide resolved

ssanderson approved these changes Apr 9, 2019

View reviewed changes

llllllllll force-pushed the numpy-conversion branch from 0287d8f to 205199c Compare April 10, 2019 03:17

BLD: try to clean up how import_array works in the tests

8662ab1

llllllllll force-pushed the numpy-conversion branch 2 times, most recently from c53a9d4 to 8662ab1 Compare April 10, 2019 03:41

TST: add more context on malloc leaks

9ec5940

llllllllll force-pushed the numpy-conversion branch from b30cf34 to 9ec5940 Compare April 10, 2019 04:00

Joe Jevnik added 4 commits April 11, 2019 13:24

TST: add more context on malloc leaks

e77b7c9

DEV: I think these are the same leaks we wanted to supress before but…

102ef8d

… we are not on a different version of numpy

ENH: push to_object into the vtable so there is less dispatching in t…

2afc87e

…o_object(any_ref)

MAINT: rename LIBPY_MAIN to LIBPY_TEST_MAIN

b265313

llllllllll changed the title ~~Numpy conversion~~ add to_object to the vtable and fix numpy "linking" Apr 11, 2019

DOC: clarify why numpy uses import_array

d9e04e7

ssanderson reviewed Apr 11, 2019

View reviewed changes

Scott Sanderson and others added 3 commits April 11, 2019 15:08

Apply suggestions from code review

93b5098

Co-Authored-By: llllllllll <[email protected]>

DEV: fix make format

22e76c0

DOC: update comment

312ab39

llllllllll merged commit a8e85cb into master Apr 16, 2019

llllllllll deleted the numpy-conversion branch April 16, 2019 21:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add to_object to the vtable and fix numpy "linking" #43

add to_object to the vtable and fix numpy "linking" #43

llllllllll commented Apr 9, 2019 •

edited

Loading

ssanderson left a comment

ssanderson left a comment

ssanderson left a comment

ssanderson Apr 11, 2019

ssanderson Apr 11, 2019

ssanderson Apr 11, 2019

llllllllll Apr 11, 2019

ssanderson Apr 11, 2019

ssanderson Apr 11, 2019 •

edited

Loading

ssanderson Apr 11, 2019

ssanderson Apr 11, 2019

ssanderson Apr 11, 2019

llllllllll Apr 11, 2019

ssanderson Apr 11, 2019

llllllllll Apr 11, 2019

ssanderson Apr 11, 2019

llllllllll Apr 11, 2019

add to_object to the vtable and fix numpy "linking" #43

add to_object to the vtable and fix numpy "linking" #43

Conversation

llllllllll commented Apr 9, 2019 • edited Loading

ssanderson left a comment

Choose a reason for hiding this comment

ssanderson left a comment

Choose a reason for hiding this comment

ssanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssanderson Apr 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llllllllll commented Apr 9, 2019 •

edited

Loading

ssanderson Apr 11, 2019 •

edited

Loading