Forward keyword options to Python runtime endpoints - #5229
Conversation
Signed-off-by: Andres Paz <andresp@nvidia.com>
CI Summary (
|
| Job | Result |
|---|---|
binaries |
⏩ skipped |
build_and_test |
✅ success |
changes |
✅ success |
config_devdeps |
✅ success |
config_source_build |
⏩ skipped |
config_wheeldeps |
✅ success |
devdeps |
✅ success |
docker_image |
⏩ skipped |
gen_code_coverage |
⏩ skipped |
metadata |
✅ success |
python_metapackages |
⏩ skipped |
python_wheels |
⏩ skipped |
source_build |
⏩ skipped |
wheeldeps |
✅ success |
⏩ Skipped jobs (7) — intentionally skipped on PR builds; run on merge_group / workflow_dispatch
| Job |
|---|
binaries |
config_source_build |
docker_image |
gen_code_coverage |
python_metapackages |
python_wheels |
source_build |
All sub-jobs (43) — every matrix leg, with links
| Job | Status | Link |
|---|---|---|
| Build and test (amd64, gcc12, openmpi) / Dev environment (Debug) | ✅ success | view |
| Build and test (amd64, gcc12, openmpi) / Dev environment (Python) | ✅ success | view |
| Build and test (amd64, llvm, openmpi) / Dev environment (Debug) | ✅ success | view |
| Build and test (amd64, llvm, openmpi) / Dev environment (Python) | ✅ success | view |
| Build and test (arm64, llvm, openmpi) / Dev environment (Debug) | ✅ success | view |
| Build and test (arm64, llvm, openmpi) / Dev environment (Python) | ✅ success | view |
| CI Summary | ❔ in_progress | view |
| Check for stable CUDA-Q changes | ✅ success | view |
| Configure build (devdeps) | ✅ success | view |
| Configure build (source_build) | ⏩ skipped | view |
| Configure build (wheeldeps) | ✅ success | view |
| Create CUDA Quantum installer | ⏩ skipped | view |
| Create Docker images | ⏩ skipped | view |
| Create Python metapackages | ⏩ skipped | view |
| Create Python wheels | ⏩ skipped | view |
| Gen code coverage | ⏩ skipped | view |
| Load dependencies (amd64, gcc12) / Caching | ✅ success | view |
| Load dependencies (amd64, gcc12) / Finalize | ✅ success | view |
| Load dependencies (amd64, gcc12) / Metadata | ✅ success | view |
| Load dependencies (amd64, llvm) / Caching | ✅ success | view |
| Load dependencies (amd64, llvm) / Finalize | ✅ success | view |
| Load dependencies (amd64, llvm) / Metadata | ✅ success | view |
| Load dependencies (arm64, gcc12) / Caching | ✅ success | view |
| Load dependencies (arm64, gcc12) / Finalize | ✅ success | view |
| Load dependencies (arm64, gcc12) / Metadata | ✅ success | view |
| Load dependencies (arm64, llvm) / Caching | ✅ success | view |
| Load dependencies (arm64, llvm) / Finalize | ✅ success | view |
| Load dependencies (arm64, llvm) / Metadata | ✅ success | view |
| Load source build cache | ⏩ skipped | view |
| Load wheel dependencies (amd64, 12.6) / Caching | ✅ success | view |
| Load wheel dependencies (amd64, 12.6) / Finalize | ✅ success | view |
| Load wheel dependencies (amd64, 12.6) / Metadata | ✅ success | view |
| Load wheel dependencies (amd64, 13.0) / Caching | ✅ success | view |
| Load wheel dependencies (amd64, 13.0) / Finalize | ✅ success | view |
| Load wheel dependencies (amd64, 13.0) / Metadata | ✅ success | view |
| Load wheel dependencies (arm64, 12.6) / Caching | ✅ success | view |
| Load wheel dependencies (arm64, 12.6) / Finalize | ✅ success | view |
| Load wheel dependencies (arm64, 12.6) / Metadata | ✅ success | view |
| Load wheel dependencies (arm64, 13.0) / Caching | ✅ success | view |
| Load wheel dependencies (arm64, 13.0) / Finalize | ✅ success | view |
| Load wheel dependencies (arm64, 13.0) / Metadata | ✅ success | view |
| Prepare cache clean-up | ❔ in_progress | view |
| Retrieve PR info | ✅ success | view |
✅ Required checks (6/6) — declared in .github/required-checks.yml for push
| Required check | Status | Link |
|---|---|---|
| Build and test (amd64, llvm, openmpi) / Dev environment (Debug) | ✅ success | view |
| Build and test (amd64, llvm, openmpi) / Dev environment (Python) | ✅ success | view |
| Build and test (arm64, llvm, openmpi) / Dev environment (Debug) | ✅ success | view |
| Build and test (arm64, llvm, openmpi) / Dev environment (Python) | ✅ success | view |
| Build and test (amd64, gcc12, openmpi) / Dev environment (Debug) | ✅ success | view |
| Build and test (amd64, gcc12, openmpi) / Dev environment (Python) | ✅ success | view |
Signed-off-by: Andres Paz <andresp@nvidia.com>
There was a problem hiding this comment.
Thanks for the work! Great API idea.
My main issue is that by introducing endpoint-specific options, we are coupling the launch call site with the runtime endpoint that is set. If the user relies on an endpoint-specific option and then changes the runtime endpoint, the option will be silently ignored and lead to surprising behaviour for the user.
The other issue is that by using nb::dict, this mechanism can only work for Python-defined endpoints. We shouldn't introduce behaviour that we cannot support in C++ down the line.
Given that we are tight on time, my suggestion would be to mitigate as much as possible the issues above, for example as follows:
- Add an optional
keywordsclass attribute to the RuntimeEndpoint python protocol. It's type would be an iterable of strings. If none is provided, defaults to empty. - Instead of storing a fully opaque endpoint_options type, store it as a std::unordered_map. The values can be fully opaque for now.
- When the runtime is about to call the python runtime endpoint, check that only valid options are provided.
- Similarly, for C++ endpoints, an exception should be thrown if any option is set (as currently none are supported). Ideally, this check and the check for the previous bullet would be one and the same.
Does that make sense?
| @@ -0,0 +1,21 @@ | |||
| /****************************************************************-*- C++ -*-**** | |||
There was a problem hiding this comment.
You don't need this additional struct. You can just forward declare PythonEndpointOptions in runtime/cudaq/algorithms/observe/policy.h.
There was a problem hiding this comment.
That being said, with the suggestion I'm making above, having some type like this to wrap the opaque values will be useful. Maybe something like
struct EndpointOptionValue {
std::any value
};This type should live in the detail namespace.
KernelArgsalready marshals positional kernel arguments into Python runtime endpoint calls. This change also forwards keyword options.Each launch policy continues to provide the CUDA-Q options it owns, while endpoint-specific keyword options are retained as an opaque payload and merged into the endpoint invocation. This lets endpoints receive options such as resource-estimation configuration without CUDA-Q needing to interpret or serialize them.
Includes coverage confirming
cudaq.estimate()forwards bothtierand arbitrary object-valued endpoint options unchanged.