BUG: Function signature mismatch: random_multinomial

@hoodmane

Describe the issue:

When I try to statically link NumPy to Pyodide, it works fine, but I am getting a warning about two conflicting definitions for random_multinomial.

Error message:

wasm-ld: warning: function signature mismatch: random_multinomial
>>> defined as (i32, i64, i32, i32, i32, i32) -> void in dist/libnumpy.a(_generator.o)
>>> defined as (i32, i32, i32, i32, i32, i32) -> void in dist/libnumpy.a(distributions.o)

Context for the issue:

@hoodmane did some investigation on this in pyodide/pyodide#4083 and concluded that:

_generator.o imports random_multinomial with type (i32, i64, i32, i32, i32, i32) -> nil and distributions.o defines it with type (i32, i32, i32, i32, i32, i32) -> nil. It seems to be that the type of RAND_INT_TYPE is controlled by the environment variable NP_RANDOM_LEGACY. If NP_RANDOM_LEGACY is defined to be 1, then it's defined to be long which is probably 32 bits on wasm32. Otherwise, it's defined to be int64. But numpy/random/c_distributions.pxd unconditionally declares it as int64. This is probably best regarded as a bug in numpy.

@charris

There's also serge-sans-paille/pythran#2139 which is due to the signature changes for np.random functions. Should we remove those #cython: binding=True changes from 1.26.0 @charris @bashtage? That release is supposed to be only build system changes, and these Cython changes seem to have side effects.

ISTR seeing related errors indicating that those were needed for cython 3. I'm not opposed to fixups as long as the tests still pass, so we could try removing them and see what happens.

I think reverting that parch from the 1.25 and subsequent 1.x branches that won't use Cython 3 was the conclusion of the other thread #24429.

@bashtage

@bashtage Anyway to make them cython version dependent? I'd like to keep Cython 3 support.

Maybe I am missing something, but removing #cython: binding=True changes does not affect Cython 3 support, right?

BUG: Change Cython binding directive to "False".

@bashtage

Turns out that the binding directive has been around for a while, but in Cython 2 it defaults to "False", while in Cython 3 it defaults to "True".

binding (True / False)
Controls whether free functions behave more like Python’s CFunctions (e.g. len()) or, when set to True, more like Python’s functions. When enabled, functions will bind to an instance when looked up as a class attribute (hence the name) and will emulate the attributes of Python functions, including introspections like argument names and annotations.

Default is True.

Changed in version 3.0.0: Default changed from False to True

I think the problem here is that distributions.c is a C file. This problem will also affect the main branch. For NumPy 1.26.x I'll just set binding to "False". I'm not sure what we want to do for 2.0.0, we may want to change the signatures of the C functions. @bashtage Thoughts?

See #24606.

But numpy/random/c_distributions.pxd unconditionally declares it as int64

Yep, that looks like a bug. Note that both meson and setup build define NP_RANDOM_LEGACY=1 by default, hence RAND_INT_TYPE should be defined as long. I note that c_distributions.pxd doesn't mention RAND_INT_TYPE anywhere, but that is used in the C code.

This was done to allow us to share as much code as possible between RandomState and Generator. RandomsState is limited to long due to legacy constraints while Generator always uses int64. Could possibly write a simple wrapper so that the visible function names would always have the same type.

I don't see why this issue would be related to the binding issue.

As an alternative, we could just change the function name at compile time so that the function names used in RandomState and Generator do not conflict.

Isn't this fixed with gh-24606 merged?

@bashtage

Sorry, I agree with @bashtage this is unrelated to binding, if there is a change in behavior, I suspect it must be related to meson somehow. The distributions.o should use the int64 version publically, I think.
But, while building mtrand -DNP_RANDOM_LEGACY=1 makes sure that we use long.

It isn't clear to me why this changed, maybe distributions.o was previously generated twice (overwritten) and now meson stopped that!?

As an alternative, we could just change the function name at compile time

@bashtage would you be willing to look into that? Unless it leads to massive duplication at least that seems quite reasonable on first thought.

It isn't clear to me why this changed, maybe distributions.o was previously generated twice (overwritten) and now meson stopped that!?

It's still built twice it looks like:

$ ls build/numpy/random/libnpyrandom.a.p/src_distributions_distr*
build/numpy/random/libnpyrandom.a.p/src_distributions_distributions.c.o

$ ls build/numpy/random/mtrand.cpython-39-x86_64-linux-gnu.so.p/src_distri*
build/numpy/random/mtrand.cpython-39-x86_64-linux-gnu.so.p/src_distributions_distributions.c.o

and the NP_RANDOM_LEGACY flag usage matches setup.py.

We haven't ported over the Emscripten CI job though, so we haven't yet been able to see if something may be wrong for Pyodide.

@rgommers

@rgommers, I suspect the mistake is including mtrand in random_pyx_sources. That means this is run:

foreach gen: random_pyx_sources
  py.extension_module(gen[0],
  ...
  link_with: [npyrandom_lib, gen[3]],

but the old code doesn't link mtrand with npyrandom_lib. So the compile step has its own copy (as it needs), but then we do more linking than we need/should.

@seberg

Ah, great catch @seberg!

I suspect I just misread this when translating it, due to the confusing variable name:

    mtrand_libs = ['m', 'npymath'] if os.name != 'nt' else ['npymath']
    config.add_extension('mtrand',
                         ...
                         libraries=mtrand_libs,

And also, libnpyrandom is linked into every other extension module in the setup.py build through the libraries=EXTRA_LIBRARIES line.

The link_with line in meson.build should be changed to link in libnpymath instead of libnpyrandom for a faithful translation.

The build config seems slightly inefficient, I'm wondering if we could gain something there. The extension modules in numpy.random are linking in libnpyrandom and/or libnpymath in every separate extension module. This makes them larger than they have to be. Right now in the Meson build (with -Dbuildtype=release):

$ ls -lh build/numpy/random/*.so  | awk '{print $5,$9}'
225K build/numpy/random/bit_generator.cpython-39-x86_64-linux-gnu.so
373K build/numpy/random/_bounded_integers.cpython-39-x86_64-linux-gnu.so
272K build/numpy/random/_common.cpython-39-x86_64-linux-gnu.so
989K build/numpy/random/_generator.cpython-39-x86_64-linux-gnu.so
127K build/numpy/random/_mt19937.cpython-39-x86_64-linux-gnu.so
802K build/numpy/random/mtrand.cpython-39-x86_64-linux-gnu.so
134K build/numpy/random/_pcg64.cpython-39-x86_64-linux-gnu.so
117K build/numpy/random/_philox.cpython-39-x86_64-linux-gnu.so
78K build/numpy/random/_sfc64.cpython-39-x86_64-linux-gnu.so

And in the setup.py build:

$ ls -lh numpy/random/*.so  | awk '{print $5,$9}'
221K numpy/random/bit_generator.cpython-39-x86_64-linux-gnu.so
369K numpy/random/_bounded_integers.cpython-39-x86_64-linux-gnu.so
272K numpy/random/_common.cpython-39-x86_64-linux-gnu.so
985K numpy/random/_generator.cpython-39-x86_64-linux-gnu.so
127K numpy/random/_mt19937.cpython-39-x86_64-linux-gnu.so
821K numpy/random/mtrand.cpython-39-x86_64-linux-gnu.so
134K numpy/random/_pcg64.cpython-39-x86_64-linux-gnu.so
117K numpy/random/_philox.cpython-39-x86_64-linux-gnu.so
78K numpy/random/_sfc64.cpython-39-x86_64-linux-gnu.so

That's something for later though.

Do you want to open a PR with the fix, or should I?

Not well versed with meson best practices, so please just go ahead.

BLD: fix bug in random.mtrand extension, don't link libnpyrandom

BUG: Change Cython binding directive to "False".

BLD: fix bug in random.mtrand extension, don't link libnpyrandom

Navigation Menu

Repository navigation

BUG: Function signature mismatch: random_multinomial #24490

Describe the issue:

Error message:

Context for the issue:

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

BUG: Function signature mismatch: random_multinomial #24490

Description

Describe the issue:

Error message:

Context for the issue:

Activity

rgommers commented on Aug 24, 2023

charris commented on Aug 24, 2023

bashtage commented on Aug 24, 2023

charris commented on Aug 24, 2023

rgommers commented on Aug 25, 2023

charris commented on Aug 31, 2023

charris commented on Aug 31, 2023

bashtage commented on Sep 1, 2023

bashtage commented on Sep 1, 2023

seberg commented on Sep 6, 2023

seberg commented on Sep 6, 2023

rgommers commented on Sep 6, 2023

seberg commented on Sep 7, 2023

rgommers commented on Sep 7, 2023

seberg commented on Sep 7, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions