Conversation
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
This PR implements a workaround for an upstream issue in llama.cpp (issue #8665) related to SYCL library linking. The changes conditionally link the SYCL library when building with Intel OneAPI, and update the llama.cpp dependency version.
- Adds conditional SYCL linking based on ONEAPI_ROOT environment variable
- Creates a workaround CMakeLists.txt file for the RPC server component
- Updates llama.cpp version dependency
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| backend/cpp/llama/prepare.sh | Adds script to copy workaround CMakeLists.txt for RPC server to address upstream issue |
| backend/cpp/llama/CMakeLists.txt.rpc-8662 | New workaround CMakeLists.txt for RPC server with conditional SYCL linking |
| backend/cpp/llama/CMakeLists.txt | Implements conditional SYCL linking for grpc-server target based on ONEAPI_ROOT |
| Makefile | Updates CPPLLAMA_VERSION to newer commit hash |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,8 @@ | |||
| # https://github.com/ggerganov/llama.cpp/issues/8665 | |||
There was a problem hiding this comment.
The issue reference points to #8665, but the filename and comment reference issue #8662. This inconsistency could cause confusion. Ensure the correct issue number is used consistently throughout the codebase.
| # https://github.com/ggerganov/llama.cpp/issues/8665 | |
| # https://github.com/ggerganov/llama.cpp/issues/8662 |
| if ( DEFINED ENV{ONEAPI_ROOT}) | ||
| target_link_libraries(${TARGET} PRIVATE common llama myclip ${CMAKE_THREAD_LIBS_INIT} absl::flags hw_grpc_proto | ||
| absl::flags_parse | ||
| gRPC::${_REFLECTION} | ||
| gRPC::${_GRPC_GRPCPP} | ||
| protobuf::${_PROTOBUF_LIBPROTOBUF} | ||
| sycl) | ||
| else() | ||
| target_link_libraries(${TARGET} PRIVATE common llama myclip ${CMAKE_THREAD_LIBS_INIT} absl::flags hw_grpc_proto | ||
| absl::flags_parse | ||
| gRPC::${_REFLECTION} | ||
| gRPC::${_GRPC_GRPCPP} | ||
| protobuf::${_PROTOBUF_LIBPROTOBUF}) | ||
| endif() | ||
|
|
There was a problem hiding this comment.
The code in both branches of the if/else statement is nearly identical, with only the sycl library addition being different. This duplication makes maintenance harder. Consider extracting the common libraries to a variable and conditionally appending sycl to avoid repeating the entire target_link_libraries call.
| if ( DEFINED ENV{ONEAPI_ROOT}) | |
| target_link_libraries(${TARGET} PRIVATE common llama myclip ${CMAKE_THREAD_LIBS_INIT} absl::flags hw_grpc_proto | |
| absl::flags_parse | |
| gRPC::${_REFLECTION} | |
| gRPC::${_GRPC_GRPCPP} | |
| protobuf::${_PROTOBUF_LIBPROTOBUF} | |
| sycl) | |
| else() | |
| target_link_libraries(${TARGET} PRIVATE common llama myclip ${CMAKE_THREAD_LIBS_INIT} absl::flags hw_grpc_proto | |
| absl::flags_parse | |
| gRPC::${_REFLECTION} | |
| gRPC::${_GRPC_GRPCPP} | |
| protobuf::${_PROTOBUF_LIBPROTOBUF}) | |
| endif() | |
| set(GRPC_SERVER_LIBS | |
| common | |
| llama | |
| myclip | |
| ${CMAKE_THREAD_LIBS_INIT} | |
| absl::flags | |
| hw_grpc_proto | |
| absl::flags_parse | |
| gRPC::${_REFLECTION} | |
| gRPC::${_GRPC_GRPCPP} | |
| protobuf::${_PROTOBUF_LIBPROTOBUF} | |
| ) | |
| if (DEFINED ENV{ONEAPI_ROOT}) | |
| list(APPEND GRPC_SERVER_LIBS sycl) | |
| endif() | |
| target_link_libraries(${TARGET} PRIVATE ${GRPC_SERVER_LIBS}) |
Description
This PR fixes #
Notes for Reviewers
Signed commits