-
Notifications
You must be signed in to change notification settings - Fork 10
ENH/API: xp-bound namespaces, array-api-compat #6
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
Comments
Hi @lucascolley! Thanks for writing this up and making this library. I think it's really helpful for the ecosystem to have something like this, and also potentially a good place for staging things the standard may or may not want to adopt. I read through all the above and will add my two cents. To be honest, I think (3) is the way to go. This is based on we want to make this as easy to use as possible. I know it sounds silly, but I think adding in extra functions like On the issue of Python scalars and lists and that kind of thing... I think there needs to be a solution to this, though not sure if this is in the scope of |
I would suggest depending on array_api_compat, but always use it as It might take a little thinking on how to handle all the different vendoring/depending combinations, but I think it's doable, and will be much simpler in the long run as you're ultimately going to want to use a lot of stuff in compat (not just array_namespace). |
I think I agree with depending on I'd expect that even functions with a universal implementation in terms of the array API primitives only are going to need library-specific direct calls for performance reasons at some point once usage of this package takes off. |
If I understand correctly:
from array_api_compat import *
# This file is copied to ../array_api_extra/ by meson.build, replacing the
# external dependency on array_api_compat with the one vendored by scipy
from ..array_api_compat import *
py3.install_sources(
[
'array_api_extra/src/array_api_extra/__init__.py',
'array_api_extra/src/array_api_extra/_funcs.py',
'array_api_extra/src/array_api_extra/_typing.py',
'array_api_compat_vendor/compat.py', # new line
],
subdir: 'scipy/_lib/array_api_extra',
) |
That doesn't look right to me, there should be no need to mess with Meson. Step 1 is fine, step 2 can conceptually be: # Allow packages that vendor `array-api-extra` as well as `array-api-compat` to override the import location
if os.path.exists('../../array_api_compat') #FIXME make this robust with __file__ etc.
# array-api-compat should be vendored right next to array-api-extra
importlib.from_spec # TODO the 3-line importlib thingy to import directly from a relative location
else:
import array_api_compat as comp (sorry, no time to work it out, but it can be made to work with a few lines of code) EDIT: back now, that was even overcomplicating things I think. If we have a # Allow packages that vendor `array-api-extra` as well as `array-api-compat` to override the import location
try:
# array-api-compat should be vendored right next to array-api-extra
from .. import _array_api_compat as compat
except ImportError:
# it's an external dependency
import array_api_compat as comp |
Fixed the example now. The main point is that this does not require any build system changes inside |
What about the issue of wrapped |
I think I agree with @izaid's take on |
The issue I'm referring to is that in SciPy, from scipy._lib._array_api import array_namespace, array_api_extra as xpx
...
xp = array_namespace(x)
xpx.cov(x, xp=xp) # uses `xp` from the wrapped `array_namespace`
...
xpx.cov(y) # uses unwrapped `array_namespace` This seems like a potential footgun to me as one might expect the |
Ah okay. Yes, it seems necessary to allow # Allow packages that vendor `array-api-extra` as well as `array-api-compat` to override the import location
try:
# allow vendoring array-api-compat and/or customizing array_namespace
from .. import _array_api_extra_vendor import array_namespace, array_api_compat as compat
except ImportError:
# it's an external dependency with no customization of array_namespace
import array_api_compat as compat
array_namespace = compat.array_namespace |
Would it be a bit cleaner as follows? > try:
> from .._array_api_compat_vendor import *
> except ImportError:
> from array_api_compat import *
from .array_api_compat import *
_array_namespace_orig = array_namespace
def array_namespace(...):
# scipy-specific override |
Currently, functions of this package require passing a standard-compatible namespace as
xp=xp
. This works fine, but there have been suggestions that it might be nice to avoid this requirement. There are at least a few ways we could go about this:(1)
xpx.bind_namespace
Usage:
A potential implementation:
I like this idea. If we encounter use cases where a library wants to use multiple
xpx
functions in the same local scope and finds thexp=xp
pattern too cumbersome, I think we should add this. I think we can leave it out for now until that situation arises.(2)
xpx.extra_namespace
Usage:
A potential implementation:
I would not want to add this yet. I think we should keep separation between the standard namespace and the 'extra' namespace, at least until this library matures.
(3) Use
array_api_compat.array_namespace
internallyThis would provide the most flexible API and be the least LOC to use. One could use
xpx
functions on standard-incompatible arrays, and let array-api-compat handle the compatibility, without having to pass anxp
argument.We don't yet have a use case where it is clearly beneficial to be able to pass standard-incompatible arrays. Consumer libraries using array-api-extra would already be computing with standard-compatible arrays internally. I don't see the need to support the following use case:
Another complication is that consumer libraries like SciPy wrap
array_namespace
to provide custom behaviour for scalars and other types. We would want the internalarray_namespace
to be the consumer library's wrapped version rather than the base one from array-api-compat.I'm also not sure that the 1 LOC save over option (1) of this post for standard-compatible arrays is worth introducing a dependency on array-api-compat.
Overall, this would complicate things a lot with situations of co-vendoring array-api-compat and array-api-extra, which is the primary use-case for the library right now. This might be a better idea in the future if a need for handling standard-incompatible arrays arises (for example, if one wants to use functions from
xpx
with just a single library).The text was updated successfully, but these errors were encountered: