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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generated __init__.py with multiple functions takes the last #21

Closed
aaron-skydio opened this issue Jan 17, 2022 · 6 comments
Closed

Generated __init__.py with multiple functions takes the last #21

aaron-skydio opened this issue Jan 17, 2022 · 6 comments
Labels
bug Something isn't working core

Comments

@aaron-skydio
Copy link
Member

aaron-skydio commented Jan 17, 2022

A minimal example:

from symforce import codegen
from symforce import typing as T

def a(x: T.Scalar):
        return x + 1

def b(x: T.Scalar):
        return x + 2

output_dir = "<something>"
codegen.Codegen.function(a, config=codegen.PythonConfig()).generate_function(output_dir=output_dir)
codegen.Codegen.function(b, config=codegen.PythonConfig()).generate_function(output_dir=output_dir)

Then output_dir/python/symforce/sym/__init__.py will contain:

# -----------------------------------------------------------------------------
# This file was autogenerated by symforce from template:
#     python_templates/function/__init__.py.jinja
# Do NOT modify by hand.
# -----------------------------------------------------------------------------

from .b import b
@aaron-skydio aaron-skydio added bug Something isn't working core labels Jan 17, 2022
@lowjunen
Copy link

bumping this. I'm working around this in a hacky way by clearing the init of the target directory and just importing the functions directly into my class (without using the codegen_util.load_generated_package function). is there a more elegant/proper way?

@aaron-skydio
Copy link
Member Author

Some thoughts on this:

  1. Generating an __init__.py like this, that imports all generated functions, is going to require some statefulness. For instance, we could read the existing __init__.py in the directory and parse out packages that it's importing and just add to them. Or we could list the contents of the directory and import everything we find. Either way, we're saying that the rendering of a template depends on rendered results of other templates, or stuff on disk, which is a sort of dependency we haven't had to deal with and will get even more thorny if we try to move towards allowing generation of stuff without writing to disk like in generate_function should not generate files on storage automatically #171 .

  2. It's sort of bad practice anyway to import things in __init__.py

For these reasons I'm leaning towards just generating an empty __init__.py (probably could just not generate one, except that py2 requires it). And then of course we'd fix codegen_util.load_generated_package to work correctly with that.

@lowjunen
Copy link

yeah empty __init__.py definitely feels right. i'm also not using codegen_util.load_generated_package. I'm guessing this is useful for setup the code to work with the Values class? Perhaps my grasp on its benefits is off but I find it's a little overkill for what I'm trying to do (i want to generate a set of functions from the full state dynamics of a quad). Ideally the functions should all be in one file. Perhaps we could add this as an optional argument to generate_function?

@aaron-skydio
Copy link
Member Author

codegen_util.load_generated_package is mostly useful if you're primarily interested in generating python code just to run it immediately, as opposed to putting it somewhere you can import from normally (e.g. somewhere on your PYTHONPATH). It sounds like it might be what you want to use if that's what you're trying to do (once we fix this issue), but if you're just trying to generate the python code and then use it e.g. from another python program, yeah you'll want to just import it normally rather than use load_generated_package.

An option to put multiple functions in a single python file (or C++ header, for that matter) is separately a good idea as well I think

@lowjunen
Copy link

understood. Is there anything i can do to contribute to these? The readme sounds supportive of community driven contributions.

@aaron-skydio
Copy link
Member Author

Absolutely! You would be welcome to tackle this and submit a PR, we're happy to provide some guidance too if that's something you'd like to do

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core
Projects
None yet
Development

No branches or pull requests

2 participants