C-API bindings for IFRT #164

mofeing · 2024-10-04T19:40:25Z

This time for real.

mofeing · 2025-01-02T13:57:58Z

i added some utilities for Julia-like conversion in C++ and better communication between Julia and C.

Type<T> is just a placeholder for representing a target type
convert converts the passing object to the object we can pass through FFI (it helps a lot deduplicating the code and avoiding bugs)
span<T> is a small std::span-like object but for C++ <20 and ABI-stable so we can view it from Julia as a Tuple{Csize, Ptr{Cvoid}}

for example, imagine a function that returns a absl::Span<tsl::RCReference<Device>>. then our C binding would be like:

extern "C" span<Device*> the_function(...) {
    return convert(Type<span<Device*>>(), some_func_that_returns_a_absl_Span(...))
}

conceptually works, in practice a need to add some particular implementations so it works (i'm sure it can be done more generically but i don't have the time nor the energy to learn the terribly complicated C++)

mofeing · 2025-01-22T09:29:18Z

@ftynse given this is a priority for the distributed working group, do you mind adding this to PRONTOLab/GB-25#1?

ftynse · 2025-01-24T22:14:52Z

Sure, we are also still figuring out the project management stack

mofeing

build fails, so it's not yet ready to merge but it can be reviewed while we fix it

deps/ReactantExtra/src/error_handling.hpp

deps/ReactantExtra/src/ifrt/Value.cpp

giordano

As mentioned already on Slack, the changes to ReactantExtra should be done in a separate PR, to be merged before this one, to allow a build on Yggdrasil

giordano · 2025-02-07T11:38:15Z

src/IFRT/IFRT.jl

+"""
+    backend(x)
+
+Returns the `Backend` trait of the given IFRT object.
+"""
+function backend end


┌ Error: 1 docstring not included in the manual: │ │ Reactant.IFRT.backend │ │ These are docstrings in the checked modules (configured with the modules keyword) │ that are not included in canonical @docs or @autodocs blocks. └ @ Documenter ~/.julia/packages/Documenter/Bs999/src/utilities/utilities.jl:44

mmm weird? there's nothing fancy around this. do you have an idea of what can be happening?

You simply need to add this docstring to the documentation

ah, misunderstood the problem

deps/ReactantExtra/src/error_handling.hpp

giordano · 2025-02-07T13:58:13Z

More errors in BB:

[07:55:57] ERROR: /workspace/srcdir/Reactant.jl/deps/ReactantExtra/BUILD:355:11: Compiling src/ifrt/Program.cpp failed: (Exit 1): aarch64-apple-darwin20-cc failed: error executing command (from target //:ReactantExtraLib)
[...]
[07:55:57] # Configuration: 733f0a17cb611a25065571f976b63834782b2e1741e54e6f530060798c5acd3e
[07:55:57] # Execution platform: @local_execution_config_platform//:platform
[07:55:57]
[07:55:57] Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
[07:55:57] In file included from src/ifrt/Program.cpp:1:
[07:55:57] In file included from ./src/type_conversion.hpp:8:
[07:55:57] ./src/memory_management.hpp:144:29: error: no member named 'reinterpret_pointer_cast' in namespace 'std'
[07:55:57]   144 |         capture_shared(std::reinterpret_pointer_cast<void>(std::const_pointer_cast<G>(ptr)))
[07:55:57]       |                        ~~~~~^
[07:55:57] ./src/memory_management.hpp:144:58: error: expected '(' for function-style cast or type construction
[07:55:57]   144 |         capture_shared(std::reinterpret_pointer_cast<void>(std::const_pointer_cast<G>(ptr)))
[07:55:57]       |                                                      ~~~~^
[07:55:57] ./src/memory_management.hpp:175:17: error: no member named 'reinterpret_pointer_cast' in namespace 'std'
[07:55:57]   175 |     return std::reinterpret_pointer_cast<T>(get_shared(ptr));
[07:55:57]       |            ~~~~~^
[07:55:57] ./src/memory_management.hpp:175:42: error: 'T' does not refer to a value
[07:55:57]   175 |     return std::reinterpret_pointer_cast<T>(get_shared(ptr));
[07:55:57]       |                                          ^
[07:55:57] ./src/memory_management.hpp:171:19: note: declared here
[07:55:57]   171 | template<typename T>
[07:55:57]       |                   ^
[07:55:57] 4 errors generated.

mofeing · 2025-02-07T14:20:26Z

mmm std::reinterpret_pointer_cast was introduced in C++17, which I think is the version we are using in ReactantExtra. do the logs show the used compiler flags?

giordano · 2025-02-07T14:58:54Z

Sounds like std::reinterpret_pointer_cast was introduced in SDK 11.3, based on a research in https://github.com/phracker/MacOSX-SDKs and https://github.com/roblabla/MacOSX-SDKs.

#164 (comment) still needs to be resolved though.

mofeing · 2025-02-07T15:22:35Z

Sounds like std::reinterpret_pointer_cast was introduced in SDK 11.3, based on a research in https://github.com/phracker/MacOSX-SDKs and https://github.com/roblabla/MacOSX-SDKs.

mmm that explains why it works in my macbook but not on yggy

#164 (comment) still needs to be resolved though.

sure. If it's not giving linking problems, I can set it as just extern "C".

i'm still having this issue though when building it with clang

dlopen(/Users/mofeing/Developer/Reactant.jl/deps/ReactantExtra/bazel-bin/libReactantExtra.so, 0x0001): symbol not found in flat namespace '__ZNSt13exception_ptr31__from_native_exception_pointerEPv'

giordano · 2025-02-07T15:38:26Z

i'm still having this issue though when building it with clang

dlopen(/Users/mofeing/Developer/Reactant.jl/deps/ReactantExtra/bazel-bin/libReactantExtra.so, 0x0001): symbol not found in flat namespace '__ZNSt13exception_ptr31__from_native_exception_pointerEPv'

I don't see that symbol in the BinaryBuilder build:

sandbox:${WORKSPACE} # c++filt -_ __ZNSt13exception_ptr31__from_native_exception_pointerEPv
std::exception_ptr::__from_native_exception_pointer(void*)
sandbox:${WORKSPACE} # nm libReactantExtra.dylib | grep __ZNSt13exception_ptr31__from_native_exception_pointerEPv
sandbox:${WORKSPACE} # nm libReactantExtra.dylib | grep exception | /opt/${MACHTYPE}/bin/${MACHTYPE}-c++filt -_
000000000687eec8 t llvm::ARMAttributeParser::ABI_FP_exceptions(llvm::ARMBuildAttrs::AttrType)
000000000687eee4 t llvm::ARMAttributeParser::ABI_FP_user_exceptions(llvm::ARMBuildAttrs::AttrType)
                 U std::exception::what() const
                 U std::exception_ptr::exception_ptr(std::exception_ptr const&)
                 U std::exception_ptr::~exception_ptr()
                 U std::exception_ptr::operator=(std::exception_ptr const&)
                 U std::__1::__assoc_sub_state::set_exception(std::exception_ptr)
                 U std::exception::~exception()
                 U std::current_exception()
                 U std::rethrow_exception(std::exception_ptr)
0000000001d6cf58 t std::exception_ptr std::make_exception_ptr<std::__1::future_error>(std::__1::future_error)
0000000001d073cc t std::exception_ptr std::make_exception_ptr<std::__1::future_error>(std::__1::future_error)
                 U typeinfo for std::exception
000000000845cee0 s llvm::ARMAttributeParser::ABI_FP_exceptions(llvm::ARMBuildAttrs::AttrType)::strings
000000000845cef0 s llvm::ARMAttributeParser::ABI_FP_user_exceptions(llvm::ARMBuildAttrs::AttrType)::strings
                 U ___cxa_allocate_exception
                 U ___cxa_free_exception
                 U _task_get_exception_ports
                 U _task_set_exception_ports

Maybe it's a problem with your toolchain?

mofeing · 2025-02-07T15:42:02Z

Maybe it's a problem with your toolchain?

probably, just found out that it links to /usr/lib/libc++.1.dylib but... there is no such file 🫠

this would explain the "mixing between libstdc++ and libc++": there's no such mix, it's just that in my local build, it expects libc++ to provide that symbol and can't link it because it doesn't find it.

i was building with Homebrew's LLVM 19, but trying again with LLVM 16.

giordano · 2025-02-07T15:44:31Z

probably, just found out that it links to /usr/lib/libc++.1.dylib but... there is no such file 🫠

That's normal, macOS hides system libraries and header files from the filesystem: https://en.wikipedia.org/wiki/System_Integrity_Protection

giordano · 2025-02-07T15:59:09Z

Also: #164 (review)

As mentioned already on Slack, the changes to ReactantExtra should be done in a separate PR, to be merged before this one, to allow a build on Yggdrasil

☝️

mofeing · 2025-02-08T11:25:29Z

As mentioned already on Slack, the changes to ReactantExtra should be done in a separate PR, to be merged before this one, to allow a build on Yggdrasil

i'm leaving this PR just for the changes in ReactantExtra and gonna move the Julia part of the bindings to a new PR

Maybe it's a problem with your toolchain?

it seems like so. using --cc gcc --gcc_host_compiler_path gcc makes the error disappear. still trying to understand what happened these last months because it worked before, but it could be on XLA's side and not on ours.

wsmoses · 2025-02-08T15:00:34Z

deps/ReactantExtra/src/memory_management.cpp

+
+template<>
+void* capture_shared(std::shared_ptr<void> ptr) {
+    captured_shared_ptr[ptr.get()] = ptr;


we should use the new wrapper type as we talked about earlier (the global can work, but will cause concurrency issues that won't happen with the wrapper)

sure, using the new wrapper type solves concurrency issues but AFAIK the logic in Reactant.jl works on single thread as of today right?

since this seems to work as of now, i wanted to get this merged first and then switch to the new holder types on the next PR.

no, Reactant.jl logic isn't limited to a single thread (and user code can allocate in multiple threads)

okay, then let me prototype sth

…s,layouts + hlomodules

* rcref wrapper + fixed some compile errors * fixed compile errors

…en `T` is const

Co-authored-by: Mosè Giordano <[email protected]>

* replace capture maps for `Holded` wrapper * Remove commented code * Write destructors for concrete sharding classes

This comment was marked as off-topic.

Sign in to view

mofeing force-pushed the ss/ifrt branch from 7a96406 to 18d7c98 Compare December 30, 2024 23:14

mofeing changed the title ~~Write Bazel rule for IFRT dialect~~ Support IFRT Dec 30, 2024

mofeing force-pushed the ss/ifrt branch from 18d7c98 to 18ab156 Compare December 31, 2024 09:32

mofeing force-pushed the ss/ifrt branch 2 times, most recently from 93f73a3 to c00ac05 Compare January 20, 2025 23:35

ftynse mentioned this pull request Jan 24, 2025

Meta Tracker PRONTOLab/GB-25#1

Open

6 tasks

mofeing marked this pull request as ready for review February 2, 2025 19:10

mofeing requested a review from wsmoses February 2, 2025 19:14

mofeing commented Feb 2, 2025

View reviewed changes

mofeing force-pushed the ss/ifrt branch from 0bba36d to 1fd4cc7 Compare February 7, 2025 11:01

giordano reviewed Feb 7, 2025

View reviewed changes

deps/ReactantExtra/src/error_handling.hpp Outdated Show resolved Hide resolved

deps/ReactantExtra/src/ifrt/Value.cpp Outdated Show resolved Hide resolved

giordano reviewed Feb 7, 2025

View reviewed changes

deps/ReactantExtra/src/error_handling.hpp Outdated Show resolved Hide resolved

mofeing force-pushed the ss/ifrt branch from 4baccd7 to 04bfc1f Compare February 8, 2025 11:15

mofeing changed the title ~~Support IFRT~~ C-API bindings for IFRT Feb 8, 2025

wsmoses reviewed Feb 8, 2025

View reviewed changes

mofeing and others added 26 commits February 8, 2025 18:22

implement LoadedExecutable::Execute

2b8ff22

fix type conversion of LoadedExecutable parameter,output x sharding…

3d9d836

…s,layouts + hlomodules

remove template matching

947be36

Wrapping RCReferences (#697)

914c23d

* rcref wrapper + fixed some compile errors * fixed compile errors

Fix LoadedExecutable::Execute call

ba31e43

Fix conversion from std::vector<std::shared_ptr<T>> to span<T> wh…

8d619b7

…en `T` is const

Fix LoadedExecutable::GetHloModules

12009e6

last fixes

0350468

Finish PjRtArray API

9050c63

Implement LoadedExecutable::GetOutputMemoryKinds

1fd80b6

Implement missing methods of Executable

ca40a4d

Fix Client::GetReadyFuture, Client::MakeTuple methods

048ab01

Refactor Topology::DeviceDescriptions to span

6f2a29c

Remove DeviceList from RCRef

f671fbd

Add comment

e98e010

Move "API.cpp" to where it was

810bb0c

Replace call to legacy cstr_from_string for reactant::convert

0026b52

added missing definitions

e0082bc

Update deps/ReactantExtra/src/ifrt/Value.cpp

70a9062

Co-authored-by: Mosè Giordano <[email protected]>

Update deps/ReactantExtra/src/error_handling.hpp

1f2ee15

Co-authored-by: Mosè Giordano <[email protected]>

Update deps/ReactantExtra/src/error_handling.hpp

360026d

Co-authored-by: Mosè Giordano <[email protected]>

Remove Julia-side of the bindings

697b122

Remove change in XLA.jl

4ca7d26

Implement OpaqueSharding, ConcreteSharding, ConcreteEvenSharding

4e07d5f

fixes

af4437b

fix memory management bugs

f043a75

mofeing force-pushed the ss/ifrt branch from 349cbcf to f043a75 Compare February 9, 2025 00:22

Replace capture maps with Holded wrapper (#715)

045ed0e

* replace capture maps for `Holded` wrapper * Remove commented code * Write destructors for concrete sharding classes

mofeing marked this pull request as draft February 9, 2025 12:06

mofeing added the IFRT label Feb 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C-API bindings for IFRT #164

C-API bindings for IFRT #164

mofeing commented Oct 4, 2024 •

edited

Loading

This comment was marked as off-topic.

This comment was marked as off-topic.

mofeing commented Jan 2, 2025

mofeing commented Jan 22, 2025

ftynse commented Jan 24, 2025

mofeing left a comment

giordano left a comment •

edited

Loading

giordano Feb 7, 2025

mofeing Feb 7, 2025

giordano Feb 7, 2025

mofeing Feb 7, 2025

giordano Feb 7, 2025

giordano commented Feb 7, 2025

mofeing commented Feb 7, 2025

giordano commented Feb 7, 2025 •

edited

Loading

mofeing commented Feb 7, 2025 •

edited

Loading

giordano commented Feb 7, 2025

mofeing commented Feb 7, 2025

giordano commented Feb 7, 2025

giordano commented Feb 7, 2025 •

edited

Loading

mofeing commented Feb 8, 2025

wsmoses Feb 8, 2025

mofeing Feb 8, 2025

wsmoses Feb 8, 2025

mofeing Feb 8, 2025

C-API bindings for IFRT #164

Are you sure you want to change the base?

C-API bindings for IFRT #164

Conversation

mofeing commented Oct 4, 2024 • edited Loading

This comment was marked as off-topic.

This comment was marked as off-topic.

mofeing commented Jan 2, 2025

mofeing commented Jan 22, 2025

ftynse commented Jan 24, 2025

mofeing left a comment

Choose a reason for hiding this comment

giordano left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giordano commented Feb 7, 2025

mofeing commented Feb 7, 2025

giordano commented Feb 7, 2025 • edited Loading

mofeing commented Feb 7, 2025 • edited Loading

giordano commented Feb 7, 2025

mofeing commented Feb 7, 2025

giordano commented Feb 7, 2025

giordano commented Feb 7, 2025 • edited Loading

mofeing commented Feb 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mofeing commented Oct 4, 2024 •

edited

Loading

giordano left a comment •

edited

Loading

giordano commented Feb 7, 2025 •

edited

Loading

mofeing commented Feb 7, 2025 •

edited

Loading

giordano commented Feb 7, 2025 •

edited

Loading