symforce-org / symforce

Fast symbolic computation, code generation, and nonlinear optimization for robotics
https://symforce.org
Apache License 2.0
1.41k stars 145 forks source link

Make sym and other genned pkgs namespace packages #261

Open bradley-solliday-skydio opened 1 year ago

bradley-solliday-skydio commented 1 year ago

Namespace packages are packages whose __path__ (which tells the python import system to look for sub-packages and sub-modules) has multiple directories, meaning it can have portions spread out across multiple directories.

Previously, sym and the other generated packages were not namespace packages. This caused issues when generated python packages in the sym namespace attempted to access, for example, sym.Rot3 (which they might do if the generated function returns a sym.Rot3). Since the generated package itself was named sym, but Rot3 was not defined locally, an AttributeError would be raised.

While an alternative would have been to instead not use the name sym for generated packages (using, say, sym_gen instead), for reasons I didn't fully look into, we still want to generate our code within the sym namespace (generating into sym.gen was considered as an option but to do so would require the changes in this commit regardless).

While currently we haven't needed to generate any functions returning a sym class in a package named sym, we intend to soon add a sym.util package which will be used in a lot of places. That can't be done until this namespace conflict is resolved.

Note, while a python2 compatible namespace package has multiple __init__.py files for the top-level package spread across different locations, only one of them will be executed. This makes it difficult to re-export the contents of sub-modules into the top-level namespace.

The normal way to re-export a name is to write

from .sub_module import name

However, since the sub-modules can be created dynamically, it is impossible to re-export all names in this manner, as the first __init__.py that is created has no way of knowing what names one might want to re-export from subsequent modules.

It is possible to put all names one wishes to export in a standard file, say _init.py, then dynamically search for such files and execute their contents, but we considered the additional complexity to be too large of a burden (as users would have a harder time understand their generated code, and this would give future maintainers a hard time).

And so, we decided to simply stop re-exporting any names in the __init__.py's of generated code (kind of in the style of pep 420 python3 packages).

This makes loading a generated function more difficult if one uses codegen_util.load_generated_package, as now simply importing a generated package won't give you access to any of the package's contents. However, this is what codegen_util.load_generated_function is for, so hopefully the user experience shouldn't be too negatively impacted.

The one exception to the general ban of re-exporting names is the sym package, as we still wish to be able to do

import sym

sym.Rot3.identity()

However, because all sub-modules we wish to export from the sym package are known at code-gen time, allowing this is not difficult. This only applies to names in the core sym package, and any additional user generated code in the sym package will not be re-rexported in the top-level namespace.

A user can prevent their package from being generated as a namespace package by setting the namespace_package field of their PythonConfig to False. This is useful in our testing as it is the generated code being tested that is imported, not, for example, the checked in sym package code which is being imported.

As a last note, I believe pkgutil.extend_path only checks for portions of the package on the sys.path, and doesn't check for any portions than can only be found by finders on the sys.meta_path (for example, symforce itself is found by a finder on the sys.meta_path but not on the sys.path during an editable pip install). I don't expect this lapse to pose a problem, and addressing it immediately might just make the __init__.pys look more complicated than they need to be, but if this does become a problem, know that the situation can be partially addressed trying to find the spec using the finders, and adding the spec's submodule_search_locations if found to the __path__.

aaron-skydio commented 1 year ago

I see the mention that sym.gen would require the changes in this PR regardless, are we planning to look into that next/later?

bradley-solliday-skydio commented 1 year ago

I see the mention that sym.gen would require the changes in this PR regardless, are we planning to look into that next/later?

I kind of assumed we would. Though, to make the change we'd have to change a lot of the usages and such (and I didn't want to make this PR any more crowded, as it already contains relatively complicated changes).