Functional: Index fields C++ backend support #1130

petiaccja · 2023-01-18T16:33:42Z

Adds index fields as a distinct argument type for the C++ binding generator. The binding generator generates the correct C++ code that passes gridtools::stencil::positional iterators to the GTFN C++ program code.

…erface params

src/functional/ffront/decorator.py

src/functional/otf/binding/pybind.py

DropD

only minor problems with applying the new import style.

src/functional/ffront/decorator.py

DropD · 2023-01-20T14:55:28Z

src/functional/otf/binding/cpp_interface.py

@@ -19,6 +19,7 @@

 import numpy as np

+import functional.otf.binding.type_specifications as ts


I am all for using the same abbreviations for the same module. This however is using an established abbreviation for a different module (even though that module re-imports a lot of the usual one).

Note, once we accept that we shouldn't try to get away with using the same abbreviation for different modules, the reason for re-importing inside the local type_specifications module goes away.

Well, the idea was that the derived modules are a strict superset of the original type_system.* modules, and in that case you can just simply import the extended module.
Sounds like it would be kinda nice to use, but to be honest it might be better as you suggest. Re-importing all the stuff is ugly and error-prone, and then not knowing which flavour of the module you are using at the moment is also a problem. Plus, it's really nasty if you want to use two extended modules at the same time.

Anyways, I think this is out of the scope of this PR, but we can change it in a follow-up PR.

I agree we should not use the same alias for different modules. I think it would be good enough for this PR to just change the alias to a similar but different one. For example:

import functional.otf.binding.type_specifications as bts

DropD · 2023-01-20T14:59:16Z

src/functional/otf/binding/type_specifications.py

+from functional.type_system.type_specifications import (  # noqa: F401
+    CallableType as CallableType,
+    DataType as DataType,
+    DeferredType as DeferredType,
+    DimensionType as DimensionType,
+    FieldType as FieldType,
+    FunctionType as FunctionType,
+    OffsetType as OffsetType,
+    ScalarKind as ScalarKind,
+    ScalarType as ScalarType,
+    TupleType as TupleType,
+    TypeSpec as TypeSpec,
+    VoidType as VoidType,
+)


If we don't import this module elsewhere as ts, which should be reserved for functional.type_system.type_specifications, then there is no reason for re-importing these classes.

The whole reason for the import <module> [as <name>] style over import <member> from <module> is to be able to track (without IDE help) what is coming from where. Why go out of our way to negate that advantage?

I also agree with @DropD here. I think it would be better if users of the type specs add the original ts module plus additional imports from extension modules to bring new types in scope, instead of trying to expand the original type_specifications module (which makes basically impossible to compose type extensions from different modules):

# This allows using extensions from different modules very easily import functional.type_system.type_specifications as ts import functional.otf.binding.type_specifications as bts

DropD · 2023-01-20T15:05:54Z

src/functional/program_processors/codegens/gtfn/gtfn_module.py

@@ -18,22 +18,37 @@

 import numpy as np

+import functional.otf.binding.type_specifications as ts


ts name should be reserved for functional.type_system.type_specifications

egparedes

Overall, it looks good to me. I left a few comments and suggestions to improve the PR.

egparedes · 2023-01-25T13:38:14Z

src/functional/otf/binding/cpp_interface.py

    else:
+        raise ValueError("unsupported scalar type")


Suggested change

raise ValueError("unsupported scalar type")

raise ValueError(f"Unsupported '{scalar_type}' scalar type.")

I didn't invest too much into exceptions because I want to clean the whole thing up next cycle. Anyways, you have a point that even clean-up is easier if the messages are better to begin with, so I added some better messages.

egparedes · 2023-01-25T13:41:05Z

src/functional/otf/binding/cpp_interface.py

        return f"BufferT{index}&& {param.name}"
+    else:
+        raise ValueError("unsupported type for parameters")


Suggested change

raise ValueError("unsupported type for parameters")

raise ValueError(f"Unsupported '{param.type_}' type for parameters.")

egparedes · 2023-01-25T13:56:46Z

src/functional/otf/binding/pybind.py

        else:
-            type_str = cpp_interface.render_python_type(param.dtype.type)
-        return type_str + " " + param.name
+            raise ValueError("Invalid type.")


Suggested change

raise ValueError("Invalid type.")

raise ValueError(f"Invalid type in '{param}' parameter.")

egparedes · 2023-01-25T14:17:37Z

src/functional/program_processors/codegens/gtfn/gtfn_module.py

+    # TODO: make type_translation extendable and specialize it for the
+    #   bindings to distinguish between buffer-backed fields and index
+    #   fields.


I now understand that the comment about making type_translation extendable is related to use a different mechanism for the pattern matching part with the actual type, and not with the preliminary handling of the messy implementation details of python typing annotations. It makes sense, but still don't see it as a high priority for now.

I wrote a new from_value function that tries to match for IndexFieldType and if it fails it delegates it to the original functional.type_system.from_value function.

I'd like to use a registry that allows you to register a match functor for every type of the type_system, then you can extend without raising the cyclomatic complexity through all these if statements and you can also simply chain the list of matchers to combine modules.

That would also replace the __gt_type__ solution which is an intrusive implementation because it forces objects to be aware of the type system.

egparedes · 2023-01-25T14:22:27Z

src/functional/program_processors/codegens/gtfn/gtfn_module.py

+    if isinstance(obj, IndexField):
+        dtype = type_translation.from_type_hint(obj.dtype.type)
+        assert isinstance(dtype, ts.ScalarType)
+        return interface.Parameter(name, ts.IndexFieldType(axis=obj.axis, dtype=dtype))


I think that, for consistency, this piece of code should be moved into type_translation.from_value(), since the similar handling of embedded.LocatedField is also implemented there. Additionally, it's easier to refactor the type translation code in the future if it is not spread across different functions.

I moved it to a new function from_value under functional.otf.binding.type_translation. It shouldn't be moved into the main type_translation module because IndexFieldType is not present there.

… by re-exporting

tehrengruber

Just adding the partial review I already started.

tehrengruber · 2023-01-30T20:38:39Z

src/functional/otf/binding/cpp_interface.py

-    param: interface.ScalarParameter | interface.BufferParameter, index: int
-) -> str:
-    if isinstance(param, interface.ScalarParameter):
-        return f"{render_python_type(param.scalar_type.type)} {param.name}"


The render_python_type function and _TYPE_MAPPING dict is now dead code and should be removed right?

Extracted from #1130 and married with Connectivities. Co-authored-by: Enrique G. Paredes <18477+egparedes@users.noreply.github.com>

petiaccja added 2 commits January 17, 2023 15:17

use frontend typesystem instead of custom classes for the binding int…

66024ea

…erface params

add index fields to bindings

accd25a

petiaccja commented Jan 20, 2023

View reviewed changes

src/functional/ffront/decorator.py Show resolved Hide resolved

src/functional/otf/binding/pybind.py Show resolved Hide resolved

petiaccja marked this pull request as ready for review January 20, 2023 10:18

petiaccja requested review from DropD and tehrengruber January 20, 2023 10:24

havogt reviewed Jan 20, 2023

View reviewed changes

src/functional/otf/binding/pybind.py Outdated Show resolved Hide resolved

DropD requested changes Jan 20, 2023

View reviewed changes

petiaccja requested a review from egparedes January 24, 2023 16:12

petiaccja added 3 commits January 24, 2023 17:21

simplify param type inference from object

184e930

renamed BufferIterator to BufferSID

308c456

fix code quality

9896961

tehrengruber mentioned this pull request Jan 25, 2023

Functional: Compiled backends integration unstructured #939

Merged

egparedes requested changes Jan 25, 2023

View reviewed changes

petiaccja added 3 commits January 30, 2023 12:29

use complementary module for binding type_system instead of extension…

12abbdf

… by re-exporting

more meaningful error messages

7a9ff98

code quality

e589950

tehrengruber reviewed Jan 31, 2023

View reviewed changes

petiaccja closed this Jan 31, 2023

havogt mentioned this pull request Mar 23, 2023

refactor[next]: Typing for bindings #1218

Merged

havogt added a commit that referenced this pull request Mar 27, 2023

refactor[next]: Typing for bindings (#1218)

9b9652a

Extracted from #1130 and married with Connectivities. Co-authored-by: Enrique G. Paredes <18477+egparedes@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Functional: Index fields C++ backend support #1130

Functional: Index fields C++ backend support #1130

petiaccja commented Jan 18, 2023

DropD left a comment

DropD Jan 20, 2023

petiaccja Jan 23, 2023

egparedes Jan 25, 2023

DropD Jan 20, 2023

egparedes Jan 25, 2023 •

edited

Loading

DropD Jan 20, 2023

egparedes left a comment

egparedes Jan 25, 2023

petiaccja Jan 30, 2023

egparedes Jan 25, 2023

egparedes Jan 25, 2023

egparedes Jan 25, 2023

petiaccja Jan 30, 2023

petiaccja Jan 30, 2023

petiaccja Jan 30, 2023

egparedes Jan 25, 2023 •

edited

Loading

petiaccja Jan 30, 2023

tehrengruber left a comment

tehrengruber Jan 30, 2023

		@@ -19,6 +19,7 @@

		import numpy as np

		import functional.otf.binding.type_specifications as ts

		@@ -18,22 +18,37 @@

		import numpy as np

		import functional.otf.binding.type_specifications as ts

	raise ValueError("unsupported scalar type")
	raise ValueError(f"Unsupported '{scalar_type}' scalar type.")

	raise ValueError("unsupported type for parameters")
	raise ValueError(f"Unsupported '{param.type_}' type for parameters.")

	raise ValueError("Invalid type.")
	raise ValueError(f"Invalid type in '{param}' parameter.")

Functional: Index fields C++ backend support #1130

Functional: Index fields C++ backend support #1130

Conversation

petiaccja commented Jan 18, 2023

DropD left a 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

egparedes Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egparedes left a 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

egparedes Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tehrengruber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egparedes Jan 25, 2023 •

edited

Loading

egparedes Jan 25, 2023 •

edited

Loading