[python] _swigconstant helpers needlessly generated for primitive types

[yarikoptic]

Please pardon my ignorance in swig since I haven't actively used it in the past years. While preparing new release of PyMVPA I have ran into failed builds due to the fact that constants we had defined in svmc.i interface (https://github.com/PyMVPA/PyMVPA/blob/master/mvpa2/clfs/libsvmc/svmc.i#L142)

enum { C_SVC, NU_SVC, ONE_CLASS, EPSILON_SVR, NU_SVR }; /* svm_type */
enum { LINEAR, POLY, RBF, SIGMOID, PRECOMPUTED };   /* kernel_type */

used to build Python bindings acquired _swigconstant suffixes all of a sudden. with 3.0.4 it is fine, with 3.0.5 we get already _swigregister (introduced in rel-3.0.3-5-gc21e242) which then later got renamed in rel-3.0.3-6-gcfaf2f9 into _swigconstant.

I guess it all was intentional and for common good, but I thought to double-check and may be seek some guru advice if there is possibly a cmdline (or other) option to revert to original behavior, possibly making it not too complicated to support multiple versions of swig across various platforms. Thank you in advance!

[yarikoptic]

FWIW for now I have worked around by not using swig3.0 and relying on swig2.0 in Debian so the problem is not that "pressing"

[ojwb]

Can you provide a small, self-contained example and explain exactly what the problem with the code generated by the latest version is? Please also include the SWIG command line to process the file.

[yarikoptic]

here is not quite a minimal but quite verbatim and complete as working for me on a Debian testing/sid:

git clone https://github.com/PyMVPA/PyMVPA  ; cd PyMVPA

swig -python -I/usr/lib/python2.7/dist-packages/numpy/core/include -I3rd/libsvm -c++ -I/usr/lib/python2.7/dist-packages/numpy/core/include -I3rd/libsvm -o mvpa2/clfs/libsvmc/svmc_wrap.cpp -outdir mvpa2/clfs/libsvmc mvpa2/clfs/libsvmc/svmc.i

g++ -I/usr/lib/python2.7/dist-packages/numpy/core/include -I3rd/libsvm -I/usr/lib/python2.7/dist-packages/numpy/core/include -I/usr/include/python2.7 -c mvpa2/clfs/libsvmc/svmc_wrap.cpp -fPIC

g++ -pthread -DNDEBUG -g -fwrapv -O2 -Wall -fno-strict-aliasing -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC ./3rd/libsvm/svm.cpp -c

c++ -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-z,relro -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security svm.o svmc_wrap.o -o mvpa2/clfs/libsvmc/_svmc.so

# just for demonstration
PYTHONPATH=$PWD/mvpa2/clfs/libsvmc python -c 'import _svmc; print dir(_svmc)'

[yarikoptic]

any other information I could provide to point toward proper resolution?

[wsfulton]

Yes, I don't understand what the actual problem is, can you elaborate what is broken or not working etc.

[ojwb]

Actually, having just looked at updating Xapian's Python bindings to use the latest SWIG, I think I now have some understanding of this issue.

In 3.0.5, there was a change to make %constant work for non-primitive types:

2015-01-15: wsfulton
            [Python] Merge patch #250 - Fixes for using %constant and objects (non-primitive types)

And the _swigconstant stuff is what provides this support.

However, the conditions under which the new machinery gets used seem to be too broad, and constants of primitive types get it - as far as I can see needlessly, since such constants worked fine before this change. This adds load-time overhead and bloats the wrappers. The additional code and the pile of extra symbols probably mean run-time overhead too.

@ptomulik It looks like this was mostly your patch. This is the gating conditional in current git master - any idea how we can tweak this to only fire for the cases that need it?

      if (!builtin && (shadow) && (!(shadow & PYSHADOW_MEMBER)) && (!in_class || !Getattr(n, "feature:python:callback"))) {
        // Generate method which registers the new constant

[yarikoptic]

I would appreciate more discussion/resolution on this issue. @ptomulik ? Thanks in advance

[ojwb]

From discussion on #debian-devel, the issue yoh is seeing is that the constants themselves now seem to have the suffix _swigconstant, and don't appear to be available without it.

[ojwb]

OK, so that aspect is resolved - the problem was due to using import _svmc rather than import svmc, and it's the Python wrapper code which now sets up the constants.

That still leaves us with the pessimisation of the needless helpers for primitive types, which would be good to fix. @ptomulik Did you see my question 3 comments above?

[yarikoptic]

the only complication is that if previous code was using all the interface constructs, I would need to convert to "proper" Pythonish style, e.g.

-        svmc.svm_problem_l_set(prob, size)
-        svmc.svm_problem_y_set(prob, y_array)
-        svmc.svm_problem_x_set(prob, x_matrix)
+        prob.l = size
+        prob.y = y_array
+        prob.x = x_matrix

and now getting some error on what I don't think I am setting anywhere... so more to be done later. And the question remains either lazy me could have just maintained the use of the same old _svmc interface ? ;)

[ptomulik]

Hi @yarikoptic, have you tried -builtin? The patch provided by #250 does not alter code generated with -builtin option which, I think, is recommended these days.

[yarikoptic]

@ptomulik thanks for the idea... but so far I seem finding peace with RFing to just use the svmc import: yarikoptic/PyMVPA@b63b9d5 . Yet to give it more testing to see if nothing is left behind

[ojwb]

@ptomulik While -builtin may provide a workaround (at least if its drawbacks aren't problematic for someone's use case), your change has introduced a regression when running SWIG with the default options.

But I don't understand the if condition in question, so it's hard for me to address short of reverting your whole change, which doesn't seem a helpful way to proceed.

Could you at least explain what that condition is trying to check for?

[ptomulik]

@ojwb, if you take a look at the patch https://github.com/swig/swig/pull/250/files, you'll see the original (pre-patch) logics for generating stuff into shadow file was like this:

if (!builtin && (shadow) && (!(shadow & PYSHADOW_MEMBER))) {
  if (!in_class) {
    Printv(f_shadow, iname, " = ", module, ".", iname, "\n", NIL);
  } else {
    if (!(Getattr(n, "feature:python:callback"))) {
      Printv(f_shadow_stubs, iname, " = ", module, ".", iname, "\n", NIL);
    }
  }
}

I used the above code as a starting point for conditional generation of the *_swigconstant boilerplate. So the first step was to extend it to this:

if (!builtin && (shadow) && (!(shadow & PYSHADOW_MEMBER))) {
  if (!in_class) {
    Printv(f_shadow, "\n",NIL);
    Printv(f_shadow, module, ".", iname, "_swigconstant(",module,")\n", NIL);
    Printv(f_shadow, iname, " = ", module, ".", iname, "\n", NIL);
  } else {
    if (!(Getattr(n, "feature:python:callback"))) {
      Printv(f_shadow_stubs, "\n",NIL);
      Printv(f_shadow_stubs, module, ".", iname, "_swigconstant(", module, ")\n", NIL);
      Printv(f_shadow_stubs, iname, " = ", module, ".", iname, "\n", NIL);
  }
}

A second step was to conditionally generate the actual implementation of *_swigconstant functions. I've derived from the above if-else construct the "squeezed" logical condition being discussed:

if (!builtin && (shadow) && (!(shadow & PYSHADOW_MEMBER)) && (!in_class || !Getattr(n, "feature:python:callback"))) {
  // Generate method which registers the new constant
  // ...
}

I must admit, I really have no idea why the original logics was as such, except the !builtin && (shadow) part which is quite obvious.

I think it would be better, if we could also disable the boilerplate for primitive types, but I have no idea how to distinguish them from non-primitives.

[ptomulik]

And, my justification for *_swigconstant -- to have the control over the point where the constants are spawned. The default implementation spawns them too early -- before non-primitive types get registered. By default, they're spawned in SWIG_Init(void) function, which is invoked before shadow class is registered from within the shadow file.

[ptomulik]

Please see PR #631.

[wsfulton]

Fixed in #631.