Skip to content

NPY_NO_DEPRECATED_API check is backwards #3008

Closed
erg opened this Issue Feb 20, 2013 · 23 comments

6 participants

@erg
erg commented Feb 20, 2013

There's an annoying warning on all of my files for basically every project that uses numpy:

/usr/lib/python2.7/site-packages/numpy/core/include/numpy/npy_deprecated_api.h:11:2: warning: #warning "Using deprecated NumPy API, disable it by #defining NPY_NO_DEPRECATED_API NPY_1_7_API_VERSION" [-Wcpp]

Looking at the check in numpy/core/include/numpy/ndarraytypes.h:

#if !(defined(NPY_NO_DEPRECATED_API) && (NPY_API_VERSION <= NPY_NO_DEPRECATED_API))
#include "npy_deprecated_api.h"
#endif

The user is supposed to set NPY_NO_DEPRECATED_API to the API version that they wish to check their codebase against for deprecated code, and the version #defines are just integers. So setting it to 7 should check that you are clean against numpy 1.7. Also, the NPY_API_VERSION should be greater than the version you're trying to check against.

Lengthy explanation, skip this if you're good at boolean algebra...

Looking at the check, you don't want to include npy_deprecated_api.h, so you want the inside of the check to be true so the not evaluates to false and the #if is skipped. The first check NPY_NO_DEPRECATED_API is True, the second check NPY_API_VERSION <= NPY_NO_DEPRECATED_API is 8 <=7, which is false. So the inside evaluates to false, we take the not of that and include the file.

Changing the check to this fixes it:

#if !(defined(NPY_NO_DEPRECATED_API) && (NPY_API_VERSION >= NPY_NO_DEPRECATED_API))
#include "npy_deprecated_api.h"
#endif
@charris
NumPy member
charris commented Feb 23, 2013

Should probably be > rather than >= ? Otherwise looks like the correct fix.

@erg
erg commented Feb 23, 2013

Wouldn't that mean that you couldn't test out if you were 1.7-deprecated-clean with numpy 1.7?

@njsmith
NumPy member
njsmith commented Mar 1, 2013

No, I believe the original check is correct, at least for the way this mechanism is set up. Setting NPY_NO_DEPRECRATED_API to 7 means "I have checked my code against numpy 1.7, and it doesn't use any APIs that were deprecated in 1.7". When compiling such code against numpy 1.7 (or earlier), the correct thing to do is to disable those deprecated APIs, so the developer can check that their assertion is true. When compiling such code against numpy 1.8+, though, there are a new set of deprecated APIs (including ones that were supported in 1.7), and your code might need those APIs, so we have to enable them -- it's like you haven't set NPY_NO_DEPRECATED_API at all.

The #warning message is wrong -- it should tell you to set it to NPY_1_8_API_VERSION. We need some mechanism to update this message when new versions are released, whether that's some stringification magic or a note in the release checklist recording that this is yet another place the version number is stored and needs to be updated by hand. Maybe something like this would work? My cpp is rusty:

#warning "Using deprecated NumPy API, disable it by #defining NPY_NO_DEPRECATED_API NPY_1_" ## NPY_API_VERSION ## "_API_VERSION"

Also this current implementation is somewhat ill-conceived all around, since it assumes that everything that was deprecated in release N will be removed in release N+1. Wouldn't it be better to do something like

#ifndef NPY_NO_DEPRECATED_API
#define NPY_NO_DEPRECATED_API 0
#endif

#if NPY_NO_DEPRECATED_API < NPY_API_VERSION
#warning "..."
#endif

#if NPY_NO_DEPRECATED_API < NPY_1_8_API_VERSION
/* APIs that were deprecated in numpy 1.8 but that have not yet been removed */
#endif

#if NPY_NO_DEPRECATED_API < NPY_1_7_API_VERSION
/* APIs that were deprecated in numpy 1.7 but that have not yet been removed */
#endif

/* etc. */

Note also that currently old_defines.h has its own independent version of this check (even though it is only included by npy_deprecated_api.h). Probably its contents should just be dropped into npy_deprecated_api.h?

@rgommers
NumPy member
rgommers commented Mar 2, 2013

I agree with @erg and @charris that the current check makes little sense. From @njsmith's first paragraph:

Setting NPY_NO_DEPRECRATED_API to 7 means "I have checked my code against numpy 1.7, 
and it doesn't use any APIs that were deprecated in 1.7".

This is how the code currently acts, but it's very counterintuitive. Setting NPY_NO_DEPRECRATED_API to 7 should mean " I want to check that my code doesn't include any APIs that were deprecated in 1.7, so don't include them".

There should indeed be some changes made for grouping APIs by version like @njsmith proposes.

@rgommers
NumPy member
rgommers commented Mar 2, 2013

As for the warning message, instead of adding yet another step to do by hand maybe there should be one script that does string replacement on the relevant source files which the release manager runs right after branching a maintenance branch. It could increment the version number, add new release notes, include those in the docs, fix this warning message, etc.

Or maybe in this case the proposed fix works, can't tell without trying.

@njsmith
NumPy member
njsmith commented Mar 2, 2013

@rgommers: "I want to check that my code doesn't include any APIs that were deprecated in 1.7, so don't include them" -- this is equivalent to what I said. In both cases the thing to do when compiling against 1.7 is to leave out all the APIs that 1.7 considers deprecated, but when compiling against 1.8 to leave in the APIs that 1.8 considers deprecated.

@rgommers
NumPy member
rgommers commented Mar 2, 2013

OK then I misunderstood what you were saying. It doesn't seem to work that way at the moment then, since your suggested multi-version handling (#ifndef NPY_NO_DEPRECATED_API ... block) isn't present yet.

@njsmith
NumPy member
njsmith commented Mar 6, 2013

Added 1.8 milestone: we need to do something like my proposed change before we release 1.8. If we do nothing, then 1.8 will start claiming that everything that was deprecated in 1.7 was deprecated again in 1.8, and needs to be checked for again...

@rgommers
NumPy member
rgommers commented Mar 6, 2013

Agree that this is important to get right for 1.8

We're apparently still not agreeing on what NPY_NO_DEPRECATED_API should do. I think it should exclude deprecated APIs for the release indicated and the ones before it. To adapt your example:

#if NPY_NO_DEPRECATED_API == NPY_1_8_API_VERSION
/* Exclude APIs that were deprecated in numpy 1.8 or before. */
#endif

#if NPY_NO_DEPRECATED_API == NPY_1_7_API_VERSION
/* 
 * Exclude APIs that were deprecated in numpy 1.7 or before.
 * Do not exclude APIs that were deprecated in 1.8.
*/
#endif 
@njsmith
NumPy member
njsmith commented Mar 6, 2013

It looks to me like your text ("exclude deprecated APIs for the release indicated and the ones before it") matches my example code? Note that what goes inside the #if's is the declarations for the deprecated API, i.e., the branch is taken if we are enabling a particular set of APIs, not excluding them.

#if NPY_NO_DEPRECATED_API < NPY_1_8_API_VERSION
void poorly_thought_out_function();
#endif

#if NPY_NO_DEPRECATED_API < NPY_1_7_API_VERSION
#define COMPLETELY_BROKEN_API_WHAT_WERE_WE_THINKING 31337
#endif
@rgommers
NumPy member
rgommers commented Mar 6, 2013

It does, but your example seems to contradict your text "everything that was deprecated in 1.7 was deprecated again in 1.8, and needs to be checked for again...". It looks like there you were trying to say that if I define NPY_NO_DEPRECATED_API to 1.8, then APIs deprecated in 1.7 should be included. I guess I misunderstood your text once again.

@njsmith
NumPy member
njsmith commented Mar 6, 2013

My text that you're quoting is describing what 1.8 will do if we leave the current code as is, and don't fix this. Right now we have a big block of stuff that was deprecated in 1.7, but in the source it's just marked as "deprecated in the current release", so when the current release changes things will get confused.

@charris
NumPy member
charris commented Mar 8, 2013

This needs to come to a conclusion for numpy 1.7.1

@njsmith
NumPy member
@erg
erg commented Mar 8, 2013

Yes, using master I can't check that my codebase is 1.7-clean.

I'm not sure I follow the conversation fully, but why are the 1.7-deprecated functions still present in 1.8? At which point do you remove them?

@njsmith
NumPy member
@erg
erg commented Mar 8, 2013

I propose leaving the deprecated 1.7 code in til 1.9 (is that the next big release?), and designing it so you can define NPY_1_7_API_VERSION to check for 1.7-deprecation-clean, NPY_1_8_API_VERSION to be 1.8-deprecation-clean, etc.

The check should make sure your numpy version is at least the version you are checking-clean against (don't want to be able to check for 1.8-deprecation-clean if we are only using numpy 1.7), and a deprecation-clean-check against 1.8 would also check against 1.7, 1.6, etc.

@njsmith
NumPy member
@erg
erg commented Mar 8, 2013

Good point. So check for deprecation-clean vs min(NPY_API_VERSION, DEPRECATION_CLEAN_VERSION_NUMBER).

Of course if they use features from 1.7 and your numpy is only 1.6, compilation will fail anyway...

@charris
NumPy member
charris commented Mar 9, 2013

The intent was that a developer could mark their code so that it would compile against, say version 6 and up, so then NPY_NO_DEPRECATED_API would be defined to 6, and if the code was compiled against 1.7 the functions/macros deprecated in 1.7 would be included for backwards compatibility. Replacements for the deprecated functions/macros are not generally available before they are deprecated, so developers can't use them when compiling against earlier versions of numpy if they want to support them.

With that understanding, I think the current code is correct. But I may be wrong...

@dmuellner

The header files in their current state not only produce a warning message but abort the compilation with an error. Hence, any C/C++ module which defines NPY_NO_DEPRECATED_API will not work with the current NumPy 1.8. Let's understand this step by step:

Say, a C source file contains

#define NPY_NO_DEPRECATED_API NPY_1_7_API_VERSION
#include <numpy/arrayobject.h>

This is perfectly valid and the recommended syntax for using the NumPY 1.7 C-API. The arrayobject header causes ndarraytypes.h to be loaded, which contains the lines

#if !(defined(NPY_NO_DEPRECATED_API) && (NPY_API_VERSION <= NPY_NO_DEPRECATED_API))
#include "npy_deprecated_api.h"
#endif

near the end. And npy_deprecated_api.h contains

#if defined(NPY_NO_DEPRECATED_API)
#error Should never include npy_deprecated_api directly.
#endif

which causes the compilation to abort.

Bottom line: this definitely needs to be fixed in NumPy 1.8. I'll submit a pull request with a fix soon.

@seberg
NumPy member
seberg commented Aug 5, 2013

@charris, this is fixed for 1.8. and can be closed now, right?

@charris
NumPy member
charris commented Aug 5, 2013

@seberg Looks like it, those lines are gone. Instead the include is surrounded by

#define NPY_DEPRECATED_INCLUDES
....
#undef NPY_DEPRECATED_INCLUDES

which is checked in the include files to make sure the include takes place in the right spot.

@charris charris closed this Aug 5, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.