Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

DOC: update NEP 31 for vendoring and being complementary to NEP-13/18 #3

Merged
merged 2 commits into from Aug 24, 2019

Conversation

hameerabbasi
Copy link
Owner

cc @rgommers, a nicer interface to discuss the NEP before it's submitted. 馃槃

@hameerabbasi
Copy link
Owner Author

I don't know why the first two switched to having a base of master, I explicitly selected uarray.

Copy link
Owner Author

@hameerabbasi hameerabbasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @peterbell10 would you like to be co-author? Also, any suggestions here?

@@ -16,19 +16,26 @@ This NEP proposes to make all of NumPy's public API overridable via a backend
mechanism, using a library called ``uarray`` `[1]`_

``uarray`` provides global and context-local overrides, as well as a dispatch
mechanism similar to NEP-18 `[2]`_. This NEP proposes to supercede NEP-18,
and is intended as a comprehensive resolution to NEP-22 `[3]`_.
mechanism similar to NEP-18 `[2]`_. First experiences with ``__array_function`` show that it is
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
mechanism similar to NEP-18 `[2]`_. First experiences with ``__array_function`` show that it is
mechanism similar to NEP-18 `[2]`_. First experiences with ``__array_function__`` show that it is

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep typo

@@ -61,7 +71,8 @@ called ``unumpy`` `[8]`_, the implementation of which is already underway. Again
will not be explained here, the reader should refer to its documentation for this purpose.

The only change this NEP proposes at its acceptance, is to make ``unumpy`` the officially recommended
way to override NumPy. ``unumpy`` will remain a separate repository/package, and will be developed
way to override NumPy. ``unumpy`` will remain a separate repository/package (which we propose to vendor
rather than depend on for the time being), and will be developed
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about a soft dependency, like in scipy.fft?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will probably be fine, just thinking about if we should put that in here. The key thing is vendoring. Whether to optionally override the vendored version with an installed version to be able to ship bug fixes quicker is a small detail.

Note also that it's not a normal soft dependency - that typically means that not installed == no functionality.

I'd actually leave it out, since it's not important at this point.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note also that it's not a normal soft dependency - that typically means that not installed == no functionality.

Is there an accepted term for this? I was just looking for a word between hard and optional.

Either way, the "optionally overriding" behaviour is very important in order for the vendored version to play nicely with other libraries. For example, if I have a decorator @implements(unumpy.sum) such as in NEP 18, then my backend won't work at all if my unumpy.sum is not the same as the user's unumpy.sum. This is exactly the problem that we get through normal vendoring.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an accepted term for this?

Not that I know of - I think it requires a one sentence description.

then my backend won't work at all if my unumpy.sum is not the same as the user's unumpy.sum

that's a good point. isn't an issue for scipy.fft (right?) but is tricky here. I think the soft dependency solves the issue, as long as other libraries don't do the same. If they either import from the numpy vendored package, or from an installed unumpy directly that should both work.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't an issue for scipy.fft (right?)

Yes, this is only an issue for numpy because unumpy is the package defining the multimethods. In scipy.fft we own the multimethods ourself so these cannot be mismatched, even if the uarray libraries were.

I think the soft dependency solves the issue

Completely agree, I just wanted to point out that shipping bug fixes faster is not the only motivation.

Copy link
Collaborator

@peterbell10 peterbell10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious what the motivation is for numpy to vendor unumpy? I personally don't think it's explained here.

I also think that it would be good to mention overrides for numpy arrays such as pyFFTW and another one I've come across was opt_einsum. It's definitely a good example of something that __array_function__ just cannot support.


Motivation and Scope
--------------------

The motivation behind this library is manifold: First, there have been several attempts to allow
The motivation behind ``uarray`` is manifold: First, there have been several attempts to allow
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The motivation behind ``uarray`` is manifold: First, there have been several attempts to allow
The motivation behind ``uarray`` is manyfold: First, there have been several attempts to allow

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC PyFFTW just monkeypatches NumPy, so yes that's good to mention. And opt_einsum too, didn't realize it had its own backend system with the build_expression stuff. @saulshanabrook that may actually be of interest to you, the transformations seem nontrivial enough that metadsl could be a nice fit.

I'm curious what the motivation is for numpy to vendor unumpy? I personally don't think it's explained here.

We really dislike dependencies. NumPy doesn't have any at the moment. Proposing a hard dependency on a brand new and fast moving package is a showstopper in itself, and will definitely derail the discussion on the merits of the dispatching mechanism.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell it should be viable for unumpy to be a completely optional dependency for numpy. I have done the same thing (albeit on a smaller scale) for my cupyx FFT backend in cupy/cupy#2355.

If scipy.fft is not available, I use a mock version instead:

try:
    import scipy.fft as _scipy_fft
except ImportError:
    class _DummyModule:
        def __getattr__(self, name):
            return None

    _scipy_fft = _DummyModule()

Then I create the multimethod overrides using _scipy_fft. e.g.

@_implements(_scipy_fft.fft)
def fft(x, n=None, axis=-1, norm=None, overwrite_x=False):

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but then the feature isn't available:) That then propagates to Dask, CuPy, etc. - that doesn't really work well. We need the always-available without a hard dependency. So it's either move into NumPy wholesale, or the vendor + "soft dependency" option.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe "manifold" is what I meant: https://www.merriam-webster.com/dictionary/manifold

@rgommers
Copy link
Collaborator

Okay, I pushed some TODOs. The whole thing isn't consistent yet. We need the usage examples so it can be compared to NEP 30 - just reference the unumpy docs won't cut it.

I'm going back and forth on the vendoring. The problem is that if it's opt-in, the imports become messed up - you cannot do import ... as np anymore, because both numpy and unumpy want to be called np.

The key problem is: wholesale adoption is going to be a very hard sell, because it effectively gets rid of __array_function__ and __array_ufunc__. A partial adoption for things that those protocols don't support makes sense from a social acceptance point of view, but seems to run into the np problem I pointed out above.

@hameerabbasi
Copy link
Owner Author

hameerabbasi commented Aug 24, 2019

A partial adoption for things that those protocols don't support makes sense from a social acceptance point of view, but seems to run into the np problem I pointed out above.

If we set NumPy as the backend (via set_global_backend) for unumpy, those overrides will be preserved.

@hameerabbasi
Copy link
Owner Author

Merging since I don't have commit rights to this PR... And I wanted to push some changes. Thanks @rgommers and @peterbell10!

@hameerabbasi hameerabbasi merged commit b637504 into hameerabbasi:uarray Aug 24, 2019
* The upgrade pathway to NumPy 2.0 becomes simpler, requiring just
a backend change, and allowing both to exist side-by-side.

**FIXME: this section doesn't match the proposal. in the abstract and motivation anymore.**
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rgommers Can you expand on this a little bit?

obviates the need to add an ever-growing list of new protocols for each new type of function or object that needs to become overridable.

It assumes that there are parts of the API that need to be overridable, and that these will grow over time. It provides a general framework and a mechanism to overridable, and that these will grow over time.

So it's consistent, even though it explains other use-cases. Would you like for this section to be removed? Personally, I'd at least like to mention the possibility of having such a system, and for unumpy as an alternative to other overrides.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I'd at least like to mention the possibility of having such a system, and for unumpy as an alternative to other overrides.

Yes, I think it's good to mention that.

Can you expand on this a little bit?

If the whole thing is opt-in and kept in a separate namespace, as in the abstract and motivation sections, then a lot of this reads strangely. The examples don't work as np.<funcname>, because that assumes you've already completely taken over the whole namespace. It's clearer when you add the full example, so I'll comment on your new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants