fix: nanobind build due to undeclared CommType #6149
Conversation
Signed-off-by: Linda-Stadter <57756729+Linda-Stadter@users.noreply.github.com>
WalkthroughThe changes update Python bindings for cache transceiver configuration and buffer management. The Changes
Sequence Diagram(s)sequenceDiagram
participant Python as Python User
participant NB as Nanobind Bindings
participant Cpp as C++ CacheTransceiverConfig
Python->>NB: Create CacheTransceiverConfig(backend, max_tokens_in_buffer)
NB->>Cpp: Construct with backend, max_tokens_in_buffer
Cpp-->>NB: Instance
NB-->>Python: Python object
Python->>NB: Use CacheTransceiverBackendType enum
NB-->>Python: Enum values (DEFAULT, MPI, UCX, NIXL)
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🧰 Additional context used🧠 Learnings (1)cpp/tensorrt_llm/nanobind/executor/executorConfig.cpp (1)🔇 Additional comments (7)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
/bot run |
|
PR_Github #12218 [ run ] triggered by Bot |
|
PR_Github #12218 [ run ] completed with state |
|
/bot run |
|
PR_Github #12240 [ run ] triggered by Bot |
| config = trtllm.CacheTransceiverConfig(backend="UCX", | ||
| max_tokens_in_buffer=1024) | ||
| config = trtllm.CacheTransceiverConfig( | ||
| backend=trtllm.CacheTransceiverBackendType.UCX, |
There was a problem hiding this comment.
There are a lot of tests that use the string format, do we have something similar to "implicitly convertible" in Nanobind?
There was a problem hiding this comment.
Which tests do you mean? I only found this test and a test in kv_cache_transceiver. But the latter accesses the enum member directly and not through a string
There was a problem hiding this comment.
We have cache_transceiver_config.backend: <str> in tests/integration/defs/disaggregated/test_configs, the type of field of backend is string.
|
PR_Github #12240 [ run ] completed with state |
| .value("MPI", tle::CacheTransceiverConfig::BackendType::MPI) | ||
| .value("UCX", tle::CacheTransceiverConfig::BackendType::UCX) | ||
| .value("NIXL", tle::CacheTransceiverConfig::BackendType::NIXL) | ||
| .def("from_string", |
There was a problem hiding this comment.
It seems that nanobind
nb::class_<MyType>(m, "MyType")
.def(nb::init_implicit<MyOtherType>());
more suitable here.
There was a problem hiding this comment.
How would it work? Unfortunately, I don't see a way to define an init function for an enum with nanobind.
Fix nanobind build
A recent PR changed bindings for pybind that weren't applied to nanobind yet. This caused the nanobind build in the CI to fail.
This PR applies these changes to nanobind.
GitHub Bot Help
/bot [-h] ['run', 'kill', 'skip', 'reuse-pipeline'] ...Provide a user friendly way for developers to interact with a Jenkins server.
Run
/bot [-h|--help]to print this help message.See details below for each supported subcommand.
run [--disable-fail-fast --skip-test --stage-list "A10-1, xxx" --gpu-type "A30, H100_PCIe" --add-multi-gpu-test --only-multi-gpu-test --disable-multi-gpu-test --post-merge --extra-stage "H100_PCIe-[Post-Merge]-1, xxx"]Launch build/test pipelines. All previously running jobs will be killed.
--disable-fail-fast(OPTIONAL) : Disable fail fast on build/tests/infra failures.--skip-test(OPTIONAL) : Skip all test stages, but still run build stages, package stages and sanity check stages. Note: Does NOT update GitHub check status.--stage-list "A10-1, xxx"(OPTIONAL) : Only run the specified test stages. Examples: "A10-1, xxx". Note: Does NOT update GitHub check status.--gpu-type "A30, H100_PCIe"(OPTIONAL) : Only run the test stages on the specified GPU types. Examples: "A30, H100_PCIe". Note: Does NOT update GitHub check status.--only-multi-gpu-test(OPTIONAL) : Only run the multi-GPU tests. Note: Does NOT update GitHub check status.--disable-multi-gpu-test(OPTIONAL) : Disable the multi-GPU tests. Note: Does NOT update GitHub check status.--add-multi-gpu-test(OPTIONAL) : Force run the multi-GPU tests. Will also run L0 pre-merge pipeline.--post-merge(OPTIONAL) : Run the L0 post-merge pipeline instead of the ordinary L0 pre-merge pipeline.--extra-stage "H100_PCIe-[Post-Merge]-1, xxx"(OPTIONAL) : Run the ordinary L0 pre-merge pipeline and specified test stages. Examples: --extra-stage "H100_PCIe-[Post-Merge]-1, xxx".For guidance on mapping tests to stage names, see
docs/source/reference/ci-overview.md.kill
killKill all running builds associated with pull request.
skip
skip --comment COMMENTSkip testing for latest commit on pull request.
--comment "Reason for skipping build/test"is required. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.reuse-pipeline
reuse-pipelineReuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.
Summary by CodeRabbit
New Features
Bug Fixes
Tests