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

Add a CI workflow for building against Eigen from source #749

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dfm
Copy link

@dfm dfm commented Oct 4, 2024

This isn't really meant to be merged, but it's a follow up from #746 demonstrating the test failure with the current master branch of Eigen. This workflow seems to fail when run on macOS (but not on Ubuntu), and (as @hawkinsp suggested) I expect the issue is related to using clang instead of gcc.

One way to "fix" this test failure is to update this function:

using Matrix1d = Eigen::Matrix<double,1,1>;
try {
m.def(
"default_arg", [](Matrix1d a, Matrix1d b) { return a + b; },
"a"_a = Matrix1d::Zero(), "b"_a = Matrix1d::Zero());
} catch (...) {
// Ignore (NumPy not installed, etc.)
}

as follows

-             "default_arg", [](Matrix1d a, Matrix1d b) { return a + b; },
+             "default_arg", [](Matrix1d a, Matrix1d b) -> Matrix1d { return a + b; },

but I think there's probably something more complicated happening here.

This isn't really meant to be merged, but it's a follow up from
wjakob#746 demonstrating the test
failure with the current master branch of Eigen.

This workflow seems to fail when run on macOS, and (as @hawkinsp
suggested) I expect the issue is related to using clang instead of gcc.
@wjakob
Copy link
Owner

wjakob commented Oct 5, 2024

That looks concerning since it affects such basic types. Let me add @WKarel as well.

@WKarel
Copy link
Contributor

WKarel commented Oct 7, 2024

a + b should create an instance of a specialization of Eigen::CwiseBinaryOp for scalar sums and the given specialization of Eigen::Matrix. Upon return, this should call nanobind's specialization of type_caster for dense Eigen expressions, which should forward to the specialization of type_caster for Eigen::Array<double, 1, 1>.

What happens if you replace the trailing return type Eigen::Matrix<double,1,1> with Eigen::Array<double,1,1>?

I do not see why type_caster<dense expression>::from_cpp is a function template:

    template <typename T2>
    static handle from_cpp(T2 &&v, rv_policy policy, cleanup_list *cleanup) noexcept {
        return Caster::from_cpp(std::forward<T2>(v), policy, cleanup);
    }

Replacing it with

    static handle from_cpp(T&& v, rv_policy policy, cleanup_list* cleanup) noexcept {
      return Caster::from_cpp(std::move(v), policy, cleanup);
    }
    static handle from_cpp(const T& v, rv_policy policy, cleanup_list* cleanup) noexcept {
      return Caster::from_cpp(v, policy, cleanup);
    }

might actually fix the issue with CLang on MacOS.

@dfm
Copy link
Author

dfm commented Oct 7, 2024

Thanks for the suggestions, @WKarel!

What happens if you replace the trailing return type Eigen::Matrix<double,1,1> with Eigen::Array<double,1,1>?

I can confirm that updating the test as follows also fixes the error:

-             "default_arg", [](Matrix1d a, Matrix1d b) { return a + b; },
+             "default_arg", [](Matrix1d a, Matrix1d b) -> Eigen::Array<double, 1, 1> { return a + b; },

However, the error persists if Matrix1d in the test is redefined as Eigen::Array<double, 1, 1>, but the return type is omitted.

Replacing it with ... might actually fix the issue with CLang on MacOS.

I tried updating the following lines (I think this is what you were referring to):

template <typename T2>
static handle from_cpp(T2 &&v, rv_policy policy, cleanup_list *cleanup) noexcept {
return Caster::from_cpp(std::forward<T2>(v), policy, cleanup);
}

with the version you suggest, unfortunately that doesn't seem to fix the problem!

@WKarel
Copy link
Contributor

WKarel commented Oct 7, 2024

I see. And yes, what you did is what I meant to give a try. Now I am out of ideas. So I would try to debug the problematic call, or temporarily introduce some debug output, e.g. starting with std::cerr << typeid(v).name(); just above

return Caster::from_cpp(std::forward<T2>(v), policy, cleanup);

@wjakob
Copy link
Owner

wjakob commented Nov 5, 2024

@dfm: could you please provide the typeid() output as suggested by @WKarel?

@dfm
Copy link
Author

dfm commented Nov 17, 2024

@wjakob, @WKarel — I'm so sorry about disappearing here! I'll see if I can make any progress debugging this later today or tomorrow, but in the meantime, here's what I get for that typeid output when the test fails:

N5Eigen13CwiseBinaryOpINS_8internal13scalar_sum_opIddEEKNS_6MatrixIdLi1ELi1ELi0ELi1ELi1EEES6_EE

If that immediately suggests anything to try, let me know!

@wjakob
Copy link
Owner

wjakob commented Nov 17, 2024

Can you run that through c++filt or c++filt -t on your machine? Just run the command, enter that long string, and post what this outputs.

@dfm
Copy link
Author

dfm commented Nov 18, 2024

Absolutely! That prints:

Eigen::CwiseBinaryOp<Eigen::internal::scalar_sum_op<double, double>, Eigen::Matrix<double, 1, 1, 0, 1, 1> const, Eigen::Matrix<double, 1, 1, 0, 1, 1> const>

@WKarel
Copy link
Contributor

WKarel commented Nov 18, 2024

That type meets my expectations. Does this test also fail in an unoptimized build?

@dfm
Copy link
Author

dfm commented Nov 23, 2024

Does this test also fail in an unoptimized build?

Good question and sorry for the slow response! It looks like switching to a Debug build fixes the error.

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.

3 participants