-
Notifications
You must be signed in to change notification settings - Fork 323
feat(config): add bool type support for target arguments #3740
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?
feat(config): add bool type support for target arguments #3740
Conversation
|
Hi @AndrewTKent, The feature proposed in #3525 is broader than this. In particular, the cuda-quantum/tools/nvqpp/nvq++.in Lines 443 to 448 in 48cf3d9
Hence, this would require some additional considerations for handling the Would you be able to look at the use case described in #3525 as well? |
|
Thanks for the feedback! You're right - the CLI side needs to know which flags are Bool type at parse time to handle Would parsing the YAML to extract argument types before the main arg loop be the right approach? Or is there a simpler pattern you'd prefer? |
Yes, I think "parsing the YAML to extract argument types" is a good approach. |
77dc115 to
f4bd9b4
Compare
|
@1tnguyen Ready for another look — implemented the YAML parsing approach you suggested. The compiler now calls Bool flags now work naturally: nvq++ --target ionq --ionq-debias program.cpp # implicit true |
khalatepradnya
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.
Thank you for the contribution!
runtime/common/ServerHelper.cpp
Outdated
| ServerHelper::getArgumentType(const std::string &key) const { | ||
| for (const auto &arg : runtimeTarget.config.TargetArguments) { | ||
| // Check both the key name and platform-arg key | ||
| if (arg.KeyName == key || arg.PlatformArgKey == key) { |
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.
nit: style - you can drop the braces here as per https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
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.
This can be applied at multiple places in this PR.
| type: string | ||
| platform-arg: debias | ||
| help-string: "Specify debiasing." | ||
| type: bool |
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.
Q: Is this a breaking change for the users of ionq target?
f4bd9b4 to
99a99d2
Compare
|
@1tnguyen Re: @khalatepradnya Re: breaking change for ionq users — No, this is backwards compatible:
The only behavioral change is that invalid values now throw a clear error instead of being silently passed to the API. This is actually an improvement — previously Updates in this push:
|
99a99d2 to
b3b15e1
Compare
Adds `Bool` to the `ArgumentType` enum for target configuration, enabling
boolean flags that can be specified without a value.
## Changes
### Core Type Support
- Add `Bool` to `ArgumentType` enum in TargetConfig.h
- Add YAML mapping "bool" -> ArgumentType::Bool in TargetConfig.cpp
### Typed Value Utilities
- Add `getTypedConfigValue()` to ServerHelper for proper JSON serialization
- Add `getArgumentType()` to look up declared types from target config
- Bool values serialize as JSON boolean literals (not strings)
### nvq++ Compiler Support
- Add `--list-bool-args` mode to cudaq-target-conf tool
- Modify nvq++ to detect Bool arguments and handle shift 1 vs shift 2
- Bool args can be specified without value (defaults to true)
### Backend Updates
- Update IonQ backend: change `debias` and `sharpen` to `type: bool`
- Simplify backend code using new typed value utilities
### Tests
- Add unit test for Bool argument YAML parsing
- Add integration test for --list-bool-args output
- Add test YAML config with Bool/String/Int arguments
## Usage
Target YAML:
```yaml
target-arguments:
- key: debias
type: bool
platform-arg: debias
```
Command line:
```bash
nvq++ --target ionq --ionq-debias program.cpp # defaults to true
nvq++ --target ionq --ionq-debias true program.cpp # explicit true
nvq++ --target ionq --ionq-debias=false program.cpp # explicit false
```
Fixes NVIDIA#3699
Fixes NVIDIA#3525
Signed-off-by: andrewtkent <[email protected]>
b3b15e1 to
822c1a1
Compare
Command Bot: Processing... |
|
For the 'Check code formatting' failure in CI, please run the clang formatter. (See: https://github.com/NVIDIA/cuda-quantum/blob/main/Developing.md#c-formatting). |
| // IMPLICIT: -DNVQPP_TARGET_BACKEND_CONFIG | ||
| // IMPLICIT-SAME: debias;true | ||
|
|
||
| // EXPLICIT: -DNVQPP_TARGET_BACKEND_CONFIG | ||
| // EXPLICIT-SAME: debias;true | ||
|
|
||
| // FALSE: -DNVQPP_TARGET_BACKEND_CONFIG | ||
| // FALSE-SAME: debias;false |
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.
Thanks! This is a good test :)
nit: Could you please move these FileCheck lines to the bottom of the file, to be consistent with other test files?
|
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
Signed-off-by: andrewtkent <[email protected]>
5bde1a9 to
54af656
Compare
Command Bot: Processing... |
|
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
|
@AndrewTKent Could you please take a look at this build failure: https://github.com/NVIDIA/cuda-quantum/actions/runs/21050052114/job/60534780678?pr=3740#step:8:1147? |
Remove redundant default label from switch statement that already covers all ArgumentType enum values. This fixes the build error: "default label in switch which covers all enumeration values" (-Werror,-Wcovered-switch-default) Signed-off-by: AndrewTKent <[email protected]>
Command Bot: Processing... |
|
@AndrewTKent : Thank you for the fix, but I think it has led to a different failure. Please see: https://github.com/NVIDIA/cuda-quantum/actions/runs/21182285081/job/60928809695?pr=3740#step:8:1165 |
64f9668 to
eeecab5
Compare
|
Took a stab at fixing the build failure. The issue appears to be GCC's Restructured the control flow to use an explicit if (value == "true" || ...)
return true;
else if (value == "false" || ...)
return false;
else
throw std::runtime_error(...);Also squashed the commits. Let's see if CI is happier now. |
Command Bot: Processing... |
runtime/common/ServerHelper.cpp
Outdated
| case config::ArgumentType::UUID: | ||
| case config::ArgumentType::FeatureFlag: | ||
| case config::ArgumentType::MachineConfig: | ||
| default: |
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.
|
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
9d451f1 to
9362a2a
Compare
Overview
Adds first-class
Booltype support for target arguments, enabling boolean flags to be specified without explicit values on the command line.Fixes #3699 · Fixes #3525
Problem
Target arguments declared as
type: boolin YAML configs were treated identically to strings at every layer:shift 2), breaking flag-style usage"true"/"false"as strings, requiring manual parsing in each backend"true"(string) instead oftrue(boolean literal)Solution
1. Compiler Driver (
nvq++.in)Added Bool argument detection so flags can omit values:
Implementation: New
--list-bool-argsmode incudaq-target-confoutputs Bool keys, whichnvq++queries at argument parse time to decideshift 1vsshift 2.2. Type System (
TargetConfig.h)3. Runtime Utilities (
ServerHelper)4. Backend Migration (IonQ)
Updated
debiasandsharpenfromtype: stringtotype: bool:Files Changed
TargetConfig.h,TargetConfig.cppnvq++.in,cudaq-target-conf.cppServerHelper.h,ServerHelper.cppIonQServerHelper.cpp,ionq.ymlTargetConfigTester.cpp,check_bool_args.configTesting
--list-bool-argsoutput validationUsage
Target YAML:
Command line:
Backend receives:
{ "debias": true }