Document solver settings in the gRPC codegen field registry - #1701
Draft
ramakrishnap-nv wants to merge 6 commits into
Draft
Document solver settings in the gRPC codegen field registry#1701ramakrishnap-nv wants to merge 6 commits into
ramakrishnap-nv wants to merge 6 commits into
Conversation
Adds `description:` and `default:` attributes to field_registry.yaml and emits them as leading comments on the generated proto fields. The generated .proto is part of the public wire contract (GRPC_INTERFACE.md, "Custom Clients"), but carried no comments, so a third-party client could not learn what a settings field means or what omitting it does. Covers all 85 fields in pdlp_settings and mip_settings. Two fields (postsolve_info, presolve_absolute_tolerance) have no documentation anywhere, so they carry only `default:` plus a note to add prose later. `default:` values are taken from the C++ member initializers rather than from docs/cuopt/source/*-settings.rst, which disagrees in four places (barrier_iterative_refinement, barrier_step_scale, pdlp_precision, semi_continuous_big_m). Those look like docs bugs and are left for a separate change. Fields without either attribute generate byte-identical output, so the only generated file that changes is cuopt_remote_data.proto, and the change is purely additive comments. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
The skill had no coverage of cpp/src/grpc/codegen/, so an agent editing a wire field could hand-edit the generated output or skip regeneration. Codegen is an explicit ./build.sh target that never runs during a normal build, so a missed regeneration surfaces only as a CI failure. Adds references/grpc_codegen.md covering the registry workflow, the optional/sentinel presence traps, field-number permanence, and the rule that the C++ member initializer rather than docs/ is ground truth for a setting's default. Adds a safety rule and pointer section in SKILL.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
The rule only named cpp/src/grpc/codegen/generated/, which reads as if that were the only generated tree. Replaces it with a source -> generated -> regenerate table covering the dependency files, doc versions, and skill/plugin version sync, and calls out that gRPC codegen is the only one not fixed automatically by a pre-commit hook. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
ramakrishnap-nv
added
non-breaking
CI Test Summary⏭️ All 5 test job(s) skipped. |
📝 WalkthroughWalkthroughChangesThe gRPC field registry now includes descriptions and defaults for PDLP and MIP settings. Code generation emits this metadata as proto comments. Developer documentation describes the registry workflow, regeneration commands, field numbering, presence, and default handling. gRPC field documentation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cpp/src/grpc/codegen/field_registry.yaml (1)
41-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the effect of
absolute_tolerance.The documented defaults match the C++ initializers. The
absolute_tolerancedescription still does not explain which feasibility checks use this value. Replace it with behavior-specific documentation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/grpc/codegen/field_registry.yaml` around lines 41 - 51, Update the absolute_tolerance entry in the field registry so its description explains which feasibility checks use this tolerance value, replacing the current generic wording while preserving the existing default and documentation-only metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/grpc/codegen/field_registry.yaml`:
- Around line 797-801: Update the absolute_tolerance field description in the
field registry to explain that it is used for constraint-feasibility checks, and
distinguish it from absolute_mip_gap, which controls solve termination. Preserve
the existing default, field number, and optionality.
In `@cpp/src/grpc/codegen/generate_conversions.py`:
- Around line 337-374: Add unit tests covering _field_doc_comment and its
integration with generated declarations: verify empty metadata returns no
comments, default-only metadata renders correctly, long whitespace-normalized
text wraps at the configured width, and comments appear immediately before both
optional and non-optional field declarations. Ensure existing undocumented-field
output remains unchanged.
In `@skills/cuopt-developer/SKILL.md`:
- Around line 173-176: Update the generated-output table in the documentation to
list both field_registry.yaml and generate_conversions.py as sources for
cpp/src/grpc/codegen/generated/. Preserve the existing regeneration command and
table structure.
---
Nitpick comments:
In `@cpp/src/grpc/codegen/field_registry.yaml`:
- Around line 41-51: Update the absolute_tolerance entry in the field registry
so its description explains which feasibility checks use this tolerance value,
replacing the current generic wording while preserving the existing default and
documentation-only metadata.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e6d40056-a064-441e-bf29-d8b57efc1661
⛔ Files ignored due to path filters (1)
cpp/src/grpc/codegen/generated/cuopt_remote_data.protois excluded by!**/generated/**
📒 Files selected for processing (4)
cpp/src/grpc/codegen/field_registry.yamlcpp/src/grpc/codegen/generate_conversions.pyskills/cuopt-developer/SKILL.mdskills/cuopt-developer/references/grpc_codegen.md
Comment on lines
797
to
801
| - absolute_tolerance: | ||
| description: MIP absolute tolerance. | ||
| default: "1e-6" | ||
| field_num: 5 | ||
| optional: true |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C4 '\babsolute_tolerance\b|\brelative_tolerance\b' \
cpp/include/cuopt/mathematical_optimization/mip/solver_settings.hpp \
cpp/src/math_optimization/solver_settings.cu \
docs/cuopt/source/mip-settings.rstRepository: NVIDIA/cuopt
Length of output: 3038
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- registry context ---'
sed -n '780,810p' cpp/src/grpc/codegen/field_registry.yaml
printf '%s\n' '--- MIP tolerance documentation ---'
rg -n -C6 'absolute tolerance|absolute_tolerance|relative tolerance|relative_tolerance|MIP.*tolerance|tolerance.*MIP' docs cpp/src/grpc cpp/include/cuopt/mathematical_optimization/mip
printf '%s\n' '--- registry generator handling ---'
rg -n -C4 'field_registry|description:|default:' cpp/src/grpc/codegen --glob '!field_registry.yaml'Repository: NVIDIA/cuopt
Length of output: 50370
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- all absolute_tolerance uses ---'
rg -n -C3 '\babsolute_tolerance\b|CUOPT_MIP_ABSOLUTE_TOLERANCE' \
cpp python docs --glob '!cpp/src/grpc/codegen/generated/*' \
| head -n 240
printf '%s\n' '--- tolerance-related solver logic ---'
rg -n -C4 'tolerance|Tolerance' cpp/src/math_optimization cpp/include/cuopt/mathematical_optimization/mip \
| head -n 320
printf '%s\n' '--- generated proto comment helper tests ---'
rg -n -C5 '_field_doc_comment|MIP absolute tolerance|absolute gap at which|proto leading comment' \
cpp/src/grpc --glob '*.py' --glob '*test*' --glob '*.md'Repository: NVIDIA/cuopt
Length of output: 33522
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- constraint tolerance semantics ---'
rg -n -C8 'get_cstr_tolerance|check_feasibility|absolute_tolerance' \
cpp/src/mip_heuristics/feasibility_jump \
cpp/src/mip_heuristics/solution/feasibility_test.cuh \
cpp/src/mip_heuristics/relaxed_lp/relaxed_lp.cu \
| head -n 260
printf '%s\n' '--- read-only semantic verifier ---'
python3 - <<'PY'
from pathlib import Path
registry = Path("cpp/src/grpc/codegen/field_registry.yaml").read_text()
proto = Path("cpp/src/grpc/codegen/generated/cuopt_remote_data.proto").read_text()
docs = Path("docs/cuopt/source/mip-settings.rst").read_text()
feasibility = Path("cpp/src/mip_heuristics/solution/feasibility_test.cuh").read_text()
relaxed_lp = Path("cpp/src/mip_heuristics/relaxed_lp/relaxed_lp.cu").read_text()
checks = {
"registry description is generic": "description: MIP absolute tolerance." in registry,
"generated proto description is generic": "// MIP absolute tolerance. (default: 1e-6)" in proto,
"public docs are also generic": "controls the MIP absolute tolerance." in docs,
"constraint feasibility uses absolute tolerance": "constraint_lower_bounds[c] - abs_tolerance" in feasibility,
"relaxed LP uses absolute tolerance": "op_problem.tolerances.absolute_tolerance" in relaxed_lp,
"absolute gap docs describe termination separately": "controls the absolute tolerance used to terminate the MIP solve." in docs,
}
for name, passed in checks.items():
print(f"{name}: {'yes' if passed else 'no'}")
if not all(checks.values()):
raise SystemExit("semantic evidence is incomplete")
PYRepository: NVIDIA/cuopt
Length of output: 15416
Document the behavior of absolute_tolerance.
MIP absolute tolerance is insufficient for proto-only clients. Describe its use in constraint-feasibility checks and distinguish it from absolute_mip_gap, which controls solve termination.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/grpc/codegen/field_registry.yaml` around lines 797 - 801, Update the
absolute_tolerance field description in the field registry to explain that it is
used for constraint-feasibility checks, and distinguish it from
absolute_mip_gap, which controls solve termination. Preserve the existing
default, field number, and optionality.
Comment on lines
+337
to
+374
| _DOC_COMMENT_WIDTH = 78 | ||
|
|
||
|
|
||
| def _field_doc_comment(f, indent=" "): | ||
| """Render a field's `description:` / `default:` registry attributes as | ||
| proto leading comment lines. | ||
|
|
||
| The generated proto is part of the public wire contract (see | ||
| GRPC_INTERFACE.md, "Custom Clients"), so a third-party client reading | ||
| only the .proto should learn what a settings field means and what it | ||
| does when omitted. Returns [] when the field carries neither | ||
| attribute, so undocumented fields emit exactly as before. | ||
|
|
||
| `default:` is rendered verbatim from the registry — it is a string | ||
| describing the C++ member initializer, not a value the generator | ||
| derives or validates. | ||
| """ | ||
| description = f.get("description") | ||
| default = f.get("default") | ||
| if not description and default is None: | ||
| return [] | ||
| body = " ".join(str(description).split()) if description else "" | ||
| if default is not None: | ||
| suffix = f"(default: {default})" | ||
| body = f"{body} {suffix}" if body else suffix | ||
| prefix = f"{indent}// " | ||
| width = max(_DOC_COMMENT_WIDTH - len(prefix), 20) | ||
| lines, current = [], "" | ||
| for word in body.split(): | ||
| candidate = f"{current} {word}" if current else word | ||
| if current and len(candidate) > width: | ||
| lines.append(f"{prefix}{current}") | ||
| current = word | ||
| else: | ||
| current = candidate | ||
| if current: | ||
| lines.append(f"{prefix}{current}") | ||
| return lines |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add unit tests for generated documentation comments.
This feature has no unit test in the supplied changes. Test empty metadata, a default without description, whitespace wrapping, and placement before optional and non-optional declarations.
As per coding guidelines, “Contributions implementing features or bug fixes must include unit tests.”
Also applies to: 1307-1309
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/grpc/codegen/generate_conversions.py` around lines 337 - 374, Add
unit tests covering _field_doc_comment and its integration with generated
declarations: verify empty metadata returns no comments, default-only metadata
renders correctly, long whitespace-normalized text wraps at the configured
width, and comments appear immediately before both optional and non-optional
field declarations. Ensure existing undocumented-field output remains unchanged.
Source: Coding guidelines
Comment on lines
+173
to
+176
| | Generated | Source | Regenerate with | | ||
| |-----------|--------|-----------------| | ||
| | `cpp/src/grpc/codegen/generated/` | `cpp/src/grpc/codegen/field_registry.yaml` | `./build.sh codegen` | | ||
| | `conda/environments/*.yaml`, `pyproject.toml` | `dependencies.yaml` | `pre-commit run --all-files` | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
List generate_conversions.py as a generated-output source.
cpp/src/grpc/codegen/generated/ depends on both field_registry.yaml and generate_conversions.py. If the generator changes, the current table identifies only the registry as the source.
Proposed correction
-| `cpp/src/grpc/codegen/generated/` | `cpp/src/grpc/codegen/field_registry.yaml` | `./build.sh codegen` |
+| `cpp/src/grpc/codegen/generated/` | `cpp/src/grpc/codegen/field_registry.yaml`, `cpp/src/grpc/codegen/generate_conversions.py` | `./build.sh codegen` |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | Generated | Source | Regenerate with | | |
| |-----------|--------|-----------------| | |
| | `cpp/src/grpc/codegen/generated/` | `cpp/src/grpc/codegen/field_registry.yaml` | `./build.sh codegen` | | |
| | `conda/environments/*.yaml`, `pyproject.toml` | `dependencies.yaml` | `pre-commit run --all-files` | | |
| | Generated | Source | Regenerate with | | |
| |-----------|--------|-----------------| | |
| | `cpp/src/grpc/codegen/generated/` | `cpp/src/grpc/codegen/field_registry.yaml`, `cpp/src/grpc/codegen/generate_conversions.py` | `./build.sh codegen` | | |
| | `conda/environments/*.yaml`, `pyproject.toml` | `dependencies.yaml` | `pre-commit run --all-files` | |
🧰 Tools
🪛 SkillSpector (2.5.1)
[warning] 76: [EA2] Autonomous Decision Making: Skill enables autonomous high-impact decisions without human-in-the-loop verification. Critical operations (destructive commands, financial transactions, data deletion) should require explicit user confirmation.
Remediation: Add human-in-the-loop confirmation for destructive, irreversible, or high-impact operations. Never auto-execute commands that modify files, send data, or alter system state.
(Excessive Agency (EA2))
[warning] 272: [PE2] Sudo/Root Execution: Commands invoke sudo or root privileges. Verify this elevated access is necessary and justified.
Remediation: Avoid sudo/root unless strictly required. Prefer least-privilege patterns. If elevation is needed, document the justification and scope.
(Privilege Escalation (PE2))
[warning] 34: [RA2] Session Persistence: Skill establishes unauthorized persistence across sessions via cron jobs, startup scripts, or state files. Session persistence allows an attacker to maintain access beyond the current interaction.
Remediation: Remove any persistence mechanisms (cron jobs, startup scripts, state files). Skills should not maintain state across sessions without explicit user consent.
(Rogue Agent (RA2))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/cuopt-developer/SKILL.md` around lines 173 - 176, Update the
generated-output table in the documentation to list both field_registry.yaml and
generate_conversions.py as sources for cpp/src/grpc/codegen/generated/. Preserve
the existing regeneration command and table structure.
mlubin
reviewed
Aug 11, 2026
| // tolerance — and costs roughly 3-7% runtime and 10-20% memory. Dual | ||
| // simplex always detects infeasibility regardless of this setting. | ||
| // (default: false) | ||
| bool detect_infeasibility = 12; |
Contributor
There was a problem hiding this comment.
(pre-existing) Is the bool not optional in the proto just because the default is false? What happens if/when the default changes?
ramakrishnap-nv
marked this pull request as draft
August 11, 2026 18:21
Exposes cuOpt to MCP clients (Claude Code, Cursor, Codex) as a stdio
subprocess holding a gRPC channel to cuopt_grpc_server. The agent host
needs no GPU and no HTTP is involved.
Solves are asynchronous — submitting returns a job_id and nothing blocks,
because a blocking tools/call exceeds the client timeout on any realistic
MILP and makes cancellation impossible. Results are shaped rather than
streamed: the binding limit on a tool result is the model's context
window, not the transport, so cuopt_result returns a summary plus narrow
accessors and writes large solutions to a file.
The settings catalogue is generated. generate_conversions.py emits
cuopt_mcp_schema.json from field_registry.yaml as a 29th artifact, so a
new solver parameter reaches the server in the same commit as the proto
with no MCP-specific work, guarded by ci/verify_grpc_codegen.sh.
Two registry attributes were needed to make that schema usable:
* param_name — the proto field name is frequently not the CUOPT_*
string parameter (relative_mip_gap vs mip_relative_gap, mir_cuts vs
mip_mixed_integer_rounding_cuts). 44 fields diverge; without the
mapping every such setting failed at solve time with "Invalid
parameter". presolve_absolute_tolerance has no constant at all and is
marked null so it is not advertised.
* x-enum-values — set_parameter takes the integer for an enum setting,
but an agent needs the name, so the schema carries both.
test_parameter_names.py asserts every advertised setting exists in
constants.h, parsed from the in-repo header so the check matches the
registry rather than whatever cuOpt happens to be installed. It already
caught a missed mapping on the second presolver entry.
Verified end to end against a live cuopt_grpc_server on a GPU: an LP and
a knapsack MILP submitted, polled, and retrieved with named solutions
through the real MCP protocol over stdio.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
The description/default attributes were rendered into cuopt_remote_data.proto because they needed some consumer to exist at all — an attribute nothing emits is not covered by verify_grpc_codegen.sh and can rot unnoticed. cuopt_mcp_schema.json is now that consumer, and a better one: it is read by every MCP client rather than by a hypothetical third-party proto reader, and it is equally covered by verify_grpc_codegen.sh. The proto rendering no longer earns its 234 lines of churn on every prose edit, so it goes, along with the now-unused comment-wrapping helper. cuopt_remote_data.proto is byte-identical to main again. Also symlink python/cuopt_mcp/LICENSE to the repo root rather than copying it, matching cuopt_self_hosted and cuopt_server. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
cuopt_mcp imports cuopt lazily so the MCP handshake stays fast, which meant the dependency was undeclared and pip install succeeded while the first tool call failed. Adds run_cuopt_mcp plus build/run/test file entries so rapids-dependency-file-generator emits the CUDA-suffixed cuopt requirement into pyproject.toml. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Solver settings had no machine-readable documentation.
constants.his a bare list of#defines, anddocs/cuopt/source/*-settings.rstkeys prose to C API constant names rather than to fields — so nothing tied a setting's meaning and default to the field itself. This addsdescription:anddefault:tofield_registry.yaml, the single source of truth for the wire surface, covering all 85 fields acrosspdlp_settingsandmip_settings.default:values come from the C++ member initializers rather than the docs, which disagree in four places —barrier_iterative_refinement,barrier_step_scale,pdlp_precision,semi_continuous_big_m. Those look like docs bugs and are left for a separate change.postsolve_infoandpresolve_absolute_toleranceare undocumented everywhere, so they carry onlydefault:plus a note.The attributes render as leading comments on the generated proto fields. That consumer is deliberately minimal — it exists so the attributes are covered by
ci/verify_grpc_codegen.shrather than sitting inert and drifting. Emitting doxygen intoconstants.h, so the descriptions reach the published C API docs where thedoxygendefinedirectives currently render nothing, is the natural follow-up.Also adds
skills/cuopt-developer/references/grpc_codegen.md— the skill had no coverage of the codegen pipeline, so an agent could hand-editgenerated/or skip./build.sh codegen.Fields without either attribute generate byte-identical output, so
cuopt_remote_data.protois the only generated file that changes and the diff is purely additive comments. Verified withci/verify_grpc_codegen.shandprotoc.Follow-ups, not in scope:
flow_cover_cutsandobjective_stepexist inmip_solver_settings_tand the docs but have no registry entry, so they cannot be set over gRPC; and nothing assertsdefault:matches the C++ initializer.🤖 Generated with Claude Code