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

Declare-then-bind for the Python wrapper for better type generation #1824

Open
ProfFan opened this issue Sep 5, 2024 · 3 comments
Open

Declare-then-bind for the Python wrapper for better type generation #1824

ProfFan opened this issue Sep 5, 2024 · 3 comments
Labels
feature New proposed feature

Comments

@ProfFan
Copy link
Collaborator

ProfFan commented Sep 5, 2024

Currently the Python type generation has a problem: many classes are not registered to pybind11 when they got used as function parameters, resulting in errors like:

pybind11_stubgen - [  ERROR] In gtsam.gtsam.TranslationRecovery.addPrior : Invalid expression 'gtsam::BinaryMeasurement<gtsam::Unit3>'

This is not an issue for pybind11-stubgen or mypy's stub-gen. This is actually caused by this: https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings

The solution is "declare-then-bind", which works by first only do the declaration and then after all declarations start binding the functions.

So instead of this:

py::class_<gtsam::Value, std::shared_ptr<gtsam::Value>>(m_, "Value")
        .def("print",[](gtsam::Value* self, string s){ py::scoped_ostream_redirect output; self->print(s);}, py::arg("s") = "")
        .def("__repr__",
                    [](const gtsam::Value& self, string s){
                        gtsam::RedirectCout redirect;
                        self.print(s);
                        return redirect.str();
                    }, py::arg("s") = "")
        .def("dim",[](gtsam::Value* self){return self->dim();});

we should do:

py::class_<gtsam::Value, std::shared_ptr<gtsam::Value>>(m_, "Value") gtsam_value;
py::class_<XXX>(m_, "Xxx") gtsam_xxx;
...

gtsam_value
        .def("print",[](gtsam::Value* self, string s){ py::scoped_ostream_redirect output; self->print(s);}, py::arg("s") = "")
        .def("__repr__",
                    [](const gtsam::Value& self, string s){
                        gtsam::RedirectCout redirect;
                        self.print(s);
                        return redirect.str();
                    }, py::arg("s") = "")
        .def("dim",[](gtsam::Value* self){return self->dim();});
gtsam_xxx
        .def(...);

Opening this here as a future TODO.

@ProfFan ProfFan added the feature New proposed feature label Sep 5, 2024
@ProfFan
Copy link
Collaborator Author

ProfFan commented Sep 6, 2024

Another problem is cross-module dependency. This is due to circular dependency in GTSAM, for example, for Values.

This is slightly harder which basically goes like this http://doxygen.lsst.codes/stack/doxygen/xlink_master_2020_06_24_18.09.15/classlsst_1_1utils_1_1python_1_1_wrapper_collection.html

Typical usage:

// _mypackage.cc
 
void wrapClassA(WrapperCollection & wrappers);
void wrapClassB(WrapperCollection & wrappers);
 
PYBIND11_MODULE(_mypackage, module) {
    WrapperCollection wrappers(module, "mypackage");
    wrapClassA(wrappers);
    wrapClassB(wrappers);
    wrappers.finish();
}
// _ClassA.cc
 
void wrapClassA(WrapperCollection & wrappers) {
    wrappers.wrapType(
        py::class_<ClassA>(wrappers.module, "ClassA"),
        [](auto & mod, auto & cls) {
            cls.def("methodOnClassA", &methodOnClassA);
        }
    );
}
// _ClassB.cc
 
void wrapClassB(WrapperCollection & wrappers) {
    wrappers.wrapType(
        py::class_<ClassB>(wrappers.module, "ClassB"),
        [](auto & mod, auto & cls) {
            cls.def("methodOnClassB", &methodOnClassB);
            mod.def("freeFunction", &freeFunction);
        }
    );
}

which literally defers all the lambdas to execute only after all the types are declared.

@ProfFan
Copy link
Collaborator Author

ProfFan commented Sep 6, 2024

Note LSST is GPLv3, so we need to write our own version of the approach, but I think it's not too hard to do.

@varunagrawal
Copy link
Collaborator

I've been getting this a lot. Would it make sense to switch back to mypy until this is resolved? I spent a while debugging these errors before seeing this issue and I am guessing others will have this problem too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New proposed feature
Projects
None yet
Development

No branches or pull requests

2 participants