-
Notifications
You must be signed in to change notification settings - Fork 37
[Feature] Support vanilla C++ STL in FFI. #228
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @DarkSharpness, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly extends the Foreign Function Interface (FFI) capabilities by adding direct support for several standard C++ STL containers, including Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces support for several C++ STL containers within the FFI layer, a valuable feature that enhances compatibility and simplifies usage. The implementation is well-structured, primarily using TypeTraits specializations for std::array, std::vector, std::tuple, std::optional, and std::variant. The modifications in function_details.h aim to correctly handle argument passing by reference. While the overall approach is solid, I've identified a critical bug in the std::optional TypeTraits implementation that would cause infinite recursion, and a performance issue related to const reference handling in function_details.h. My review includes specific suggestions to address these points.
|
please split the core changes likely stl should go into extra. The main reason is that while stl can be helpful sometimes, the tradeoff is when present in cases like object, we can no longer to near zero cost access in static languages like rust via directly ptr and access (otherwise it have to be a call to reflection getter that does conversion under the hood, so we should always encourage ffi types when possible (and document such rationales in stl.h). |
I agree that stl should go into extra. The main goal of this PR is to introduce a pybind/PyTorch-style interface that provides greater flexibility for user-defined conversions. In performance-critical paths, however, FFI types should always take priority. This change only performs a one-time transformation for convenience and potential backward compatibility with existing code. |
|
@DarkSharpness that sounds good, can we still move function_details.h chaneg into a separate PR? mainly want to make sure core changes are clearly scoped |
|
@tqchen now in #229 . Also tried to fix the More tests of this PR is working in progress. |
Related PR #228. We now support all kinds of argument type (T, const T, T&, const T&, T&&, const T&& have been tested) in C++ exported functions.
0ee3492 to
7749d6f
Compare
|
cc @tqchen . We now support For STL containers, when passed from Python to C++, we will always construct from the Python object. When passed from C++ to Python, we will construct the corresponding object (e.g. We also allow types that doesn't enable storage (e.g. Finally, do you think I should split the |
|
Thanks @DarkSharpness , would be good to get some reviews, cc @junrushao @Ubospica @Kathryn-cat |
junrushao
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this change. Please rebase to latest HEAD and I’m happy to get it in.
If I read correctly, it doesn’t support transferring a C++ STL container’s ownership to Python, or allow Python side to inplace update C++ STL containers, right? It’s okay given STLs are really secondary in our system, but we will need to document this properly.
In the old code, we reuse traits like `TypeTrait<Array<T>>` and `TypeTrait<Tuple<T>>`. This may result in unwanted copy between FFI containers and STL containers. We fix all of them.
88e0eec to
c46ea0f
Compare
|
rebased cc @junrushao . Yes, your understanding is right. Since STL is not native to tvm::ffi, type conversion will happen at each Python <-> C++. I'm not sure where to document this. I have added a warning here in /*!
* \file tvm/ffi/extra/stl.h
* \brief STL container support.
* \note This file is an extra extension of TVM FFI,
* which provides support for STL containers in C++ exported functions.
*
* Whenever possible, prefer using tvm/ffi/container/ implementations,
* such as `tvm::ffi::Array` and `tvm::ffi::Tuple`, over STL containers
* in exported functions for better performance and compatibility.
*/ |
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this to tests/python/cpp_src
include/tvm/ffi/extra/stl.h
Outdated
| * | ||
| * Whenever possible, prefer using tvm/ffi/container/ implementations, | ||
| * such as `tvm::ffi::Array` and `tvm::ffi::Tuple`, over STL containers | ||
| * in exported functions for better performance and compatibility. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add more notes:
Native ffi objects comes with stable data layout and can be directly accessed through compiled languages (Rust) and DSLs(via LLVM) with raw pointer access for better performance and compatibility
|
Should be good to go once CI greens up |
|
This CI failures (seg-fault) make no sense to me. The latest commit is just a doc-string update. It's rather tricky as I failed to replay the seg-fault in my local environment. Still investigating on the root cause. |
Similar to pybind, we add a
stl.hwhich supportarray,vector,tuple,optionalandvariant. After this file is included, users can use native C++ components, which could hopefully improve compatibility and reduce manual effort in converting between tvm::ffi components to C++ STL components.We also modify thefunction_detail.ha little, so that we support all kinds of argument type (T,const T,T&,const T&,T&&,const T&&have been tested) in C++ exported functions.Example code:
Python part: