GDP-3: Duck Storages #28

gronerl · 2020-05-26T12:22:59Z

No description provided.

docs/GDPs/gdp-duck-storage.rst

docs/GDPs/gdp-0003-duck-storage.rst

gronerl · 2020-06-26T15:07:38Z

I addressed some of your comments @mcgibbon . Regarding the domain, I think there should be a discussion on the terminology and semantics, also including what you mentioned regarding cell vs edge values, but also for the stencil interface etc, not only the storage.

…s, halo parameter and halo/domain attributes

Co-authored-by: Linus Groner <linus.groner@cscs.ch> Co-authored-by: Enrique G. Paredes <enriqueg@cscs.ch>

…rce.

Co-authored-by: Jeremy McGibbon <jeremy.mcgibbon@gmail.com>

…evious implementation with `dims` as alternative.

… of ufuncs

mcgibbon

Looks good! My comments are mostly stylistic, or asking for clarification on some options. The code ideas are all solid.

mcgibbon · 2020-10-19T20:44:28Z

docs/GDPs/gdp-0003-duck-storage.rst

+Finally, without internally keeping information about the semantic meaning of dimensions, e.g. the
+best layout and proper broadcasting for the resulting storage can not be determined. Further,
+implementations would depend on the availability of a library implementing the operations for a
+given device. We have already observed performance problems when using cupy on AMD hardware. For
+future hardware, these libraries might be entirely unavailable. Therefore, we will not commit to
+supporting such operations.


I agree this appears to be necessary. This in particular should be communicated to users.

mcgibbon · 2020-10-19T20:45:42Z

docs/GDPs/gdp-0003-duck-storage.rst

+The implementation of this GDP breaks the integration with external libraries which require a NumPy
+`ndarray` subclass. Further, we propose some API changes like renaming or repurposing of keyword


Could you add here a short description of how to migrate such codes? e.g. indicate what attribute name is guaranteed to be an ndarray subclass which provides access to the storage memory?

mcgibbon · 2020-10-19T20:46:20Z

docs/GDPs/gdp-0003-duck-storage.rst

+
+
+Detailed Description


Suggested change

Detailed Description

Detailed Description

mcgibbon · 2020-10-19T20:48:14Z

docs/GDPs/gdp-0003-duck-storage.rst

+meaning is the same as in the NumPy :code:`__array_interface__` and the
+:code:`__cuda_array_interface__`:


A hyperlink to numpy/cupy documentation would be helpful here.

mcgibbon · 2020-10-19T20:48:23Z

docs/GDPs/gdp-0003-duck-storage.rst

+ :code:`"data": Tuple[int, bool]`
+ :code:`"strides": Tuple[int, ...]`
+
+In Addition, the following optional keys can be contained:


Suggested change

In Addition, the following optional keys can be contained:

In addition, the following optional keys can be contained:

mcgibbon · 2020-10-19T22:03:09Z

docs/GDPs/gdp-0003-duck-storage.rst

+    the respective dimension, which can be used to denote asymmetric halos. It defaults to no halo,
+    i.e. :code:`(0, 0, 0)`. (See also Section :ref:`domain_and_halo`)
+
+:code:`layout: Optional[Sequence[int]]`


nit: layout is a slightly overloaded term, we're using it already to indicate the number of ranks along edges of a tile face. Could this be named stride_order or something similar?

When using as_storage, is this derived by default from the array passed as input? If so, can you describe that here? Also, what is the interplay between this and the dims argument, if only one is given?

mcgibbon · 2020-10-19T22:13:51Z

docs/GDPs/gdp-0003-duck-storage.rst

+    The rationale behind this is that in this way, storages allocated with :code:`defaults` set to a
+    backend will always get optimal performance, while :code:`defaults` set to :code:`"F"` or


This sounds like it disagrees somewhat with the last paragraph of the introduction.

mcgibbon · 2020-10-19T22:14:38Z

docs/GDPs/gdp-0003-duck-storage.rst

+:code:`managed: Optional[str]`
+    :code:`None`, :code:`"gt4py"` or :code:`"cuda"`. It only has effect if :code:`device="gpu"` and
+    it specifies whether the synchronization between the host and device buffers is not done
+    (:code:`None`), GT4Py (:code:`"gt4py"`) or CUDA (:code:`"cuda"`). It defaults to :code:`"gt4py"`


What's the difference in behavior between None and cuda? It could be worth having a short description of what GT4Py and CUDA management are.

mcgibbon · 2020-10-19T22:15:26Z

docs/GDPs/gdp-0003-duck-storage.rst

+The values of parameters which are not explicitly defined by the user will be inferred from the
+first alternative source where the parameter is defined in the following search order:
+
+1. The provided :code:`defaults` parameter set.
+2. The provided :code:`data` or :code:`device_data` parameters.
+3. A fallback default value specified above. The only case where this is not available is
+   :code:`shape`, in which case an exception is raised.


This part about inferring parameters should probably be written at the start of this section. Which parameters can be derived from data/device_data, and how are they derived?

mcgibbon · 2020-10-19T22:22:46Z

docs/GDPs/gdp-0003-duck-storage.rst

+    Internally holds a reference to a `CuPy <https://cupy.chainer.org/>`_ `ndarray`. This storage
+    does not have a CPU buffer.
+
+:code:`SoftwareManagedGPUStorage`


nit: The previous name of ExplicitlyManaged was good for this object, I think. "Software" reads like an external binary running on the host system outside of Python. It contrasts well with CUDAManagedGPUStorage.

Accessing `.storage` on a quantity which was initialized with a numpy or cupy array and a gt4py_backend value currently causes `view` to become out of sync with the underlying data of the quantity, since the data is re-allocated in creating the storage but the view is unchanged and retains a reference to the old data. This PR updates the routine which re-allocates the data to also update the view. This is a work-around to deal with not being able to initialize gt4py storages from existing numpy or cupy arrays. We should be able to remove the work-around after GDP-3 (GridTools/gt4py#28) is merged.

mcgibbon · 2020-12-09T17:29:54Z

docs/GDPs/gdp-0003-duck-storage.rst

+ :code:`"acquire": Optional[Callable[[], Any]]` Is called on all objects that are passed to a
+  stencil, before running computations. It can be used to trigger a copy to the respective device.


I just realized I got mixed up to think that acquire was an argument passed to stencil, not one passed to storage. I would suggest this re-wording to make it clearer:

Suggested change

+ :code:`"acquire": Optional[Callable[[], Any]]` Is called on all objects that are passed to a

stencil, before running computations. It can be used to trigger a copy to the respective device.

+ :code: The `"acquire": Optional[Callable[[], Any]]` method of each object passed to a

stencil is called before running computations. It can be used to trigger a copy to the respective device.

Knowing that, does the acquire function need any information about the device the stencil is about to be run on?

havogt · 2021-09-28T15:41:34Z

@gronerl or @egparedes can you summarize what led to decision to close it, then we can merge it as declined.

gronerl · 2021-11-03T16:08:39Z

After this GDP and its reference implementation have not gained any traction for roughly a year, and after more discussions offline, we propose to decline this GDP.
The main points why this GDP is not the right way for storages are as follows.

The original motivation for this GDP was that moving away from the design where storages are subclasses of np.ndarray. The GDP proposed to make this change so that one-sided changes to coupled host/device buffers managed by GT4Py storages can be more reliably tracked, since functionality is not handled by the parent class. This issue is indeed solved by the GDP. However, it just moves the problem to a new one: The changes that can now be tracked will often require the allocation of a new buffer, for which performance optimal allocation can not be guaranteed based on the information available.

Resolving this in any way leads to a storage that is either severely limited in functionality, which prompts users to escape to different frameworks anyways (as is the case with the old storages), or that performance may be severely impacted depending on the exact use of the storages, which is not transparent to the user. In other cases the behavior may be unintuitive (broadcasting, slicing). Overall it seems impossible to find a solution that is a good trade off for a substantial share of all use cases.

Instead, we currently propose to pursue a "No Storage" approach in the future. The idea there would be that gt4py provides utilities that allocate buffers which have desireable properties for stencils, yet the framework will not provide any methods on the buffers, i.e. it is up to the user to fill values in the allocated memory, implement ufuncs if desired etc.

However, a generic interface like the __gt_data_interface__ proposed in this GDP is still assumed to be necessary.

Please, raise objections to declining this GDP until Nov 10, 2021.

ofuhrer · 2021-11-04T08:14:59Z

@mcgibbon Thoughts?

mcgibbon · 2021-11-04T19:57:04Z

It feels like less is more when it comes to storages, and this seems to fit the bill. It sounds exciting to be able to provide gt_data_interface on our Quantity class some day and have that be treated directly as a Storage.

gronerl · 2021-11-11T08:08:50Z

I'm pasting some remarks by @jdahm on Slack to preserve them:


johann  8 days ago
I assume this would still be limited to arrays that implement the buffer protocol? We'd also still need a default_origin, but I suppose that could be in a thin wrapper on top of an array. (edited) 




Linus  8 days ago
Yeah, the intention would be to put that info in the __gt_data_interface__ ...i.e. your thin wrapper could expose it in that way. For your FrozenStencils it shouldn't be needed anyways though, right?

johann  8 days ago
I'm more in favor of keeping the default_origin, and using a cached stencil argument approach, but both should be possible.

johann  8 days ago
Correct, it is a hands-off approach where the origins are handled in the application instead of the storage.

Linus  8 days ago
Of course that idea isn't fully shaped yet, so the specifics of specifying origins can be discussed.

egparedes

Agree with the conclusions.

…onstruct_inside Remove code duplication and decrease complexity of PAST Lowering

mbianco reviewed Jun 12, 2020

View reviewed changes

gronerl force-pushed the gdp-duck-storage branch 2 times, most recently from 12209d8 to e0c2c5c Compare June 15, 2020 16:03

gronerl force-pushed the gdp-duck-storage branch from 7fd7a84 to 67290e9 Compare June 23, 2020 23:47

gronerl marked this pull request as ready for review June 23, 2020 23:49

mcgibbon reviewed Jun 24, 2020

View reviewed changes

havogt added this to Backlog in GridTools via automation Aug 5, 2020

havogt moved this from Backlog to To Do in GridTools Aug 5, 2020

tehrengruber mentioned this pull request Aug 6, 2020

Support for 1d and 2d fields #71

Closed

Linus Groner and others added 13 commits August 7, 2020 13:46

Initial GDP-3: Duck Storages

866315b

GDP: support for permuted axes, default_origin->aligned_index

1455d49

asstorage->as_storage, addition of to_numpy/to_cupy/to_ndarray method…

490b46c

…s, halo parameter and halo/domain attributes

First round of suggested changes for GDP 3 - Duck storages

a981066

Co-authored-by: Linus Groner <linus.groner@cscs.ch> Co-authored-by: Enrique G. Paredes <enriqueg@cscs.ch>

WIP

969337d

First "public" draft of GDP-3

4e4539a

Co-authored-by: Linus Groner <linus.groner@cscs.ch> Co-authored-by: Enrique G. Paredes <enriqueg@cscs.ch>

Expanded explanation of ufuncs

0965b5f

Fix typos and move paragraph to right location

5497966

Fixed error in description of layout parameter, layout fix in rst sou…

01e1caf

…rce.

Improved table layout

38ce2e8

writing style: expand "s.t." to "such that"

b2cee25

Co-authored-by: Jeremy McGibbon <jeremy.mcgibbon@gmail.com>

address @mcgibbon review comments.

acb1d83

added data interface to GDP

230e458

gronerl force-pushed the gdp-duck-storage branch from b6aebd2 to 230e458 Compare August 7, 2020 11:47

egparedes mentioned this pull request Aug 7, 2020

Slices of gt4py storages cannot be copied #31

Closed

Linus Groner added 3 commits August 7, 2020 16:15

removed axes-depending storage properties (except in allocation)

db0c8b3

added description of default xarray accessor and briefly discuss pr…

0ac700d

…evious implementation with `dims` as alternative.

Changed layout definition, added layout property to storage

fcf1454

havogt moved this from To Do to In Progress in GridTools Aug 18, 2020

stripped ufuncs, numpy api except transpose

eca3d5d

mbianco moved this from In Progress to To Do in GridTools Sep 2, 2020

Updated details of use of __gt_data_interface__ and removed residuals…

8e9e9f3

… of ufuncs

gronerl requested a review from egparedes September 8, 2020 11:32

gronerl moved this from To Do to In Progress in GridTools Sep 15, 2020

added halo back

f183fc2

egparedes assigned gronerl Oct 13, 2020

gronerl mentioned this pull request Oct 14, 2020

Duck Storage: Implementation #29

Closed

mbianco moved this from In Progress to Done in GridTools Oct 14, 2020

mcgibbon reviewed Oct 19, 2020

View reviewed changes

mcgibbon mentioned this pull request Nov 9, 2020

fix bug where quantity.view could become out of sync with data ai2cm/fv3gfs-util#27

Merged

mcgibbon reviewed Dec 9, 2020

View reviewed changes

gronerl mentioned this pull request Jan 19, 2021

GDP-3: Duck Storages: reference implementation #308

Closed

Linus Groner added 4 commits November 11, 2021 09:32

Declined GDP

5079972

Merge branch 'master' into gdp-duck-storage

a6efcaf

Moved GDP-3 file

366ab83

Added GDP in new location...

1153138

egparedes approved these changes Nov 11, 2021

View reviewed changes

egparedes merged commit 9057f90 into GridTools:master Nov 11, 2021

tehrengruber pushed a commit to tehrengruber/gt4py that referenced this pull request Jul 28, 2022

Merge pull request GridTools#28 from tehrengruber/tuple_out_program_c…

34ee813

…onstruct_inside Remove code duplication and decrease complexity of PAST Lowering

gronerl deleted the gdp-duck-storage branch January 20, 2023 10:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GDP-3: Duck Storages #28

GDP-3: Duck Storages #28

gronerl commented May 26, 2020

gronerl commented Jun 26, 2020

mcgibbon left a comment •

edited

Loading

mcgibbon Oct 19, 2020

mcgibbon Oct 19, 2020

mcgibbon Oct 19, 2020

mcgibbon Oct 19, 2020

mcgibbon Oct 19, 2020

mcgibbon Oct 19, 2020

mcgibbon Oct 19, 2020

mcgibbon Oct 19, 2020

mcgibbon Oct 19, 2020

mcgibbon Oct 19, 2020

mcgibbon Oct 19, 2020

mcgibbon Dec 9, 2020

havogt commented Sep 28, 2021

gronerl commented Nov 3, 2021

ofuhrer commented Nov 4, 2021

mcgibbon commented Nov 4, 2021

gronerl commented Nov 11, 2021

egparedes left a comment

		The implementation of this GDP breaks the integration with external libraries which require a NumPy
		`ndarray` subclass. Further, we propose some API changes like renaming or repurposing of keyword

		meaning is the same as in the NumPy :code:`__array_interface__` and the
		:code:`__cuda_array_interface__`:

	In Addition, the following optional keys can be contained:
	In addition, the following optional keys can be contained:

		The rationale behind this is that in this way, storages allocated with :code:`defaults` set to a
		backend will always get optimal performance, while :code:`defaults` set to :code:`"F"` or

		+ :code:`"acquire": Optional[Callable[[], Any]]` Is called on all objects that are passed to a
		stencil, before running computations. It can be used to trigger a copy to the respective device.

GDP-3: Duck Storages #28

GDP-3: Duck Storages #28

Conversation

gronerl commented May 26, 2020

gronerl commented Jun 26, 2020

mcgibbon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

havogt commented Sep 28, 2021

gronerl commented Nov 3, 2021

ofuhrer commented Nov 4, 2021

mcgibbon commented Nov 4, 2021

gronerl commented Nov 11, 2021

egparedes left a comment

Choose a reason for hiding this comment

mcgibbon left a comment •

edited

Loading