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

Explore pybind11 around wpilibc #605

Open
virtuald opened this issue May 19, 2019 · 12 comments

Comments

@virtuald
Copy link
Member

commented May 19, 2019

RobotPy is a lot of effort to maintain. As more and more third party vendors appear and as the libraries gain more functionality, it's difficult to continue translating Java into Python year after year, and there are often difficult to address performance issues that arise from doing so.

We've had a lot of success with autogenerating pybind11-based wrappers for C++ code for various things, and I think it's worth investigating if we should move to a mostly-wrapper based approach (of course, there are things that would be best done in pure python). If successful, this would potentially make it easier to create wrappers for third party vendor libraries (though we might need to redo our existing wrappers).

While there's definitely an upfront investment required, I feel like it would be less work in the long run. robotpy-cscore is a great example of this -- it doesn't take a lot of effort to update it each year because we don't care about how the internals work, just need to maintain the interface API.

There are definitely packaging issues that would need to be solved, particularly with regards to loading shared libraries on all of our supported platforms. However, if we depend on upstream wpilibc, they're already building those, so we would 'just' need to make the right wheels and it should work.

One really big issue is that moving to wrappers would cause us to lose our simulation capability. This is by far the best feature of RobotPy, and losing it would be a Bad Thing. However, there's a potential here for creating a backend using C++/HAL that could be usable from all languages -- and that would be a net benefit for everyone.

Moving to wrappers would allow us to benefit from any desktop HAL simulation support that third party vendors create in the future -- which if a C++/Java backend existed, they would be more likely to do.

@auscompgeek

This comment has been minimized.

Copy link
Member

commented May 19, 2019

Note that this would mean that pynetworktables would no longer be pure-Python, as wpilibc depends on the C++ ntcore. This will likely make using NetworkTables with Python on a coprocessor to be slightly more painful than currently (although I plan to maintain my packaging to compensate for this on the more common platforms).

Another thing is that this will mean simulation will only be supported on whatever upstream WPILib supports, whereas the RobotPy simulation works on whatever Python runs on. It'll also be more painful to install simulation stuff on anything that PyPI doesn't support wheels for (i.e. other Python implementations (such as PyPy), non-x86 platforms and non-glibc *nix). Dunno whether this would be a deal-breaker for anyone - hopefully people will speak up if so.

The other big thing for me is that this approach will mean duck typing will no longer be possible for anything that gets passed into our wpilib package. Probably not a huge deal-breaker, but something we might need to consider.

One thing we will need to double-check is whether subclassing across pybind11 modules works as expected, isinstance and all. If not, we will have to use something else, perhaps shiboken2.

(It's also possible that using shiboken2 might be easier than trying to manipulate our existing pybind11 wrapper generation to behave correctly for wpilibc. I'm not confident that the stuff in robotpy-rev will actually work correctly as is against wpilibc.)

@PeterJohnson

This comment has been minimized.

Copy link
Member

commented May 19, 2019

Coming full circle, considering that the original RobotPy was SIP-generaged wpilibc wrappers: https://github.com/robotpy/robotpy-crio/tree/2014/Packages/wpilib/sip

@virtuald

This comment has been minimized.

Copy link
Member Author

commented May 19, 2019

Yep! And if you recall, I was skeptical of pure-python wpilib then too. It was a good experiment, and for the most part it's good enough -- but it's so. much. work. Work that just isn't interesting.

Additionally, pybind11 is much easier to work with IMHO than SIP, and things like the advent of wheels have made some of the disadvantages of wrappers less than they used to be.

@ThadHouse

This comment has been minimized.

Copy link

commented May 19, 2019

Regarding the native builds, we do have the CMake build setup that can build basically on any platform, and for just ntcore works even without OpenCV with the right flags, and can ignore Java too. By enabling any pybind builds to detect cmake installed libraries (the build can install them too), you should in theory be able to easily support any platform, not just the base ones we support.

Note for 2020 we will likely be adding upstream support for Jetson TX2 and Jetson Nano, and we already have Pi builds, so it would only be weird other platforms that would need the custom builds.

@auscompgeek

This comment has been minimized.

Copy link
Member

commented May 19, 2019

pybind11 does have a CMake-based build, so if we're going to do CMake that'd be the way to go.

@virtuald

This comment has been minimized.

Copy link
Member Author

commented May 19, 2019

Historically, I've wanted 'pip install foo' to just work, so I've avoided using cmake+pybind11. However, that may need to be revisited.

@auscompgeek

This comment has been minimized.

Copy link
Member

commented May 19, 2019

pybind11 also have an example where they call cmake from setup.py, so that shouldn't be an issue for source builds: https://github.com/pybind/cmake_example/blob/master/setup.py

@ThadHouse

This comment has been minimized.

Copy link

commented May 19, 2019

Yeah, at least for non supported by WPILib platforms. It would likely be best to use the wpilib builds for the platforms we support, to ensure compatibility with vendors, but for other platforms its not a bad solution.

Can you make setup.py do different things on known vs unknown platforms?

And on unknown platforms, I don't think CMake and GCC is an unreasonable requirement. On Windows CMake and MSVC is unreasonable, but binary blobs are much easier on windows then other platforms, because they do actually just work.

@auscompgeek

This comment has been minimized.

Copy link
Member

commented May 19, 2019

setup.py is arbitrary code, so we can make it do anything we want to 😉

@auscompgeek

This comment has been minimized.

Copy link
Member

commented May 19, 2019

Something else that we will need to consider if we go down this path is generating documentation, and being able to generate typing stubs.

@auscompgeek

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

Another random thought: a bunch of methods take out params, and some also take raw pointers to arrays with a separate size param. We will want to make sure these methods have the same method signature our existing pure-Python code has. Doing this with our pybind11 generation scripts may be nontrivial.

@auscompgeek

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

Thinking aloud: Could https://github.com/google/clif be a better option than pybind11 here? hmm... doesn't support overloading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.