Centralize code for creating new code generation backends #158

hmartiro · 2022-06-03T05:45:17Z

Move almost all backend-specific code into a sub-package of symforce/codegen/backends, rather than being scattered around multiple core files. This includes the code printer, codegen config, and a bunch of switch statements around the codegen machinery. The goal is to centralize and make it cleaner to add new codegen backends. These changes were made in parallel with adding a javascript backend, which will come in a follow-on review.

See this README for the steps to add a backend after this review.

There are still several ways to improve code quality and reorganize backend-specific code, but this is a step forward.

TODO: Regenerate all templates, since the template path changed (written to preamble).

hmartiro · 2022-06-03T05:59:50Z

Here's what it looks like to add a language after this review: #159

symforce/codegen/backends/README.md

symforce/codegen/backends/cpp/cpp_config.py

hayk-skydio · 2022-06-03T15:50:16Z

symforce/codegen/codegen_util.py

@@ -342,20 +342,19 @@ def get_formatted_list(
            formatted_symbols = [sm.Symbol(key)]
            flattened_value = [value]
        elif issubclass(arg_cls, geo.Matrix):
-            if isinstance(config, codegen_config.PythonConfig):
-                # TODO(nathan): Not sure this works for 2D matrices
+            if config.matrix_is_1d:


I preserved the behavior, but don't fully understand it. Would like to discuss

Let's move this discussion into the IMU preintegration review (#160) where it is in play

resolved in the other PRe

symforce/codegen/template_util.py

aaron-skydio

Not sure about the name "backends" for target languages, since we also use that for symbolic backends. Could call them "targets"?

symforce/codegen/codegen_config.py

aaron-skydio · 2022-06-03T06:11:04Z

symforce/codegen/codegen_config.py


+    def printer(self) -> "sm.CodePrinter":


I think the correct annotation here is sympy.CodePrinter, which mypy should be happy with without the string, and should indicate that they should subclass the sympy printer on either backend

Made it sympy. I think part of the reason this was in quotes was to avoid importing SymPy into the config files. We could import sympy in an if T.TYPE_CHECKING block, but that's a bit ugly. Do you think we should do that?

It's probably fair to assume if you're importing the codegen config you're going to generate code, so you'll need to import sympy anyway - I'd lean towards just importing sympy here, and if there's a use case where that's annoyingly slow we can move it to a T.TYPE_CHECKING block

Okay I'm on board. It looks like it should be sympy.printing.codeprinter.CodePrinter though?

symforce/codegen/backends/README.md

hayk-skydio · 2022-06-05T13:05:03Z

@gcross-zipline since you're working on adding Rust bindings, please see this review which simplifies things. We should aim to rebase on top of this once it merges, so it looks like #159.

hayk-skydio · 2022-06-07T06:01:51Z

Not sure about the name "backends" for target languages, since we also use that for symbolic backends. Could call them "targets"?

It's a good callout. I feel like this use of backend is pretty similar to LLVM's terminology which makes me want to keep it. Target is fine, but I'm just having trouble getting behind it. I wonder if the sympy/symengine thing could be called something else like "get_sympy". Now that i think about it symforce.set_backend does make me think it's setting the language it's going to generate to

aaron-skydio · 2022-06-07T23:11:28Z

Yeah I'm down with calling these backends and renaming sympy/symengine, maybe call that set_symbolic_api? Worried that set_sympy will be confusing between whether it "sets the sympy we're using" or "sets symforce to use sympy (as opposed to symengine)"

hayk-skydio · 2022-06-11T19:33:19Z

Yeah I'm down with calling these backends and renaming sympy/symengine, maybe call that set_symbolic_api? Worried that set_sympy will be confusing between whether it "sets the sympy we're using" or "sets symforce to use sympy (as opposed to symengine)"

Okay, doing that.

Move almost all backend-specific code into a sub-package of symforce/codegen/backends, rather than being scattered around multiple core files. This includes the code printer, codegen config, and a bunch of switch statements around the codegen machinery. The goal is to centralize and make it cleaner to add new codegen backends. These changes were made in parallel with adding a javascript backend, which will come in a follow-on review. See codegen/backends/README.md for the steps to add a backend after this review. There are still several ways to improve code quality and reorganize backend-specific code, but this is a step forward.

Because we are calling the codegen langauge targets "backends", it would be confusing to keep doing so with the SymPy / SymEngine switch. Backend matches LLVM terminology, so it makes sense there. We switch to using symbolic_api for the latter, which has some downsides in that we're setting the implementation and not the API, but it seems like the best choice of the options we've discussed.

gareth-cross

Added a couple of comments based on my experience adding basic rust code-gen support.

gareth-cross · 2022-06-21T21:25:19Z

symforce/codegen/codegen.py

@@ -393,56 +394,23 @@ def generate_function(
        self.namespace = namespace

        template_data = dict(self.common_data(), spec=self)


Having the config be able to modify template_data would be useful. For example, to expose utility methods that are specific to a given language: main...ZiplineTeam:add-rust-code-generation#diff-655b6a9779a72c1b59d1990ad1581a08986c7676a68d1f0c72a06018797e8c65R454

That link isn't working for me?

Eh, strange. In any case, it just adds another module to the template_data dict like so:

import rust_util template_data.update(rust_util=rust_util) # <- rust_util can be called from jinja now.

Giving different language CodegenConfig the ability to modify the data passed to jinja could be useful.

gareth-cross · 2022-06-21T21:25:34Z

symforce/codegen/codegen_config.py

@@ -20,6 +24,7 @@ class CodegenConfig:
        use_eigen_types: Use eigen_lcm types for vectors instead of lists


Should use_eigen_types exist on the base class, as it does not exist in other languages. (I realize this is unchanged by your PR, but it seemed relevant to the change).

It affects what we put in LCM definitions for generated LCM types regardless of target language (although of course the languages that currently support generated functions that take/return LCM types are currently just Python and C++ I think)

gareth-cross · 2022-06-21T21:28:16Z

symforce/codegen/codegen_util.py

                # NOTE(brad): The order of the symbols must match the storage order of geo.Matrix
                # (as returned by geo.Matrix.to_storage). Hence, if there storage order were
                # changed to, say, row major, the below for loops would have to be swapped to
                # reflect that.
+                formatted_symbols = []
                for j in range(value.shape[1]):
                    for i in range(value.shape[0]):
                        formatted_symbols.append(sm.Symbol(f"{key}({i}, {j})"))


Probably not immediately important to you, but this indexing style also requires customization in some languages (eg. Rust). Could eventually have a method like format_data_accessor for this as well.

+1 from me on that

hmartiro requested review from aaron-skydio and bradley-solliday-skydio June 3, 2022 05:52

hmartiro mentioned this pull request Jun 3, 2022

JavaScript backend core functionality #159

Open

hayk-skydio reviewed Jun 3, 2022

View reviewed changes

aaron-skydio reviewed Jun 3, 2022

View reviewed changes

hayk-skydio assigned aaron-skydio Jun 5, 2022

hmartiro force-pushed the centralize-backend-specific-code branch from a76176b to 3715c7d Compare June 11, 2022 20:09

hayk-skydio added 2 commits June 11, 2022 13:16

hmartiro force-pushed the centralize-backend-specific-code branch from 3715c7d to 5874834 Compare June 11, 2022 20:17

aaron-skydio approved these changes Jun 13, 2022

View reviewed changes

gareth-cross reviewed Jun 21, 2022

View reviewed changes

hayk-skydio closed this Jun 28, 2022

aaron-skydio deleted the centralize-backend-specific-code branch September 28, 2022 07:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Centralize code for creating new code generation backends #158

Centralize code for creating new code generation backends #158

hmartiro commented Jun 3, 2022 •

edited by hayk-skydio

Loading

hmartiro commented Jun 3, 2022

hayk-skydio Jun 3, 2022

hayk-skydio Jun 5, 2022

hayk-skydio Jun 11, 2022

aaron-skydio left a comment

aaron-skydio Jun 3, 2022

hayk-skydio Jun 5, 2022

aaron-skydio Jun 6, 2022

hayk-skydio Jun 11, 2022

hayk-skydio commented Jun 5, 2022 •

edited

Loading

hayk-skydio commented Jun 7, 2022

aaron-skydio commented Jun 7, 2022

hayk-skydio commented Jun 11, 2022

gareth-cross left a comment

gareth-cross Jun 21, 2022

aaron-skydio Jun 21, 2022 •

edited

Loading

gareth-cross Jun 21, 2022

gareth-cross Jun 21, 2022

aaron-skydio Jun 21, 2022

gareth-cross Jun 21, 2022

aaron-skydio Jun 21, 2022

		@@ -393,56 +394,23 @@ def generate_function(
		self.namespace = namespace

		template_data = dict(self.common_data(), spec=self)

		@@ -20,6 +24,7 @@ class CodegenConfig:
		use_eigen_types: Use eigen_lcm types for vectors instead of lists

Centralize code for creating new code generation backends #158

Centralize code for creating new code generation backends #158

Conversation

hmartiro commented Jun 3, 2022 • edited by hayk-skydio Loading

hmartiro commented Jun 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaron-skydio 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

hayk-skydio commented Jun 5, 2022 • edited Loading

hayk-skydio commented Jun 7, 2022

aaron-skydio commented Jun 7, 2022

hayk-skydio commented Jun 11, 2022

gareth-cross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaron-skydio Jun 21, 2022 • 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

hmartiro commented Jun 3, 2022 •

edited by hayk-skydio

Loading

hayk-skydio commented Jun 5, 2022 •

edited

Loading

aaron-skydio Jun 21, 2022 •

edited

Loading