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

Do not check type dependencies when determine if bindings should be skipped #304

Conversation

haiiliin
Copy link
Contributor

@haiiliin haiiliin commented Jun 25, 2024

Description

I am using binder to generate bindings for functions/methods that take Eigen matrices as arguments or return values. For example:

#include <Eigen/Dense>

void eigen_argument_function(const Eigen::Matrix<T, 3, 3> &m) {};

I don't want to bind the whole Eigen library, thus I have to exclude the Eigen namespace for binder:

- namespace Eigen

However, the bindings of functions/methods that take Eigen matrices as arguments or return values do not generate because these functions/methods depend on the eigen library and thus skipped. Bindings of any functions/methods that have dependencies of excluded namespaces will not be generated, and this is not what I wanted.

In my own understanding, when I exclude a namespace, I mean I only want to exclude classes/functions in that namespace, but not classes/functions that have dependencies on that namespace.

In addition, I found that when generating bindings for enums, the dependencies will not be checked:

binder/source/enum.cpp

Lines 59 to 67 in 7d6ec91

/// check if user requested binding for the given declaration
bool is_binding_requested(clang::EnumDecl const *E, Config const &config)
{
if( config.is_enum_binding_requested( E->getQualifiedNameAsString() ) ) return true;
bool bind = config.is_namespace_binding_requested(namespace_from_named_decl(E));
//for( auto &t : get_type_dependencies(E) ) bind &= !is_skipping_requested(t, config);
return bind;
}

binder/source/enum.cpp

Lines 69 to 82 in 7d6ec91

// check if user requested skipping for the given declaration
bool is_skipping_requested(clang::EnumDecl const *E, Config const &config)
{
string qualified_name = standard_name(E->getQualifiedNameAsString());
if( config.is_enum_skipping_requested(qualified_name) ) return true;
if( config.is_enum_binding_requested(qualified_name) ) return false;
bool skip = config.is_namespace_skipping_requested(namespace_from_named_decl(E));
//for( auto &t : get_type_dependencies(E) ) skip |= is_skipping_requested(t, config);
return skip;
}

If this change is not appropriate, maybe adding a configuration option would be helpful.

@lyskov
Copy link
Member

lyskov commented Jun 25, 2024

@haiiliin i see your point. Current design and behavior is intentional though. Here is how it ""suppose to work": explicitly trigger bindings of namespaces that you want to bind and do not mention (either with +namespace or -namespace) namespaces that you do not mind being bound but for which you do not bindings by default, ie like the bindings for Eigen library in your case.

So say your main project is reside in C++ namespace aaaa, then we specify +namespace aaaa and do not mention eigen namespace at all. The result should be that only function/classes from Eigen that got used in your main project will be bound. Have you tried this approach?

If you curios: reason for current design is that - namespace/class/function options is viewed as "direct" orders to skip particular objects no matter what.

@haiiliin
Copy link
Contributor Author

So say your main project is reside in C++ namespace aaaa, then we specify +namespace aaaa and do not mention eigen namespace at all. The result should be that only function/classes from Eigen that got used in your main project will be bound. Have you tried this approach?

I tried this approach, but it turned out the bindings of the eigen library are still generated in the unknown directory, same as the situation in #85.

@lyskov
Copy link
Member

lyskov commented Jun 26, 2024

Do you see some binding generated for Eigen or do you have reason to believe that Binder is trying to bind all the Eigen code? The former is expected behavior while later could be indicator of bug in Binder. Also, could you please cut-paste +/-namespace directives from you Binder config here?

Re placement bindings in unknown dir: i am guessing the Eigen library using comma-based incudes which usually make Binder fall back to auto generating source file names in unknown dir. This by itself is not a sign of any problem.

EDIT: minor fixes for clarity

@haiiliin
Copy link
Contributor Author

Binder tries to generate some bindings for the library. For simple eigen library usage, the binder only generates several unknown files. However, my project generates almost 5000 unknown files. I don't use many of the eigen library, just the eigen matrix, some of its methods, and the linear algebra functions.

Neither way, the build will fail. https://github.com/haiiliin/binder-eigen-test is a project for the testing, please check it out.

Here are some logs of the build:

[1/2] Building CXX object bindings\CMakeFiles\bindings.dir\unknown\unknown.cpp.obj
FAILED: bindings/CMakeFiles/bindings.dir/unknown/unknown.cpp.obj 
C:\PROGRA~1\MIB055~1\2022\COMMUN~1\VC\Tools\MSVC\1437~1.328\bin\Hostx64\x64\cl.exe  /nologo /TP -Dbindings_EXPORTS -IC:\Users\hailin\Desktop\binder-eigen-test\deps\eigen -IC:\Users\hailin\Desktop\binder-eigen-test\include -external:IC:\Users\hailin\AppData\Local\Programs\Python\Python312\include -external:IC:\Users\hailin\AppData\Local\Programs\Python\Python311\include -external:W0 /DWIN32 /D_WINDOWS /EHsc /Zi /Ob0 /Od /RTC1 -MDd /bigobj /showIncludes /Fobindings\CMakeFiles\bindings.dir\unknown\unknown.cpp.obj /Fdbindings\CMakeFiles\bindings.dir\ /FS -c C:\Users\hailin\Desktop\binder-eigen-test\bindings\unknown\unknown.cpp
C:\Users\hailin\AppData\Local\Programs\Python\Python311\include\pybind11\cast.h(746): error C2338: static_assert failed: 'Holder classes are only supported for custom types'
C:\Users\hailin\AppData\Local\Programs\Python\Python311\include\pybind11\cast.h(820): note: see reference to class template instantiation 'pybind11::detail::copyable_holder_caster<base,holder,void>' being compiled
        with
        [
            base=Eigen::Matrix<float,3,3,0,3,3>,
            holder=std::shared_ptr<Eigen::Matrix<float,3,3,0,3,3>>
        ]
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.37.32822\include\type_traits(1312): note: see reference to class template instantiation 'pybind11::detail::type_caster<holder,void>' being compiled
        with
        [
            holder=std::shared_ptr<Eigen::Matrix<float,3,3,0,3,3>>
        ]
C:\Users\hailin\AppData\Local\Programs\Python\Python311\include\pybind11\cast.h(867): note: see reference to class template instantiation 'std::is_base_of<pybind11::detail::copyable_holder_caster<base,holder,void>,pybind11::detail::type_caster<holder,void>>' being compiled
        with
        [
            base=Eigen::Matrix<float,3,3,0,3,3>,
            holder=std::shared_ptr<Eigen::Matrix<float,3,3,0,3,3>>
        ]
C:\Users\hailin\AppData\Local\Programs\Python\Python311\include\pybind11\detail/common.h(863): note: see reference to class template instantiation 'pybind11::detail::is_holder_type<type_,std::shared_ptr<Eigen::Matrix<float,3,3,0,3,3>>>' being compiled
        with
        [
            type_=Eigen::Matrix<float,3,3,0,3,3>
        ]
C:\Users\hailin\AppData\Local\Programs\Python\Python311\include\pybind11\pybind11.h(1511): note: see reference to class template instantiation 'pybind11::detail::exactly_one<pybind11::class_<Eigen::Matrix<float,3,3,0,3,3>,std::shared_ptr<Eigen::Matrix<float,3,3,0,3,3>>>::is_holder,std::unique_ptr<Eigen::Matrix<float,3,3,0,3,3>,std::default_delete<Eigen::Matrix<float,3,3,0,3,3>>>,std::shared_ptr<Eigen::Matrix<float,3,3,0,3,3>>>' being compiled
C:\Users\hailin\AppData\Local\Programs\Python\Python311\include\pybind11\pybind11.h(1511): note: see reference to alias template instantiation 'pybind11::detail::exactly_one_t<pybind11::class_<Eigen::Matrix<float,3,3,0,3,3>,std::shared_ptr<Eigen::Matrix<float,3,3,0,3,3>>>::is_holder,std::unique_ptr<Eigen::Matrix<float,3,3,0,3,3>,std::default_delete<Eigen::Matrix<float,3,3,0,3,3>>>,std::shared_ptr<Eigen::Matrix<float,3,3,0,3,3>>>' being compiled
C:\Users\hailin\Desktop\binder-eigen-test\bindings\unknown\unknown.cpp(18): note: see reference to class template instantiation 'pybind11::class_<Eigen::Matrix<float,3,3,0,3,3>,std::shared_ptr<Eigen::Matrix<float,3,3,0,3,3>>>' being compiled
C:\Users\hailin\Desktop\binder-eigen-test\bindings\unknown\unknown.cpp(43): error C2440: 'type cast': cannot convert from 'overloaded-function' to 'Eigen::Matrix<float,3,3,0,3,3> &(__cdecl Eigen::Matrix<float,3,3,0,3,3>::* )(const Eigen::Matrix<float,3,3,0,3,3> &) const'
C:\Users\hailin\Desktop\binder-eigen-test\bindings\unknown\unknown.cpp(43): note: None of the functions with this name in scope match the target type
C:\Users\hailin\Desktop\binder-eigen-test\bindings\unknown\unknown.cpp(84): error C2440: 'type cast': cannot convert from 'overloaded-function' to 'Eigen::Matrix<double,3,3,0,3,3> &(__cdecl Eigen::Matrix<double,3,3,0,3,3>::* )(const Eigen::Matrix<double,3,3,0,3,3> &) const'
C:\Users\hailin\Desktop\binder-eigen-test\bindings\unknown\unknown.cpp(84): note: None of the functions with this name in scope match the target type
ninja: build stopped: subcommand failed.

@haiiliin haiiliin closed this Jun 30, 2024
@lyskov
Copy link
Member

lyskov commented Jul 1, 2024

re 5k files: this most likely is due to dependencies chain: ie your project used the some of the Matrix classes which in turn uses some other classes and so on. If this is absolutely not what you want then perhaps using your proposed path for your projects is the best approach. I certainly have similar cases in the past when for some projects it was not unclear how bind them without introducing some project specific features into Binder 🤷🏻‍♂️.

Re errors: which Pybind11 version do you use? If this is upstream then i would recommend to try use one from our fork and see if that makes a difference. Also, it might worth to check if generated code look correct and if yes then propagate this to Pybind11 team.

Hope this helps,

@haiiliin haiiliin deleted the do-not-check-type-dependenicies branch July 29, 2024 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants