Add a code generator for boilerplate to move data in and out of protobuf messages for grpc#1007
Open
tmckayus wants to merge 3 commits intoNVIDIA:release/26.04from
Open
Add a code generator for boilerplate to move data in and out of protobuf messages for grpc#1007tmckayus wants to merge 3 commits intoNVIDIA:release/26.04from
tmckayus wants to merge 3 commits intoNVIDIA:release/26.04from
Conversation
tmckayus
added
non-breaking
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces a code generation pipeline for gRPC protobuf conversions, powered by a YAML field registry and Python generator. Data type definitions migrate from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 (11)
build.sh (1)
361-368: Make regeneration incremental or explicit.This now runs the generator on every
libcuopt/cuopt_grpc_serverbuild, which makes a normal compile rewrite repo-local generated sources and adds a hard Python/codegen dependency even when nothing changed. Given the workflow in the PR description, this looks better as an explicit regen target or a timestamp-based check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.sh` around lines 361 - 368, The regeneration block unconditionally runs the Python generator when building libcuopt/cuopt_grpc_server; change it to either an explicit make target (e.g., add a "regen-codegen" target) or guard the python invocation with a timestamp/hash check against field_registry.yaml and generated outputs: keep the current conditional on buildAll/hasArg but require an explicit arg (like hasArg regen_codegen) or compare modification times (field_registry.yaml vs files in cpp/codegen/generated) and only invoke python generate_conversions.py when the registry is newer or the generated files are missing; update buildAll/hasArg usage and the python call site in this block to implement the chosen approach.cpp/codegen/generated_baseline/generated_build_array_chunks.inc (1)
46-60: Consider explicit handling of all var_t enum values in the generator.The
default: vt_bytes.push_back('C');case (line 55) silently treats unknown variable types asCONTINUOUS. If additionalvar_tenum values are added in the future (e.g.,BINARY,SEMI_CONTINUOUS), this could silently produce incorrect behavior.Since this is auto-generated code, consider updating the generator (
generate_conversions.py) to either:
- Enumerate all known
var_tvalues explicitly- Throw an exception for unknown types instead of defaulting
This is a minor defensive coding suggestion for the generator—the current code works correctly for the existing enum values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/codegen/generated_baseline/generated_build_array_chunks.inc` around lines 46 - 60, The generator currently emits a switch over var_t that uses a default mapping to 'C', which can silently mis-handle new enum values; update generate_conversions.py so the generated code explicitly handles every var_t enum member (e.g., CONTINUOUS, INTEGER, BINARY, SEMI_CONTINUOUS, etc.) in the switch that builds vt_bytes (the block around problem.get_variable_types_host()), and if an unknown enum value is encountered emit code that throws or logs an error instead of using a permissive default; ensure the generated switch produces a clear failure path (exception or assert) referencing var_t so chunk_byte_blob(requests, cuopt::remote::FIELD_VARIABLE_TYPES, vt_bytes, upload_id, chunk_size_bytes) only receives validated values.cpp/codegen/generated_baseline/generated_chunked_arrays_to_problem.inc (1)
120-120: Variable shadowing: loop variablecshadows objective coefficientsc.The loop variable
char con line 120 shadows theauto cvariable declared on line 81 for objective coefficients. While this is functionally safe (the outercis no longer used), it's poor style that could cause confusion during debugging.Since this is auto-generated code, consider updating the generator to use a different loop variable name (e.g.,
chorvt_char).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/codegen/generated_baseline/generated_chunked_arrays_to_problem.inc` at line 120, The loop variable "char c" inside the for loop over var_types_str shadows the outer objective coefficient variable named "c"; rename the loop variable (e.g., to "ch" or "vt_char") in the generated code so it no longer collides with the outer "c" used for objective coefficients (update the generator that emits the for (char c : var_types_str) to use the new name and ensure any uses inside the loop reference that new identifier).cpp/codegen/README.md (1)
26-72: Add language specifier to fenced code block.The file layout code block lacks a language specifier, which triggers a markdownlint warning. Use
textorplaintextfor ASCII tree diagrams.📝 Suggested fix
-``` +```text cpp/codegen/ ├── field_registry.yaml # Source of truth for all fields🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/codegen/README.md` around lines 26 - 72, The fenced code block that shows the ASCII tree starting with "cpp/codegen/" in README.md is missing a language specifier; update the opening fence from ``` to ```text (or ```plaintext) so the block is recognized as plain text and the markdownlint warning is resolved.cpp/codegen/generated_baseline/cuopt_remote_data.proto (2)
6-6: Package directory mismatch flagged by static analysis.The proto package
cuopt.remoteconventionally expects the file to reside in acuopt/remote/directory structure, but this generated file is placed incpp/codegen/generated_baseline/. While this may work with the current CMake protobuf generation setup, it could cause issues with other proto tooling (e.g., Buf, language-specific proto plugins that rely on directory conventions).Since this is auto-generated, consider updating the generator or build configuration to either:
- Place generated protos in a directory matching the package path, or
- Configure proto tooling to ignore this mismatch if intentional
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/codegen/generated_baseline/cuopt_remote_data.proto` at line 6, The proto package declaration "package cuopt.remote;" in cuopt_remote_data.proto does not match the file's generated location; update the generator or build config so generated protos are emitted under a directory that matches the package path (e.g., cuopt/remote/) or, if intentional, configure the proto tooling to ignore/package-mismatch (e.g., adjust generator output path or your proto toolchain settings) so tools like Buf and language-specific plugins won't fail on the package-directory mismatch.
30-36: Enum values lack prefixes unlike other enums.The
PDLPSolverModeenum values (Stable1,Stable2, etc.) don't have a prefix, whilePDLPTerminationStatusandMIPTerminationStatususe prefixes (PDLP_,MIP_). Similarly,LPMethodvalues are unprefixed. This inconsistency is minor but could cause confusion or naming collisions in some language bindings.Consider adding a
PDLP_MODE_or similar prefix to solver mode values for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/codegen/generated_baseline/cuopt_remote_data.proto` around lines 30 - 36, The enum PDLPSolverMode defines values Stable1, Stable2, Methodical1, Fast1, Stable3 without the project prefix; update these identifiers to use a consistent prefix (e.g., PDLP_MODE_) such as PDLP_MODE_STABLE1, PDLP_MODE_STABLE2, PDLP_MODE_METHODICAL1, PDLP_MODE_FAST1, PDLP_MODE_STABLE3 in the enum PDLPSolverMode declaration and then update all references/usages of PDLPSolverMode values throughout the codebase (serialization/deserialization sites, switch/case, tests, and any generated bindings) to the new names to keep naming consistent with PDLP/MIP termination enums.cpp/codegen/generate_conversions.py (4)
1297-1335: Unused loop variablegnamein setter_groups iteration.The loop variable
gnameis not used within the loop body. This is a minor code smell flagged by Ruff (B007).Proposed fix
- for gname, gdef in setter_groups.items(): + for _gname, gdef in setter_groups.items(): cond = gdef.get("to_proto_condition")Apply similar changes at lines 1374, 1783, and 2022.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/codegen/generate_conversions.py` around lines 1297 - 1335, The for-loop over setter_groups currently uses an unused variable gname; change the loop header to use a throwaway variable (for example: for _, gdef in setter_groups.items():) to silence the Ruff B007 warning and make intent clear; apply the same replacement in the other occurrences mentioned (the loops around the code that references _find_problem_field, _proto_cpp_name, _default_problem_getter, and _array_element_cast at the comparable blocks near the later occurrences).
2180-2193: Convert lambda assignments to function definitions.Ruff flags E731 for lambda assignments. While functional, using
defis more idiomatic Python and provides better stack traces for debugging.Proposed fix
def _normalize_bare_strings(entries): """Convert bare string entries in a field list to single-key dicts. This lets the auto-numbering logic treat them uniformly.""" try: from ruamel.yaml.comments import CommentedMap mk_inner = CommentedMap - mk_outer = lambda name, inner: CommentedMap([(name, inner)]) + def mk_outer(name, inner): + return CommentedMap([(name, inner)]) except ImportError: mk_inner = dict - mk_outer = lambda name, inner: {name: inner} + def mk_outer(name, inner): + return {name: inner}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/codegen/generate_conversions.py` around lines 2180 - 2193, The mk_outer lambda should be replaced with a proper function definition to avoid E731; locate the mk_outer assignment in the try and except branches and replace each "mk_outer = lambda name, inner: ..." with a named def mk_outer(name, inner): that returns the corresponding CommentedMap([(name, inner)]) in the try branch and {name: inner} in the except branch, leaving mk_inner and subsequent logic (the loop over entries and handling of entries.ca) unchanged.
41-44: Specify explicit encoding for file operations.The
write_filefunction opens files without specifying encoding. While Python 3 defaults to UTF-8 on most systems, explicitly specifyingencoding="utf-8"ensures consistent behavior across platforms and avoids potential issues on Windows where the default encoding may differ.Proposed fix
def write_file(path, content): os.makedirs(os.path.dirname(path) or ".", exist_ok=True) - with open(path, "w") as f: + with open(path, "w", encoding="utf-8") as f: f.write(content) print(f" wrote {path}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/codegen/generate_conversions.py` around lines 41 - 44, The write_file helper currently opens files without an explicit encoding; update the write_file(path, content) function to open the file with encoding="utf-8" (i.e., pass encoding to open) so writes are deterministic across platforms and avoid Windows default-encoding issues; keep os.makedirs(os.path.dirname(path) or ".", exist_ok=True) and the function signature unchanged.
2409-2421: Add explicit encoding when reading/writing YAML files.File operations at lines 2409, 2414, and 2420 lack explicit encoding. This can cause issues on systems with non-UTF-8 default encodings.
Proposed fix
- with open(args.registry) as f: + with open(args.registry, encoding="utf-8") as f: rt_data = ryaml.load(f) n = auto_assign_field_numbers(rt_data) if n > 0: - with open(args.registry, "w") as f: + with open(args.registry, "w", encoding="utf-8") as f: ryaml.dump(rt_data, f) print(f" auto-numbered {n} field(s) in {args.registry}") else: print(" all field numbers already assigned") - with open(args.registry) as f: + with open(args.registry, encoding="utf-8") as f: registry = yaml.safe_load(f)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/codegen/generate_conversions.py` around lines 2409 - 2421, The three file opens that read/write YAML (the open calls around ryaml.load, ryaml.dump, and yaml.safe_load that use args.registry) lack explicit encoding; update each open(...) to specify encoding="utf-8" so reads/writes are deterministic across platforms (the locations to change are the open used with ryaml.load, the open used with ryaml.dump, and the final open used with yaml.safe_load).cpp/src/grpc/cuopt_remote_data.proto (1)
1-7: Package directory mismatch flagged by Buf linter.The Buf linter reports that files with package
cuopt.remoteshould be in directorycuopt/remote, but this file is incpp/src/grpc. This is a common project structure choice where proto files are co-located with their consumers rather than following a package-based directory layout. If this is intentional, consider adding abuf.yamlexception or adjusting Buf configuration to suppress this warning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/grpc/cuopt_remote_data.proto` around lines 1 - 7, The Buf linter flags that the proto package "cuopt.remote" (in cuopt_remote_data.proto) does not match the file directory (currently cpp/src/grpc); either move cuopt_remote_data.proto into a directory matching the package (cuopt/remote) or add a Buf configuration exception: update buf.yaml to allow files located under cpp/src/grpc for package cuopt.remote (e.g., configure module/file/ignore or add an allowlist) so the linter stops reporting the package-directory mismatch; pick whichever approach (relocate the file or adjust buf.yaml) fits project conventions and apply it consistently for other generated proto files as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/codegen/generated_baseline/generated_proto_to_pdlp_settings.inc`:
- Line 22: The enum casts like static_cast<presolver_t>(pb_settings.presolver())
and the similar static_cast<pdlp_precision_t> must be guarded by a validation
step in the generator: update the generator that emits
generated_proto_to_pdlp_settings.inc to read the C++ enum macro values from
constants.h and compare them against the proto integer values from
field_registry.yaml (the pb_settings.<field>() values) before emitting raw
static_casts; if any mismatch is detected, emit a runtime-check or generate code
that logs/throws a clear error (including the enum name, macro value, and proto
value) and fail the build/generation, ensuring the validation covers presolver_t
and pdlp_precision_t and any other enums converted this way.
In `@cpp/codegen/generated_baseline/generated_proto_to_problem.inc`:
- Around line 64-72: The loop mapping vt_str -> vtypes must not treat unknown
codes as CONTINUOUS; update the switch in the iterator over vt_str so that the
default branch reports/propagates a validation error (e.g., throw
std::invalid_argument or return a validation result) including the offending
char, instead of pushing var_t::CONTINUOUS; ensure has_integers and vtypes are
only updated for recognized codes (cases 'C','I','B') and that the caller of
this generated_proto_to_problem.inc surface the validation failure rather than
proceeding to solve.
In `@cpp/codegen/generated_baseline/generated_result_header_fields.proto.inc`:
- Around line 1-35: The generated baseline file has stale field numbers (e.g.,
PDLPTerminationStatus lp_termination_status, MIPTerminationStatus
mip_termination_status and many scalar fields) that do not match the current
proto (cuopt_remote_data.proto uses 1000/2000 ranges); fix by regenerating the
baseline with the codegen script (generate_conversions.py) so
generated_result_header_fields.proto.inc reflects the current proto field
numbers, or alternatively add clear documentation near the generator invocation
explaining why this file is intentionally diverged and how to update it (mention
lp_termination_status, mip_termination_status, and generate_conversions.py to
locate the relevant code paths).
---
Nitpick comments:
In `@build.sh`:
- Around line 361-368: The regeneration block unconditionally runs the Python
generator when building libcuopt/cuopt_grpc_server; change it to either an
explicit make target (e.g., add a "regen-codegen" target) or guard the python
invocation with a timestamp/hash check against field_registry.yaml and generated
outputs: keep the current conditional on buildAll/hasArg but require an explicit
arg (like hasArg regen_codegen) or compare modification times
(field_registry.yaml vs files in cpp/codegen/generated) and only invoke python
generate_conversions.py when the registry is newer or the generated files are
missing; update buildAll/hasArg usage and the python call site in this block to
implement the chosen approach.
In `@cpp/codegen/generate_conversions.py`:
- Around line 1297-1335: The for-loop over setter_groups currently uses an
unused variable gname; change the loop header to use a throwaway variable (for
example: for _, gdef in setter_groups.items():) to silence the Ruff B007 warning
and make intent clear; apply the same replacement in the other occurrences
mentioned (the loops around the code that references _find_problem_field,
_proto_cpp_name, _default_problem_getter, and _array_element_cast at the
comparable blocks near the later occurrences).
- Around line 2180-2193: The mk_outer lambda should be replaced with a proper
function definition to avoid E731; locate the mk_outer assignment in the try and
except branches and replace each "mk_outer = lambda name, inner: ..." with a
named def mk_outer(name, inner): that returns the corresponding
CommentedMap([(name, inner)]) in the try branch and {name: inner} in the except
branch, leaving mk_inner and subsequent logic (the loop over entries and
handling of entries.ca) unchanged.
- Around line 41-44: The write_file helper currently opens files without an
explicit encoding; update the write_file(path, content) function to open the
file with encoding="utf-8" (i.e., pass encoding to open) so writes are
deterministic across platforms and avoid Windows default-encoding issues; keep
os.makedirs(os.path.dirname(path) or ".", exist_ok=True) and the function
signature unchanged.
- Around line 2409-2421: The three file opens that read/write YAML (the open
calls around ryaml.load, ryaml.dump, and yaml.safe_load that use args.registry)
lack explicit encoding; update each open(...) to specify encoding="utf-8" so
reads/writes are deterministic across platforms (the locations to change are the
open used with ryaml.load, the open used with ryaml.dump, and the final open
used with yaml.safe_load).
In `@cpp/codegen/generated_baseline/cuopt_remote_data.proto`:
- Line 6: The proto package declaration "package cuopt.remote;" in
cuopt_remote_data.proto does not match the file's generated location; update the
generator or build config so generated protos are emitted under a directory that
matches the package path (e.g., cuopt/remote/) or, if intentional, configure the
proto tooling to ignore/package-mismatch (e.g., adjust generator output path or
your proto toolchain settings) so tools like Buf and language-specific plugins
won't fail on the package-directory mismatch.
- Around line 30-36: The enum PDLPSolverMode defines values Stable1, Stable2,
Methodical1, Fast1, Stable3 without the project prefix; update these identifiers
to use a consistent prefix (e.g., PDLP_MODE_) such as PDLP_MODE_STABLE1,
PDLP_MODE_STABLE2, PDLP_MODE_METHODICAL1, PDLP_MODE_FAST1, PDLP_MODE_STABLE3 in
the enum PDLPSolverMode declaration and then update all references/usages of
PDLPSolverMode values throughout the codebase (serialization/deserialization
sites, switch/case, tests, and any generated bindings) to the new names to keep
naming consistent with PDLP/MIP termination enums.
In `@cpp/codegen/generated_baseline/generated_build_array_chunks.inc`:
- Around line 46-60: The generator currently emits a switch over var_t that uses
a default mapping to 'C', which can silently mis-handle new enum values; update
generate_conversions.py so the generated code explicitly handles every var_t
enum member (e.g., CONTINUOUS, INTEGER, BINARY, SEMI_CONTINUOUS, etc.) in the
switch that builds vt_bytes (the block around
problem.get_variable_types_host()), and if an unknown enum value is encountered
emit code that throws or logs an error instead of using a permissive default;
ensure the generated switch produces a clear failure path (exception or assert)
referencing var_t so chunk_byte_blob(requests,
cuopt::remote::FIELD_VARIABLE_TYPES, vt_bytes, upload_id, chunk_size_bytes) only
receives validated values.
In `@cpp/codegen/generated_baseline/generated_chunked_arrays_to_problem.inc`:
- Line 120: The loop variable "char c" inside the for loop over var_types_str
shadows the outer objective coefficient variable named "c"; rename the loop
variable (e.g., to "ch" or "vt_char") in the generated code so it no longer
collides with the outer "c" used for objective coefficients (update the
generator that emits the for (char c : var_types_str) to use the new name and
ensure any uses inside the loop reference that new identifier).
In `@cpp/codegen/README.md`:
- Around line 26-72: The fenced code block that shows the ASCII tree starting
with "cpp/codegen/" in README.md is missing a language specifier; update the
opening fence from ``` to ```text (or ```plaintext) so the block is recognized
as plain text and the markdownlint warning is resolved.
In `@cpp/src/grpc/cuopt_remote_data.proto`:
- Around line 1-7: The Buf linter flags that the proto package "cuopt.remote"
(in cuopt_remote_data.proto) does not match the file directory (currently
cpp/src/grpc); either move cuopt_remote_data.proto into a directory matching the
package (cuopt/remote) or add a Buf configuration exception: update buf.yaml to
allow files located under cpp/src/grpc for package cuopt.remote (e.g., configure
module/file/ignore or add an allowlist) so the linter stops reporting the
package-directory mismatch; pick whichever approach (relocate the file or adjust
buf.yaml) fits project conventions and apply it consistently for other generated
proto files as well.
🪄 Autofix (Beta)
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: Pro
Run ID: 4d031009-1820-4b8d-86c4-e9139d68680d
⛔ Files ignored due to path filters (30)
cpp/codegen/generated/cuopt_remote_data.protois excluded by!**/generated/**cpp/codegen/generated/generated_array_field_element_size.incis excluded by!**/generated/**cpp/codegen/generated/generated_build_array_chunks.incis excluded by!**/generated/**cpp/codegen/generated/generated_chunked_arrays_to_problem.incis excluded by!**/generated/**cpp/codegen/generated/generated_chunked_header_to_problem.incis excluded by!**/generated/**cpp/codegen/generated/generated_chunked_to_lp_solution.incis excluded by!**/generated/**cpp/codegen/generated/generated_chunked_to_mip_solution.incis excluded by!**/generated/**cpp/codegen/generated/generated_collect_lp_arrays.incis excluded by!**/generated/**cpp/codegen/generated/generated_collect_mip_arrays.incis excluded by!**/generated/**cpp/codegen/generated/generated_enum_converters_problem.incis excluded by!**/generated/**cpp/codegen/generated/generated_enum_converters_settings.incis excluded by!**/generated/**cpp/codegen/generated/generated_enum_converters_solution.incis excluded by!**/generated/**cpp/codegen/generated/generated_estimate_lp_size.incis excluded by!**/generated/**cpp/codegen/generated/generated_estimate_mip_size.incis excluded by!**/generated/**cpp/codegen/generated/generated_estimate_problem_size.incis excluded by!**/generated/**cpp/codegen/generated/generated_lp_chunked_header.incis excluded by!**/generated/**cpp/codegen/generated/generated_lp_solution_to_proto.incis excluded by!**/generated/**cpp/codegen/generated/generated_mip_chunked_header.incis excluded by!**/generated/**cpp/codegen/generated/generated_mip_settings_to_proto.incis excluded by!**/generated/**cpp/codegen/generated/generated_mip_solution_to_proto.incis excluded by!**/generated/**cpp/codegen/generated/generated_pdlp_settings_to_proto.incis excluded by!**/generated/**cpp/codegen/generated/generated_populate_chunked_header_lp.incis excluded by!**/generated/**cpp/codegen/generated/generated_populate_chunked_header_mip.incis excluded by!**/generated/**cpp/codegen/generated/generated_problem_to_proto.incis excluded by!**/generated/**cpp/codegen/generated/generated_proto_to_lp_solution.incis excluded by!**/generated/**cpp/codegen/generated/generated_proto_to_mip_settings.incis excluded by!**/generated/**cpp/codegen/generated/generated_proto_to_mip_solution.incis excluded by!**/generated/**cpp/codegen/generated/generated_proto_to_pdlp_settings.incis excluded by!**/generated/**cpp/codegen/generated/generated_proto_to_problem.incis excluded by!**/generated/**cpp/codegen/generated/generated_result_enums.proto.incis excluded by!**/generated/**
📒 Files selected for processing (45)
GRPC_SERVER_ARCHITECTURE.mdbuild.shcpp/CMakeLists.txtcpp/codegen/README.mdcpp/codegen/field_registry.yamlcpp/codegen/generate_conversions.pycpp/codegen/generated_baseline/cuopt_remote_data.protocpp/codegen/generated_baseline/generated_array_field_element_size.inccpp/codegen/generated_baseline/generated_build_array_chunks.inccpp/codegen/generated_baseline/generated_chunked_arrays_to_problem.inccpp/codegen/generated_baseline/generated_chunked_header_to_problem.inccpp/codegen/generated_baseline/generated_chunked_to_lp_solution.inccpp/codegen/generated_baseline/generated_chunked_to_mip_solution.inccpp/codegen/generated_baseline/generated_collect_lp_arrays.inccpp/codegen/generated_baseline/generated_collect_mip_arrays.inccpp/codegen/generated_baseline/generated_enum_converters.inccpp/codegen/generated_baseline/generated_estimate_lp_size.inccpp/codegen/generated_baseline/generated_estimate_mip_size.inccpp/codegen/generated_baseline/generated_estimate_problem_size.inccpp/codegen/generated_baseline/generated_lp_chunked_header.inccpp/codegen/generated_baseline/generated_lp_solution_to_proto.inccpp/codegen/generated_baseline/generated_mip_chunked_header.inccpp/codegen/generated_baseline/generated_mip_settings_to_proto.inccpp/codegen/generated_baseline/generated_mip_solution_to_proto.inccpp/codegen/generated_baseline/generated_pdlp_settings_to_proto.inccpp/codegen/generated_baseline/generated_populate_chunked_header_lp.inccpp/codegen/generated_baseline/generated_populate_chunked_header_mip.inccpp/codegen/generated_baseline/generated_problem_to_proto.inccpp/codegen/generated_baseline/generated_proto_to_lp_solution.inccpp/codegen/generated_baseline/generated_proto_to_mip_settings.inccpp/codegen/generated_baseline/generated_proto_to_mip_solution.inccpp/codegen/generated_baseline/generated_proto_to_pdlp_settings.inccpp/codegen/generated_baseline/generated_proto_to_problem.inccpp/codegen/generated_baseline/generated_result_enums.proto.inccpp/codegen/generated_baseline/generated_result_header_fields.proto.inccpp/include/cuopt/linear_programming/cpu_optimization_problem.hppcpp/src/grpc/client/grpc_client.cppcpp/src/grpc/cuopt_remote.protocpp/src/grpc/cuopt_remote_data.protocpp/src/grpc/cuopt_remote_service.protocpp/src/grpc/grpc_problem_mapper.cppcpp/src/grpc/grpc_settings_mapper.cppcpp/src/grpc/grpc_solution_mapper.cppcpp/src/grpc/grpc_solution_mapper.hppcpp/src/grpc/server/grpc_field_element_size.hpp
💤 Files with no reviewable changes (1)
- cpp/src/grpc/grpc_solution_mapper.hpp
cpp/codegen/generated_baseline/generated_proto_to_pdlp_settings.inc
Outdated
Show resolved
Hide resolved
cpp/codegen/generated_baseline/generated_result_header_fields.proto.inc
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
cpp/src/grpc/grpc_solution_mapper.cpp (1)
35-42: Helper functiondoubles_to_bytesalways converts todoubleregardless off_t.The template function converts
vecelements todoublebefore serialization. Whenf_tisfloat, this performs an implicit widening conversion. While this ensures wire-format consistency, it may be intentional to always transmit asdoublefor precision. Verify this matches the expected behavior and proto schema (which usesrepeated doublefor numeric arrays).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/grpc/grpc_solution_mapper.cpp` around lines 35 - 42, The helper doubles_to_bytes currently converts the input vector elements to double unconditionally inside doubles_to_bytes, which widens floats implicitly; decide whether the wire format must be repeated double or should preserve the original f_t size. If the proto expects repeated double, make the conversion explicit and document/rename the function to reflect that it serializes as doubles (and ensure no loss of intent); otherwise change the implementation to serialize the original type by using a temporary vector of f_t (or directly memcpy vec.data()) and use sizeof(f_t) so floats remain 32-bit on the wire. Update doubles_to_bytes and any callers accordingly and add a static_assert or comment referencing the proto schema (repeated double) to make the choice explicit.cpp/src/grpc/grpc_settings_mapper.cpp (2)
14-16: Potentially unused includes after refactor.The
<stdexcept>and<string>headers may no longer be needed since the hand-written enum conversion logic (which usedstd::invalid_argument) has been replaced with generated code. Consider removing them if the generated.incfiles don't require them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/grpc/grpc_settings_mapper.cpp` around lines 14 - 16, The includes <stdexcept> and <string> in grpc_settings_mapper.cpp appear to be vestigial after switching to generated enum conversion code; remove those two lines and rebuild to confirm they aren't required by the included generated .inc files (leave <limits> if still used); if the build breaks, reintroduce only the specific header (stdexcept or string) required by the failing translation unit or update the generated .inc to include the needed header so the top-level file remains minimal.
20-22: Enum conversion now silently defaults unknown values instead of throwing.The generated enum converter (per context snippet
cpp/codegen/generated/generated_enum_converters_settings.inc) returns a default value (e.g.,Stable3) for unknown enum values rather than throwingstd::invalid_argument. This is a behavioral change from the previous hand-written mappers. This is actually correct for proto3, which requires graceful handling of unknown enum values, but callers that previously relied on exceptions for invalid enum detection will need adjustment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/grpc/grpc_settings_mapper.cpp` around lines 20 - 22, The generated enum converters included via generated_enum_converters_settings.inc now map unknown proto enum values to a default (e.g., Stable3) instead of throwing std::invalid_argument, so update callers or add a thin validation wrapper in grpc_settings_mapper.cpp: implement a function (e.g., ValidateAndMapSettingEnum) that calls the generated converter from generated_enum_converters_settings.inc, detects the sentinel/default result (or an unrecognized proto ordinal) and throws std::invalid_argument for unknown values, and then replace direct calls to the generated converter with this wrapper (or else update each caller to handle the default result explicitly).cpp/codegen/generated_baseline/cuopt_remote_data.proto (1)
6-6: Buf lint error: Package directory mismatch.The static analysis reports that files with package
cuopt.remoteshould be in acuopt/remotedirectory, but this file is incpp/codegen/generated_baseline. This is likely acceptable for a baseline/reference file that isn't directly compiled by protoc, but consider adding this path to buf's exclude list or documenting that this is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/codegen/generated_baseline/cuopt_remote_data.proto` at line 6, The proto package declaration "package cuopt.remote" in cuopt_remote_data.proto conflicts with buf's package-directory rule; either move or duplicate this file under a matching directory structure (cuopt/remote) or add the current path (cpp/codegen/generated_baseline) to buf's exclude/ignore configuration so buf lint accepts this baseline; update the project's buf.yaml (or documentation) to reflect the intentional exception and reference the proto by its package name "cuopt.remote" so reviewers can find the impacted file.cpp/codegen/generate_conversions.py (2)
1-5: Duplicate SPDX license identifier.Line 5 duplicates the license identifier already on line 3.
📝 Suggested fix
#!/usr/bin/env python3 # SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION & AFFILIATES. # SPDX-License-Identifier: Apache-2.0 # All rights reserved. -# SPDX-License-Identifier: Apache-2.0 """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/codegen/generate_conversions.py` around lines 1 - 5, The file header contains a duplicated SPDX license identifier; remove the redundant "SPDX-License-Identifier: Apache-2.0" entry so only a single SPDX-License-Identifier line remains in cpp/codegen/generate_conversions.py (keep the canonical SPDX header and delete the duplicate on line 5).
1822-1862: Generator produces silent default for unknown variable types.The
_gen_chunked_arrays_to_problemfunction (and similarly_gen_proto_to_problemaround lines 1423-1442) generates code that silently maps unrecognized variable type characters tovar_t::CONTINUOUS. This was flagged in past reviews on the generated.incfiles.Consider updating the generator to emit validation logic that throws or logs an error for unknown codes instead of silently defaulting. For example, the generated switch could include a
defaultcase that reports the offending character.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/codegen/generate_conversions.py` around lines 1822 - 1862, The generator currently emits switch logic that maps unknown variable type characters silently to var_t::CONTINUOUS; update _gen_chunked_arrays_to_problem (and similarly _gen_proto_to_problem) so the generated switch includes a default case that reports the offending character and fails fast (e.g., throw std::runtime_error or call an error logger) instead of defaulting. In practice, change the code that emits the switch for variable types (look for the logic emitting var_t::CONTINUOUS) to append a default: branch that includes the field/variable identifier and the unexpected character in the error message so the compiled .inc will surface invalid codes at runtime or during initialization. Ensure the emitted message references the variable name/field and the raw char so debugging is straightforward.cpp/codegen/README.md (1)
26-26: Add language specifier to fenced code block.The file layout code block is missing a language specifier. Using
textorplaintextwould satisfy the linter and improve syntax highlighting in some renderers.📝 Suggested fix
-``` +```text cpp/codegen/ ├── field_registry.yaml # Source of truth for all fields🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/codegen/README.md` at line 26, The fenced code block that starts with the snippet "cpp/codegen/" is missing a language specifier; update the opening triple backticks for that block to include a language such as text or plaintext (e.g., change ``` to ```text) so the linter passes and renderers get proper highlighting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/codegen/generated_baseline/cuopt_remote_data.proto`:
- Around line 218-219: The baseline proto is stale: update the generated file so
ChunkedResultHeader matches the current schema (replace the field `bool is_mip =
1;` with `ProblemCategory problem_category = 1;` as in
cpp/src/grpc/cuopt_remote_data.proto) by re-running the codegen/regeneration
process (invoke the project’s proto/codegen script used to produce
generated_baseline files) so the generated message ChunkedResultHeader and its
field definition align with the actual proto.
---
Nitpick comments:
In `@cpp/codegen/generate_conversions.py`:
- Around line 1-5: The file header contains a duplicated SPDX license
identifier; remove the redundant "SPDX-License-Identifier: Apache-2.0" entry so
only a single SPDX-License-Identifier line remains in
cpp/codegen/generate_conversions.py (keep the canonical SPDX header and delete
the duplicate on line 5).
- Around line 1822-1862: The generator currently emits switch logic that maps
unknown variable type characters silently to var_t::CONTINUOUS; update
_gen_chunked_arrays_to_problem (and similarly _gen_proto_to_problem) so the
generated switch includes a default case that reports the offending character
and fails fast (e.g., throw std::runtime_error or call an error logger) instead
of defaulting. In practice, change the code that emits the switch for variable
types (look for the logic emitting var_t::CONTINUOUS) to append a default:
branch that includes the field/variable identifier and the unexpected character
in the error message so the compiled .inc will surface invalid codes at runtime
or during initialization. Ensure the emitted message references the variable
name/field and the raw char so debugging is straightforward.
In `@cpp/codegen/generated_baseline/cuopt_remote_data.proto`:
- Line 6: The proto package declaration "package cuopt.remote" in
cuopt_remote_data.proto conflicts with buf's package-directory rule; either move
or duplicate this file under a matching directory structure (cuopt/remote) or
add the current path (cpp/codegen/generated_baseline) to buf's exclude/ignore
configuration so buf lint accepts this baseline; update the project's buf.yaml
(or documentation) to reflect the intentional exception and reference the proto
by its package name "cuopt.remote" so reviewers can find the impacted file.
In `@cpp/codegen/README.md`:
- Line 26: The fenced code block that starts with the snippet "cpp/codegen/" is
missing a language specifier; update the opening triple backticks for that block
to include a language such as text or plaintext (e.g., change ``` to ```text) so
the linter passes and renderers get proper highlighting.
In `@cpp/src/grpc/grpc_settings_mapper.cpp`:
- Around line 14-16: The includes <stdexcept> and <string> in
grpc_settings_mapper.cpp appear to be vestigial after switching to generated
enum conversion code; remove those two lines and rebuild to confirm they aren't
required by the included generated .inc files (leave <limits> if still used); if
the build breaks, reintroduce only the specific header (stdexcept or string)
required by the failing translation unit or update the generated .inc to include
the needed header so the top-level file remains minimal.
- Around line 20-22: The generated enum converters included via
generated_enum_converters_settings.inc now map unknown proto enum values to a
default (e.g., Stable3) instead of throwing std::invalid_argument, so update
callers or add a thin validation wrapper in grpc_settings_mapper.cpp: implement
a function (e.g., ValidateAndMapSettingEnum) that calls the generated converter
from generated_enum_converters_settings.inc, detects the sentinel/default result
(or an unrecognized proto ordinal) and throws std::invalid_argument for unknown
values, and then replace direct calls to the generated converter with this
wrapper (or else update each caller to handle the default result explicitly).
In `@cpp/src/grpc/grpc_solution_mapper.cpp`:
- Around line 35-42: The helper doubles_to_bytes currently converts the input
vector elements to double unconditionally inside doubles_to_bytes, which widens
floats implicitly; decide whether the wire format must be repeated double or
should preserve the original f_t size. If the proto expects repeated double,
make the conversion explicit and document/rename the function to reflect that it
serializes as doubles (and ensure no loss of intent); otherwise change the
implementation to serialize the original type by using a temporary vector of f_t
(or directly memcpy vec.data()) and use sizeof(f_t) so floats remain 32-bit on
the wire. Update doubles_to_bytes and any callers accordingly and add a
static_assert or comment referencing the proto schema (repeated double) to make
the choice explicit.
🪄 Autofix (Beta)
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: Pro
Run ID: f43cf9d9-dbe4-4c8d-bb72-b255a829bb29
⛔ Files ignored due to path filters (30)
cpp/codegen/generated/cuopt_remote_data.protois excluded by!**/generated/**cpp/codegen/generated/generated_array_field_element_size.incis excluded by!**/generated/**cpp/codegen/generated/generated_build_array_chunks.incis excluded by!**/generated/**cpp/codegen/generated/generated_chunked_arrays_to_problem.incis excluded by!**/generated/**cpp/codegen/generated/generated_chunked_header_to_problem.incis excluded by!**/generated/**cpp/codegen/generated/generated_chunked_to_lp_solution.incis excluded by!**/generated/**cpp/codegen/generated/generated_chunked_to_mip_solution.incis excluded by!**/generated/**cpp/codegen/generated/generated_collect_lp_arrays.incis excluded by!**/generated/**cpp/codegen/generated/generated_collect_mip_arrays.incis excluded by!**/generated/**cpp/codegen/generated/generated_enum_converters_problem.incis excluded by!**/generated/**cpp/codegen/generated/generated_enum_converters_settings.incis excluded by!**/generated/**cpp/codegen/generated/generated_enum_converters_solution.incis excluded by!**/generated/**cpp/codegen/generated/generated_estimate_lp_size.incis excluded by!**/generated/**cpp/codegen/generated/generated_estimate_mip_size.incis excluded by!**/generated/**cpp/codegen/generated/generated_estimate_problem_size.incis excluded by!**/generated/**cpp/codegen/generated/generated_lp_chunked_header.incis excluded by!**/generated/**cpp/codegen/generated/generated_lp_solution_to_proto.incis excluded by!**/generated/**cpp/codegen/generated/generated_mip_chunked_header.incis excluded by!**/generated/**cpp/codegen/generated/generated_mip_settings_to_proto.incis excluded by!**/generated/**cpp/codegen/generated/generated_mip_solution_to_proto.incis excluded by!**/generated/**cpp/codegen/generated/generated_pdlp_settings_to_proto.incis excluded by!**/generated/**cpp/codegen/generated/generated_populate_chunked_header_lp.incis excluded by!**/generated/**cpp/codegen/generated/generated_populate_chunked_header_mip.incis excluded by!**/generated/**cpp/codegen/generated/generated_problem_to_proto.incis excluded by!**/generated/**cpp/codegen/generated/generated_proto_to_lp_solution.incis excluded by!**/generated/**cpp/codegen/generated/generated_proto_to_mip_settings.incis excluded by!**/generated/**cpp/codegen/generated/generated_proto_to_mip_solution.incis excluded by!**/generated/**cpp/codegen/generated/generated_proto_to_pdlp_settings.incis excluded by!**/generated/**cpp/codegen/generated/generated_proto_to_problem.incis excluded by!**/generated/**cpp/codegen/generated/generated_result_enums.proto.incis excluded by!**/generated/**
📒 Files selected for processing (53)
GRPC_INTERFACE.mdGRPC_QUICK_START.mdGRPC_SERVER_ARCHITECTURE.mdbuild.shcpp/CMakeLists.txtcpp/codegen/README.mdcpp/codegen/field_registry.yamlcpp/codegen/generate_conversions.pycpp/codegen/generated_baseline/cuopt_remote_data.protocpp/codegen/generated_baseline/generated_array_field_element_size.inccpp/codegen/generated_baseline/generated_build_array_chunks.inccpp/codegen/generated_baseline/generated_chunked_arrays_to_problem.inccpp/codegen/generated_baseline/generated_chunked_header_to_problem.inccpp/codegen/generated_baseline/generated_chunked_to_lp_solution.inccpp/codegen/generated_baseline/generated_chunked_to_mip_solution.inccpp/codegen/generated_baseline/generated_collect_lp_arrays.inccpp/codegen/generated_baseline/generated_collect_mip_arrays.inccpp/codegen/generated_baseline/generated_enum_converters.inccpp/codegen/generated_baseline/generated_estimate_lp_size.inccpp/codegen/generated_baseline/generated_estimate_mip_size.inccpp/codegen/generated_baseline/generated_estimate_problem_size.inccpp/codegen/generated_baseline/generated_lp_chunked_header.inccpp/codegen/generated_baseline/generated_lp_solution_to_proto.inccpp/codegen/generated_baseline/generated_mip_chunked_header.inccpp/codegen/generated_baseline/generated_mip_settings_to_proto.inccpp/codegen/generated_baseline/generated_mip_solution_to_proto.inccpp/codegen/generated_baseline/generated_pdlp_settings_to_proto.inccpp/codegen/generated_baseline/generated_populate_chunked_header_lp.inccpp/codegen/generated_baseline/generated_populate_chunked_header_mip.inccpp/codegen/generated_baseline/generated_problem_to_proto.inccpp/codegen/generated_baseline/generated_proto_to_lp_solution.inccpp/codegen/generated_baseline/generated_proto_to_mip_settings.inccpp/codegen/generated_baseline/generated_proto_to_mip_solution.inccpp/codegen/generated_baseline/generated_proto_to_pdlp_settings.inccpp/codegen/generated_baseline/generated_proto_to_problem.inccpp/codegen/generated_baseline/generated_result_enums.proto.inccpp/codegen/generated_baseline/generated_result_header_fields.proto.inccpp/include/cuopt/linear_programming/cpu_optimization_problem.hppcpp/src/grpc/client/grpc_client.cppcpp/src/grpc/client/grpc_client.hppcpp/src/grpc/cuopt_remote.protocpp/src/grpc/cuopt_remote_data.protocpp/src/grpc/cuopt_remote_service.protocpp/src/grpc/grpc_problem_mapper.cppcpp/src/grpc/grpc_settings_mapper.cppcpp/src/grpc/grpc_solution_mapper.cppcpp/src/grpc/grpc_solution_mapper.hppcpp/src/grpc/server/grpc_field_element_size.hppcpp/src/grpc/server/grpc_server_main.cppcpp/src/grpc/server/grpc_server_types.hppcpp/tests/linear_programming/grpc/CMakeLists.txtcpp/tests/linear_programming/grpc/grpc_client_test.cppcpp/tests/linear_programming/grpc/grpc_pipe_serialization_test.cpp
💤 Files with no reviewable changes (1)
- cpp/src/grpc/grpc_solution_mapper.hpp
✅ Files skipped from review due to trivial changes (26)
- cpp/src/grpc/server/grpc_server_types.hpp
- cpp/src/grpc/client/grpc_client.cpp
- cpp/src/grpc/server/grpc_server_main.cpp
- cpp/codegen/generated_baseline/generated_estimate_mip_size.inc
- cpp/src/grpc/client/grpc_client.hpp
- cpp/codegen/generated_baseline/generated_mip_solution_to_proto.inc
- cpp/codegen/generated_baseline/generated_chunked_header_to_problem.inc
- cpp/codegen/generated_baseline/generated_populate_chunked_header_lp.inc
- cpp/codegen/generated_baseline/generated_lp_solution_to_proto.inc
- cpp/codegen/generated_baseline/generated_collect_mip_arrays.inc
- cpp/codegen/generated_baseline/generated_estimate_lp_size.inc
- cpp/codegen/generated_baseline/generated_chunked_to_mip_solution.inc
- cpp/codegen/generated_baseline/generated_populate_chunked_header_mip.inc
- cpp/codegen/generated_baseline/generated_collect_lp_arrays.inc
- cpp/codegen/generated_baseline/generated_build_array_chunks.inc
- cpp/codegen/generated_baseline/generated_mip_settings_to_proto.inc
- cpp/codegen/generated_baseline/generated_proto_to_mip_settings.inc
- cpp/codegen/generated_baseline/generated_pdlp_settings_to_proto.inc
- GRPC_QUICK_START.md
- cpp/codegen/generated_baseline/generated_proto_to_pdlp_settings.inc
- cpp/codegen/generated_baseline/generated_problem_to_proto.inc
- cpp/codegen/generated_baseline/generated_chunked_to_lp_solution.inc
- cpp/codegen/generated_baseline/generated_result_enums.proto.inc
- cpp/codegen/generated_baseline/generated_proto_to_mip_solution.inc
- cpp/codegen/generated_baseline/generated_array_field_element_size.inc
- cpp/codegen/generated_baseline/generated_result_header_fields.proto.inc
🚧 Files skipped from review as they are similar to previous changes (10)
- cpp/src/grpc/server/grpc_field_element_size.hpp
- cpp/codegen/generated_baseline/generated_mip_chunked_header.inc
- cpp/CMakeLists.txt
- cpp/codegen/generated_baseline/generated_proto_to_lp_solution.inc
- cpp/include/cuopt/linear_programming/cpu_optimization_problem.hpp
- cpp/codegen/generated_baseline/generated_lp_chunked_header.inc
- cpp/codegen/generated_baseline/generated_estimate_problem_size.inc
- cpp/src/grpc/cuopt_remote_service.proto
- GRPC_SERVER_ARCHITECTURE.md
- cpp/codegen/generated_baseline/generated_enum_converters.inc
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cpp/codegen/generate_conversions.py (3)
54-68: Consider usingnext(iter(...))for single-key dict access.The pattern
list(entry.keys())[0]appears multiple times throughout the file (lines 58, 75, 100, 2079, 2091, 2119, 2151). Usingnext(iter(entry.keys()))is slightly more idiomatic and avoids creating an intermediate list.♻️ Example refactor
- name = list(entry.keys())[0] + name = next(iter(entry.keys()))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/codegen/generate_conversions.py` around lines 54 - 68, Replace uses of list(entry.keys())[0] (used when extracting the single key from a one-key dict in the conversion logic that handles variable "entry") with next(iter(entry)) (or next(iter(entry.keys()))) to avoid creating an intermediate list; update every occurrence in this file (including the shown block that returns {"name": name, ...} and the other spots noted in the review) and keep the same assertion that the dict has one key so behavior is unchanged.
1297-1297: Unused loop variablegnamein setter_groups iteration.This pattern appears at lines 1297, 1374, 1783, and 2022. Consider renaming to
_gnameto indicate intentional non-use, or use.values()if only the value is needed.♻️ Example fix
- for gname, gdef in setter_groups.items(): + for _gname, gdef in setter_groups.items():Or if
gnameis truly never needed:- for gname, gdef in setter_groups.items(): + for gdef in setter_groups.values():🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/codegen/generate_conversions.py` at line 1297, The for-loop uses an unused variable name gname in "for gname, gdef in setter_groups.items()"; rename gname to _gname (or iterate over setter_groups.values() as "for gdef in setter_groups.values()") to signal intentional non-use and eliminate the unused-variable warning, and apply the same change to the other identical loops where "for gname, gdef in setter_groups.items()" appears.
1-19: Well-documented code generator with clear purpose.The header and docstring clearly document the generator's purpose and output files. The licensing is correctly specified, though there's a duplicate
SPDX-License-Identifieron lines 3 and 5.🔧 Minor: Remove duplicate license identifier
# SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION & AFFILIATES. -# SPDX-License-Identifier: Apache-2.0 # All rights reserved. # SPDX-License-Identifier: Apache-2.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/codegen/generate_conversions.py` around lines 1 - 19, The file header contains a duplicated SPDX-License-Identifier entry; remove the redundant line so only a single "SPDX-License-Identifier: Apache-2.0" remains in the top-of-file comments (leave the shebang and the SPDX-FileCopyrightText intact); locate this in generate_conversions.py near the file header and delete the extra SPDX-License-Identifier line.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/codegen/generate_conversions.py`:
- Around line 1166-1171: The current logic in parse_field/getter derives
size_getter by blindly doing getter.replace("_host()", "_size()"), which breaks
when a field supplies a custom getter via the YAML `getter` key; update the code
to first check if the field dict `f` contains an explicit `size_getter` (or
`size_getter` YAML key) and use that when present, otherwise validate that
`getter` ends with "_host()" before performing the replacement and raise a clear
error if not; change the variable referenced in lines.append to use the chosen
`size_getter` (as used now) and ensure parse_field or the calling site
documents/accepts the new `size_getter` key so custom getters are handled
safely.
---
Nitpick comments:
In `@cpp/codegen/generate_conversions.py`:
- Around line 54-68: Replace uses of list(entry.keys())[0] (used when extracting
the single key from a one-key dict in the conversion logic that handles variable
"entry") with next(iter(entry)) (or next(iter(entry.keys()))) to avoid creating
an intermediate list; update every occurrence in this file (including the shown
block that returns {"name": name, ...} and the other spots noted in the review)
and keep the same assertion that the dict has one key so behavior is unchanged.
- Line 1297: The for-loop uses an unused variable name gname in "for gname, gdef
in setter_groups.items()"; rename gname to _gname (or iterate over
setter_groups.values() as "for gdef in setter_groups.values()") to signal
intentional non-use and eliminate the unused-variable warning, and apply the
same change to the other identical loops where "for gname, gdef in
setter_groups.items()" appears.
- Around line 1-19: The file header contains a duplicated
SPDX-License-Identifier entry; remove the redundant line so only a single
"SPDX-License-Identifier: Apache-2.0" remains in the top-of-file comments (leave
the shebang and the SPDX-FileCopyrightText intact); locate this in
generate_conversions.py near the file header and delete the extra
SPDX-License-Identifier line.
🪄 Autofix (Beta)
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: Pro
Run ID: b4d5c993-698a-48f5-a46f-e19d86bbecff
⛔ Files ignored due to path filters (30)
cpp/codegen/generated/cuopt_remote_data.protois excluded by!**/generated/**cpp/codegen/generated/generated_array_field_element_size.incis excluded by!**/generated/**cpp/codegen/generated/generated_build_array_chunks.incis excluded by!**/generated/**cpp/codegen/generated/generated_chunked_arrays_to_problem.incis excluded by!**/generated/**cpp/codegen/generated/generated_chunked_header_to_problem.incis excluded by!**/generated/**cpp/codegen/generated/generated_chunked_to_lp_solution.incis excluded by!**/generated/**cpp/codegen/generated/generated_chunked_to_mip_solution.incis excluded by!**/generated/**cpp/codegen/generated/generated_collect_lp_arrays.incis excluded by!**/generated/**cpp/codegen/generated/generated_collect_mip_arrays.incis excluded by!**/generated/**cpp/codegen/generated/generated_enum_converters_problem.incis excluded by!**/generated/**cpp/codegen/generated/generated_enum_converters_settings.incis excluded by!**/generated/**cpp/codegen/generated/generated_enum_converters_solution.incis excluded by!**/generated/**cpp/codegen/generated/generated_estimate_lp_size.incis excluded by!**/generated/**cpp/codegen/generated/generated_estimate_mip_size.incis excluded by!**/generated/**cpp/codegen/generated/generated_estimate_problem_size.incis excluded by!**/generated/**cpp/codegen/generated/generated_lp_chunked_header.incis excluded by!**/generated/**cpp/codegen/generated/generated_lp_solution_to_proto.incis excluded by!**/generated/**cpp/codegen/generated/generated_mip_chunked_header.incis excluded by!**/generated/**cpp/codegen/generated/generated_mip_settings_to_proto.incis excluded by!**/generated/**cpp/codegen/generated/generated_mip_solution_to_proto.incis excluded by!**/generated/**cpp/codegen/generated/generated_pdlp_settings_to_proto.incis excluded by!**/generated/**cpp/codegen/generated/generated_populate_chunked_header_lp.incis excluded by!**/generated/**cpp/codegen/generated/generated_populate_chunked_header_mip.incis excluded by!**/generated/**cpp/codegen/generated/generated_problem_to_proto.incis excluded by!**/generated/**cpp/codegen/generated/generated_proto_to_lp_solution.incis excluded by!**/generated/**cpp/codegen/generated/generated_proto_to_mip_settings.incis excluded by!**/generated/**cpp/codegen/generated/generated_proto_to_mip_solution.incis excluded by!**/generated/**cpp/codegen/generated/generated_proto_to_pdlp_settings.incis excluded by!**/generated/**cpp/codegen/generated/generated_proto_to_problem.incis excluded by!**/generated/**cpp/codegen/generated/generated_result_enums.proto.incis excluded by!**/generated/**
📒 Files selected for processing (24)
GRPC_INTERFACE.mdGRPC_QUICK_START.mdGRPC_SERVER_ARCHITECTURE.mdbuild.shcpp/CMakeLists.txtcpp/codegen/README.mdcpp/codegen/field_registry.yamlcpp/codegen/generate_conversions.pycpp/include/cuopt/linear_programming/cpu_optimization_problem.hppcpp/src/grpc/client/grpc_client.cppcpp/src/grpc/client/grpc_client.hppcpp/src/grpc/cuopt_remote.protocpp/src/grpc/cuopt_remote_data.protocpp/src/grpc/cuopt_remote_service.protocpp/src/grpc/grpc_problem_mapper.cppcpp/src/grpc/grpc_settings_mapper.cppcpp/src/grpc/grpc_solution_mapper.cppcpp/src/grpc/grpc_solution_mapper.hppcpp/src/grpc/server/grpc_field_element_size.hppcpp/src/grpc/server/grpc_server_main.cppcpp/src/grpc/server/grpc_server_types.hppcpp/tests/linear_programming/grpc/CMakeLists.txtcpp/tests/linear_programming/grpc/grpc_client_test.cppcpp/tests/linear_programming/grpc/grpc_pipe_serialization_test.cpp
💤 Files with no reviewable changes (1)
- cpp/src/grpc/grpc_solution_mapper.hpp
✅ Files skipped from review due to trivial changes (7)
- cpp/src/grpc/client/grpc_client.cpp
- cpp/tests/linear_programming/grpc/CMakeLists.txt
- build.sh
- GRPC_QUICK_START.md
- cpp/src/grpc/client/grpc_client.hpp
- cpp/CMakeLists.txt
- cpp/include/cuopt/linear_programming/cpu_optimization_problem.hpp
🚧 Files skipped from review as they are similar to previous changes (6)
- cpp/src/grpc/server/grpc_server_main.cpp
- cpp/src/grpc/server/grpc_server_types.hpp
- cpp/src/grpc/server/grpc_field_element_size.hpp
- cpp/tests/linear_programming/grpc/grpc_pipe_serialization_test.cpp
- GRPC_SERVER_ARCHITECTURE.md
- GRPC_INTERFACE.md
Comment on lines
+1166
to
+1171
| f = parse_field(entry) | ||
| getter = f.get("getter", f"get_{f['name']}_host()") | ||
| size_getter = getter.replace("_host()", "_size()") | ||
| lines.append( | ||
| f"{ind}est += static_cast<size_t>(solution.{size_getter}) * sizeof(double);" | ||
| ) |
There was a problem hiding this comment.
Size getter derivation relies on naming convention.
The transformation getter.replace("_host()", "_size()") at line 1168 assumes all host getters follow the *_host() pattern with a corresponding *_size() method. If a field uses a custom getter (via the getter YAML key), this replacement may produce an invalid method name.
Consider adding explicit size_getter support in the YAML schema or validating the derived name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/codegen/generate_conversions.py` around lines 1166 - 1171, The current
logic in parse_field/getter derives size_getter by blindly doing
getter.replace("_host()", "_size()"), which breaks when a field supplies a
custom getter via the YAML `getter` key; update the code to first check if the
field dict `f` contains an explicit `size_getter` (or `size_getter` YAML key)
and use that when present, otherwise validate that `getter` ends with "_host()"
before performing the replacement and raise a clear error if not; change the
variable referenced in lines.append to use the chosen `size_getter` (as used
now) and ensure parse_field or the calling site documents/accepts the new
`size_getter` key so custom getters are handled safely.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/codegen/README.md`:
- Line 32: The markdown fenced code block opened at Line 32 in
cpp/codegen/README.md lacks a language tag causing markdownlint MD040; update
that opening fence to include a language identifier (e.g., "text" or "bash") so
the block is fenced as a specific language, keeping the rest of the block
content unchanged to satisfy the linter.
🪄 Autofix (Beta)
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: Pro
Run ID: 17775126-41c3-4556-9e2a-32c0c23ee5fb
📒 Files selected for processing (9)
build.shci/test_cpp.shconda/environments/all_cuda-129_arch-aarch64.yamlconda/environments/all_cuda-129_arch-x86_64.yamlconda/environments/all_cuda-131_arch-aarch64.yamlconda/environments/all_cuda-131_arch-x86_64.yamlconda/recipes/libcuopt/recipe.yamlcpp/codegen/README.mddependencies.yaml
✅ Files skipped from review due to trivial changes (1)
- conda/recipes/libcuopt/recipe.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- build.sh
|
|
||
| ## File Layout | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced block at Line 32.
Line 32 opens a fenced code block without a language, which triggers markdownlint MD040.
Suggested fix
-```
+```text
cpp/codegen/
├── field_registry.yaml # Source of truth for all fields
...
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/codegen/README.md` at line 32, The markdown fenced code block opened at
Line 32 in cpp/codegen/README.md lacks a language tag causing markdownlint
MD040; update that opening fence to include a language identifier (e.g., "text"
or "bash") so the block is fenced as a specific language, keeping the rest of
the block content unchanged to satisfy the linter.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
cpp/codegen/generate_conversions.py (1)
1165-1171:⚠️ Potential issue | 🟡 MinorDon’t derive the size accessor by blind string replacement.
getter.replace("_host()", "_size()")only works for accessors that happen to follow that suffix convention. A valid YAML override with a different getter shape will emit bad generated code here. Either support an explicitsize_getteroverride or fail fast when the getter does not end in_host().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/codegen/generate_conversions.py` around lines 1165 - 1171, The code generating size accessors currently derives size_getter by doing getter.replace("_host()", "_size()"), which silently produces invalid code for custom getter overrides; update generate_conversions.py to either read an explicit size_getter from the parsed field (via parse_field result) when present or validate the computed getter: check that the computed getter string (variable getter from parse_field) endswith("_host()") and if not raise an explicit error/exception (fail fast) explaining that a size_getter override is required; adjust the loop over obj.get("arrays", []) where getter and size_getter are computed and where lines.append(...) is emitted to use the explicit size_getter or to abort with a clear message referencing parse_field, getter, and size_getter.
🧹 Nitpick comments (1)
conda/environments/all_cuda-131_arch-x86_64.yaml (1)
63-64: Redundantpyyamlentries: unpinned and pinned versions coexist.The generated file now contains both
pyyaml(unpinned) andpyyaml>=6.0.0. While conda will resolve this correctly to>=6.0.0, the redundancy is a minor code smell. Per context snippet 1-3, this arises becausebuild_cpp/test_cppgroups independencies.yamldefine unpinnedpyyamlwhilerun_cuoptdefines the pinned version.Consider using the
&pyyamlanchor (already defined inrun_cuopt) in thebuild_cppandtest_cppgroups independencies.yamlto emit a single consistent entry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@conda/environments/all_cuda-131_arch-x86_64.yaml` around lines 63 - 64, The conda env has redundant pyyaml entries; update dependencies.yaml so build_cpp and test_cpp use the existing &pyyaml anchor from run_cuopt (rather than listing an unpinned "pyyaml") so the generated all_cuda-131...yaml emits only the pinned "pyyaml>=6.0.0" entry; modify the build_cpp and test_cpp group entries to reference &pyyaml to ensure a single consistent pyyaml declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ci/verify_codegen.sh`:
- Around line 43-48: The verification currently skips when the generated tmp
proto exists but PROTO_DEST (cpp/src/grpc/cuopt_remote_data.proto) is missing;
change verify_codegen.sh so that if "${TMPDIR}/cuopt_remote_data.proto" exists
but "${PROTO_DEST}" does not, the script prints a missing-file error and sets
FAILED=1. Specifically, in the block referencing
"${TMPDIR}/cuopt_remote_data.proto" and "${PROTO_DEST}", add an explicit check
for the absence of PROTO_DEST (or invert the logic) to emit "MISMATCH:
cpp/src/grpc/cuopt_remote_data.proto (not copied from codegen)" and set FAILED=1
instead of silently skipping.
- Around line 28-41: The current loop over TMPDIR uses diff piped to head which
fails under set -o pipefail and aborts the script on first mismatch and it also
never detects stale files in GENERATED_DIR; modify the comparison so the diff
output is captured without letting the pipe exit non-zero terminate the loop
(e.g., run diff -u "${committed}" "$f" > some buffer or use diff ... | head -30
|| true) and explicitly set FAILED=1 when a mismatch is detected, and add a
second pass that iterates over files in "${GENERATED_DIR}" to flag any files
that do not have a counterpart in "${TMPDIR}" (use the same basename comparison
used for fname) so stale generated files are reported; update references to
TMPDIR, GENERATED_DIR, FAILED, and the diff invocation accordingly.
In `@cpp/codegen/generate_conversions.py`:
- Around line 2054-2076: The current _registry_has_field_numbers only detects if
any field_num/array_id exists but we must instead reject registries that are
partially numbered (some entries numbered, some None) unless the auto-numbering
flag is enabled; update _registry_has_field_numbers (and the analogous check
around the block referenced at lines 2320-2326) to scan each
section/scalars/arrays and settings (use _collect_field_nums and
_collect_settings_field_nums) and return a tri-state or raise/report an error
when mixed presence is found — i.e., if any numeric IDs exist but any entry
lacks a number, fail the generation unless the --auto-number option is active;
ensure checks inspect both top-level sections and warm_start subsections and
reference function names _registry_has_field_numbers, _collect_field_nums,
_collect_settings_field_nums, and the auto-number flag handler to gate the
rejection.
- Around line 1319-1349: The current unary setter group only checks the first
field's presence before calling the multi-array setter (using first_pname and
pb_problem.{first_pname}_size()), which allows partial/malformed data to pass;
change the guard to require that every non-None grouped field has data (i.e.,
pb_problem.{pname}_size() > 0 for all fields in fields that are not None) before
constructing the vectors and invoking cpu_problem.{setter_name}(...), mirroring
the chunked-path validation; update the if condition that uses first_pname to a
combined all(...) style check over each field's proto size so the unary
generator enforces presence of all member arrays.
In `@cpp/tests/linear_programming/grpc/grpc_client_test.cpp`:
- Around line 1750-1818: The test
SettingsMapperTest.MIPSettingsRoundTrip_AllFields omits the presolver field; set
original.presolver to a non-default presolver_t value (e.g., cast from an int
like 2) before mapping and add an EXPECT_EQ comparing restored.presolver to
original.presolver after map_proto_to_mip_settings; update the assertions in
this test to cover presolver so map_mip_settings_to_proto and
map_proto_to_mip_settings are exercised for the presolver conversion.
In `@docs/cuopt/grpc/GRPC_CODE_GENERATION.md`:
- Around line 20-24: The doc currently states "It runs automatically before
every `libcuopt` or `cuopt_grpc_server` build via `build.sh`", which is
misleading; change the wording to describe regeneration as an explicit
manual/codegen workflow step (e.g., "Regenerate the gRPC code by running the
generator as a separate `codegen` step or via `build.sh codegen` before
committing generated files") and update the related paragraphs (including the
block around lines 75–79) to instruct contributors to run the generator after
editing YAML and to re-run the codegen step when reviewing generated files;
mention the optional dependencies (`--auto-number`/`--strip` require
`ruamel.yaml`) and the runtime dependency (`PyYAML`) so readers know
prerequisites.
In `@docs/cuopt/grpc/GRPC_INTERFACE.md`:
- Around line 42-49: The docs incorrectly claim ChunkedProblemHeader carries
per-array metadata (ArrayDescriptor); update the description for
StartChunkedUpload/ChunkedProblemHeader to say it only includes scalar fields
(problem-level metadata) and not per-array descriptors, and change the bullet
about per-array metadata to reference ArrayChunk (which contains field_id,
element_offset, total_elements) as the place implementers should read those
fields; keep mention that the server reports max_message_bytes and that
FinishChunkedUpload triggers reassembly/job submission.
- Line 25: Add the missing language tag to the fenced code blocks that contain
the ASCII protocol diagrams in GRPC_INTERFACE.md so markdownlint MD040 stops
firing: change the opening triple-backticks for each ASCII diagram block to
include the language tag "text" (i.e., ```text) for both diagram fences
referenced in the comment.
---
Duplicate comments:
In `@cpp/codegen/generate_conversions.py`:
- Around line 1165-1171: The code generating size accessors currently derives
size_getter by doing getter.replace("_host()", "_size()"), which silently
produces invalid code for custom getter overrides; update
generate_conversions.py to either read an explicit size_getter from the parsed
field (via parse_field result) when present or validate the computed getter:
check that the computed getter string (variable getter from parse_field)
endswith("_host()") and if not raise an explicit error/exception (fail fast)
explaining that a size_getter override is required; adjust the loop over
obj.get("arrays", []) where getter and size_getter are computed and where
lines.append(...) is emitted to use the explicit size_getter or to abort with a
clear message referencing parse_field, getter, and size_getter.
---
Nitpick comments:
In `@conda/environments/all_cuda-131_arch-x86_64.yaml`:
- Around line 63-64: The conda env has redundant pyyaml entries; update
dependencies.yaml so build_cpp and test_cpp use the existing &pyyaml anchor from
run_cuopt (rather than listing an unpinned "pyyaml") so the generated
all_cuda-131...yaml emits only the pinned "pyyaml>=6.0.0" entry; modify the
build_cpp and test_cpp group entries to reference &pyyaml to ensure a single
consistent pyyaml declaration.
🪄 Autofix (Beta)
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: Pro
Run ID: 1b2786bd-eec5-455b-bf3e-e0d02c74e8ba
⛔ Files ignored due to path filters (5)
cpp/codegen/generated/generated_build_array_chunks.incis excluded by!**/generated/**cpp/codegen/generated/generated_chunked_arrays_to_problem.incis excluded by!**/generated/**cpp/codegen/generated/generated_estimate_problem_size.incis excluded by!**/generated/**cpp/codegen/generated/generated_problem_to_proto.incis excluded by!**/generated/**cpp/codegen/generated/generated_proto_to_problem.incis excluded by!**/generated/**
📒 Files selected for processing (14)
GRPC_INTERFACE.mdci/verify_codegen.shconda/environments/all_cuda-129_arch-aarch64.yamlconda/environments/all_cuda-129_arch-x86_64.yamlconda/environments/all_cuda-131_arch-aarch64.yamlconda/environments/all_cuda-131_arch-x86_64.yamlcpp/CMakeLists.txtcpp/codegen/field_registry.yamlcpp/codegen/generate_conversions.pycpp/tests/linear_programming/grpc/CMakeLists.txtcpp/tests/linear_programming/grpc/grpc_client_test.cppdependencies.yamldocs/cuopt/grpc/GRPC_CODE_GENERATION.mddocs/cuopt/grpc/GRPC_INTERFACE.md
💤 Files with no reviewable changes (1)
- GRPC_INTERFACE.md
✅ Files skipped from review due to trivial changes (3)
- conda/environments/all_cuda-129_arch-aarch64.yaml
- conda/environments/all_cuda-131_arch-aarch64.yaml
- cpp/tests/linear_programming/grpc/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (3)
- conda/environments/all_cuda-129_arch-x86_64.yaml
- cpp/CMakeLists.txt
- dependencies.yaml
Comment on lines
+28
to
+41
| for f in "${TMPDIR}"/*; do | ||
| fname=$(basename "$f") | ||
| committed="${GENERATED_DIR}/${fname}" | ||
| if [ ! -f "${committed}" ]; then | ||
| echo "MISSING: ${committed} (new generated file not committed)" | ||
| FAILED=1 | ||
| continue | ||
| fi | ||
| if ! diff -q "$f" "${committed}" > /dev/null 2>&1; then | ||
| echo "MISMATCH: cpp/codegen/generated/${fname}" | ||
| diff -u "${committed}" "$f" | head -30 | ||
| FAILED=1 | ||
| fi | ||
| done |
There was a problem hiding this comment.
Make the directory comparison exhaustive under pipefail.
diff -u "${committed}" "$f" | head -30 returns non-zero on a real diff when set -o pipefail is enabled, so this block exits on the first mismatch before FAILED=1 is recorded or the rest of the generated files are checked. The loop also only walks ${TMPDIR}, so stale files that still exist in cpp/codegen/generated/ are never reported.
Suggested fix
for f in "${TMPDIR}"/*; do
fname=$(basename "$f")
committed="${GENERATED_DIR}/${fname}"
if [ ! -f "${committed}" ]; then
echo "MISSING: ${committed} (new generated file not committed)"
FAILED=1
continue
fi
if ! diff -q "$f" "${committed}" > /dev/null 2>&1; then
echo "MISMATCH: cpp/codegen/generated/${fname}"
- diff -u "${committed}" "$f" | head -30
+ diff -u "${committed}" "$f" | head -30 || true
FAILED=1
fi
done
+
+for committed in "${GENERATED_DIR}"/*; do
+ fname=$(basename "${committed}")
+ if [ ! -f "${TMPDIR}/${fname}" ]; then
+ echo "STALE: ${committed} (committed file is no longer generated)"
+ FAILED=1
+ fi
+done📝 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
| for f in "${TMPDIR}"/*; do | |
| fname=$(basename "$f") | |
| committed="${GENERATED_DIR}/${fname}" | |
| if [ ! -f "${committed}" ]; then | |
| echo "MISSING: ${committed} (new generated file not committed)" | |
| FAILED=1 | |
| continue | |
| fi | |
| if ! diff -q "$f" "${committed}" > /dev/null 2>&1; then | |
| echo "MISMATCH: cpp/codegen/generated/${fname}" | |
| diff -u "${committed}" "$f" | head -30 | |
| FAILED=1 | |
| fi | |
| done | |
| for f in "${TMPDIR}"/*; do | |
| fname=$(basename "$f") | |
| committed="${GENERATED_DIR}/${fname}" | |
| if [ ! -f "${committed}" ]; then | |
| echo "MISSING: ${committed} (new generated file not committed)" | |
| FAILED=1 | |
| continue | |
| fi | |
| if ! diff -q "$f" "${committed}" > /dev/null 2>&1; then | |
| echo "MISMATCH: cpp/codegen/generated/${fname}" | |
| diff -u "${committed}" "$f" | head -30 || true | |
| FAILED=1 | |
| fi | |
| done | |
| for committed in "${GENERATED_DIR}"/*; do | |
| fname=$(basename "${committed}") | |
| if [ ! -f "${TMPDIR}/${fname}" ]; then | |
| echo "STALE: ${committed} (committed file is no longer generated)" | |
| FAILED=1 | |
| fi | |
| done |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ci/verify_codegen.sh` around lines 28 - 41, The current loop over TMPDIR uses
diff piped to head which fails under set -o pipefail and aborts the script on
first mismatch and it also never detects stale files in GENERATED_DIR; modify
the comparison so the diff output is captured without letting the pipe exit
non-zero terminate the loop (e.g., run diff -u "${committed}" "$f" > some buffer
or use diff ... | head -30 || true) and explicitly set FAILED=1 when a mismatch
is detected, and add a second pass that iterates over files in
"${GENERATED_DIR}" to flag any files that do not have a counterpart in
"${TMPDIR}" (use the same basename comparison used for fname) so stale generated
files are reported; update references to TMPDIR, GENERATED_DIR, FAILED, and the
diff invocation accordingly.
Comment on lines
+43
to
+48
| if [ -f "${TMPDIR}/cuopt_remote_data.proto" ] && [ -f "${PROTO_DEST}" ]; then | ||
| if ! diff -q "${TMPDIR}/cuopt_remote_data.proto" "${PROTO_DEST}" > /dev/null 2>&1; then | ||
| echo "MISMATCH: cpp/src/grpc/cuopt_remote_data.proto (not copied from codegen)" | ||
| FAILED=1 | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Fail when the copied proto is missing.
generate_conversions.py always writes cuopt_remote_data.proto into ${TMPDIR}, so the only time this block skips is when cpp/src/grpc/cuopt_remote_data.proto is missing. That should fail CI like any other missing generated artifact; otherwise deleting the copied proto passes verification silently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ci/verify_codegen.sh` around lines 43 - 48, The verification currently skips
when the generated tmp proto exists but PROTO_DEST
(cpp/src/grpc/cuopt_remote_data.proto) is missing; change verify_codegen.sh so
that if "${TMPDIR}/cuopt_remote_data.proto" exists but "${PROTO_DEST}" does not,
the script prints a missing-file error and sets FAILED=1. Specifically, in the
block referencing "${TMPDIR}/cuopt_remote_data.proto" and "${PROTO_DEST}", add
an explicit check for the absence of PROTO_DEST (or invert the logic) to emit
"MISMATCH: cpp/src/grpc/cuopt_remote_data.proto (not copied from codegen)" and
set FAILED=1 instead of silently skipping.
Comment on lines
+1319
to
+1349
| # Setter groups — guard on first field having data | ||
| for gname, gdef in setter_groups.items(): | ||
| setter_name = gdef["setter"] | ||
| fields = [ | ||
| _find_problem_field(obj, fn) for fn in gdef.get("fields", []) | ||
| ] | ||
| first = next((f for f in fields if f is not None), None) | ||
| if first is None: | ||
| continue | ||
| first_pname = _proto_cpp_name(first["name"]) | ||
| lines.append(f"{ind}if (pb_problem.{first_pname}_size() > 0) {{") | ||
| ii = ind + " " | ||
|
|
||
| for f in fields: | ||
| if f is None: | ||
| continue | ||
| pname = _proto_cpp_name(f["name"]) | ||
| ftype = f.get("type", "repeated double") | ||
| cpp_t = _array_cpp_type(ftype) | ||
| lines.append( | ||
| f"{ii}std::vector<{cpp_t}> {f['name']}(pb_problem.{pname}().begin(), pb_problem.{pname}().end());" | ||
| ) | ||
|
|
||
| args = [] | ||
| for f in fields: | ||
| if f is None: | ||
| continue | ||
| args.append(f"{f['name']}.data()") | ||
| args.append(f"static_cast<i_t>({f['name']}.size())") | ||
| lines.append(f"{ii}cpu_problem.{setter_name}({', '.join(args)});") | ||
| lines.append(f"{ind}}}") |
There was a problem hiding this comment.
Unary setter groups should require all member arrays.
This path only checks the first grouped field before calling the multi-array setter. A malformed unary OptimizationProblem with A_values present but A_indices / A_offsets missing will still call set_csr_constraint_matrix(...) with partial data. The chunked path later in this file already guards on every grouped field, so the unary generator should enforce the same rule.
Suggested fix
- first_pname = _proto_cpp_name(first["name"])
- lines.append(f"{ind}if (pb_problem.{first_pname}_size() > 0) {{")
+ guard_parts = [
+ f"pb_problem.{_proto_cpp_name(f['name'])}_size() > 0"
+ for f in fields
+ if f is not None
+ ]
+ lines.append(f"{ind}if ({' && '.join(guard_parts)}) {{")As per coding guidelines, "**/*.{cpp,hpp,h}: Validate input sanitization to prevent buffer overflows and resource exhaustion attacks; avoid unsafe deserialization of problem files".
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 1320-1320: Loop control variable gname not used within loop body
Rename unused gname to _gname
(B007)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/codegen/generate_conversions.py` around lines 1319 - 1349, The current
unary setter group only checks the first field's presence before calling the
multi-array setter (using first_pname and pb_problem.{first_pname}_size()),
which allows partial/malformed data to pass; change the guard to require that
every non-None grouped field has data (i.e., pb_problem.{pname}_size() > 0 for
all fields in fields that are not None) before constructing the vectors and
invoking cpu_problem.{setter_name}(...), mirroring the chunked-path validation;
update the if condition that uses first_pname to a combined all(...) style check
over each field's proto size so the unary generator enforces presence of all
member arrays.
Comment on lines
+2054
to
+2076
| def _registry_has_field_numbers(registry): | ||
| """Check if the registry has at least one field_num or array_id assigned.""" | ||
| for section_key in list(registry.keys()): | ||
| section = registry.get(section_key) | ||
| if not isinstance(section, dict): | ||
| continue | ||
| for list_key in ["scalars", "arrays"]: | ||
| for nums in [ | ||
| _collect_field_nums(section.get(list_key, []), "field_num"), | ||
| _collect_field_nums(section.get(list_key, []), "array_id"), | ||
| ]: | ||
| if nums: | ||
| return True | ||
| if "fields" in section: | ||
| if _collect_settings_field_nums(section["fields"]): | ||
| return True | ||
| sub = section.get("warm_start") | ||
| if isinstance(sub, dict): | ||
| for list_key in ["scalars", "arrays"]: | ||
| for key in ["field_num", "array_id"]: | ||
| if _collect_field_nums(sub.get(list_key, []), key): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
Reject partially numbered registries before generating.
The preflight only checks whether any field_num / array_id exists. Every generator below silently skips entries whose numbers are None, so adding one unnumbered field produces incomplete .proto / .inc outputs with no error. This should fail unless --auto-number is being used.
Also applies to: 2320-2326
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/codegen/generate_conversions.py` around lines 2054 - 2076, The current
_registry_has_field_numbers only detects if any field_num/array_id exists but we
must instead reject registries that are partially numbered (some entries
numbered, some None) unless the auto-numbering flag is enabled; update
_registry_has_field_numbers (and the analogous check around the block referenced
at lines 2320-2326) to scan each section/scalars/arrays and settings (use
_collect_field_nums and _collect_settings_field_nums) and return a tri-state or
raise/report an error when mixed presence is found — i.e., if any numeric IDs
exist but any entry lacks a number, fail the generation unless the --auto-number
option is active; ensure checks inspect both top-level sections and warm_start
subsections and reference function names _registry_has_field_numbers,
_collect_field_nums, _collect_settings_field_nums, and the auto-number flag
handler to gate the rejection.
Comment on lines
+1750
to
+1818
| TEST(SettingsMapperTest, MIPSettingsRoundTrip_AllFields) | ||
| { | ||
| mip_solver_settings_t<int32_t, double> original; | ||
| original.time_limit = 42.5; | ||
| original.tolerances.relative_mip_gap = 0.01; | ||
| original.tolerances.absolute_mip_gap = 1e-8; | ||
| original.tolerances.integrality_tolerance = 1e-6; | ||
| original.tolerances.absolute_tolerance = 1e-7; | ||
| original.tolerances.relative_tolerance = 1e-13; | ||
| original.tolerances.presolve_absolute_tolerance = 1e-5; | ||
| original.log_to_console = false; | ||
| original.heuristics_only = true; | ||
| original.num_cpu_threads = 8; | ||
| original.num_gpus = 2; | ||
| original.mip_scaling = true; | ||
|
|
||
| original.work_limit = 100.0; | ||
| original.node_limit = 5000; | ||
| original.reliability_branching = 3; | ||
| original.mip_batch_pdlp_strong_branching = 1; | ||
| original.max_cut_passes = 20; | ||
| original.mir_cuts = 2; | ||
| original.mixed_integer_gomory_cuts = 1; | ||
| original.knapsack_cuts = 0; | ||
| original.clique_cuts = 3; | ||
| original.strong_chvatal_gomory_cuts = -1; | ||
| original.reduced_cost_strengthening = 1; | ||
| original.cut_change_threshold = 0.05; | ||
| original.cut_min_orthogonality = 0.3; | ||
| original.determinism_mode = 1; | ||
| original.seed = 12345; | ||
|
|
||
| cuopt::remote::MIPSolverSettings proto; | ||
| map_mip_settings_to_proto(original, &proto); | ||
|
|
||
| mip_solver_settings_t<int32_t, double> restored; | ||
| map_proto_to_mip_settings(proto, restored); | ||
|
|
||
| EXPECT_DOUBLE_EQ(restored.time_limit, original.time_limit); | ||
| EXPECT_DOUBLE_EQ(restored.tolerances.relative_mip_gap, original.tolerances.relative_mip_gap); | ||
| EXPECT_DOUBLE_EQ(restored.tolerances.absolute_mip_gap, original.tolerances.absolute_mip_gap); | ||
| EXPECT_DOUBLE_EQ(restored.tolerances.integrality_tolerance, | ||
| original.tolerances.integrality_tolerance); | ||
| EXPECT_DOUBLE_EQ(restored.tolerances.absolute_tolerance, original.tolerances.absolute_tolerance); | ||
| EXPECT_DOUBLE_EQ(restored.tolerances.relative_tolerance, original.tolerances.relative_tolerance); | ||
| EXPECT_DOUBLE_EQ(restored.tolerances.presolve_absolute_tolerance, | ||
| original.tolerances.presolve_absolute_tolerance); | ||
| EXPECT_EQ(restored.log_to_console, original.log_to_console); | ||
| EXPECT_EQ(restored.heuristics_only, original.heuristics_only); | ||
| EXPECT_EQ(restored.num_cpu_threads, original.num_cpu_threads); | ||
| EXPECT_EQ(restored.num_gpus, original.num_gpus); | ||
| EXPECT_EQ(restored.mip_scaling, original.mip_scaling); | ||
|
|
||
| EXPECT_DOUBLE_EQ(restored.work_limit, original.work_limit); | ||
| EXPECT_EQ(restored.node_limit, original.node_limit); | ||
| EXPECT_EQ(restored.reliability_branching, original.reliability_branching); | ||
| EXPECT_EQ(restored.mip_batch_pdlp_strong_branching, original.mip_batch_pdlp_strong_branching); | ||
| EXPECT_EQ(restored.max_cut_passes, original.max_cut_passes); | ||
| EXPECT_EQ(restored.mir_cuts, original.mir_cuts); | ||
| EXPECT_EQ(restored.mixed_integer_gomory_cuts, original.mixed_integer_gomory_cuts); | ||
| EXPECT_EQ(restored.knapsack_cuts, original.knapsack_cuts); | ||
| EXPECT_EQ(restored.clique_cuts, original.clique_cuts); | ||
| EXPECT_EQ(restored.strong_chvatal_gomory_cuts, original.strong_chvatal_gomory_cuts); | ||
| EXPECT_EQ(restored.reduced_cost_strengthening, original.reduced_cost_strengthening); | ||
| EXPECT_DOUBLE_EQ(restored.cut_change_threshold, original.cut_change_threshold); | ||
| EXPECT_DOUBLE_EQ(restored.cut_min_orthogonality, original.cut_min_orthogonality); | ||
| EXPECT_EQ(restored.determinism_mode, original.determinism_mode); | ||
| EXPECT_EQ(restored.seed, original.seed); | ||
| } |
There was a problem hiding this comment.
MIPSettingsRoundTrip_AllFields is missing presolver.
This test never sets or asserts the presolver round-trip, even though the generated MIP mapper has a dedicated int32 ↔ presolver_t cast path. A regression there would slip through while this test still advertises “AllFields”.
Suggested fix
TEST(SettingsMapperTest, MIPSettingsRoundTrip_AllFields)
{
mip_solver_settings_t<int32_t, double> original;
original.time_limit = 42.5;
@@
original.num_cpu_threads = 8;
original.num_gpus = 2;
+ original.presolver = presolver_t::Papilo;
original.mip_scaling = true;
@@
EXPECT_EQ(restored.num_cpu_threads, original.num_cpu_threads);
EXPECT_EQ(restored.num_gpus, original.num_gpus);
+ EXPECT_EQ(static_cast<int>(restored.presolver), static_cast<int>(original.presolver));
EXPECT_EQ(restored.mip_scaling, original.mip_scaling);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/tests/linear_programming/grpc/grpc_client_test.cpp` around lines 1750 -
1818, The test SettingsMapperTest.MIPSettingsRoundTrip_AllFields omits the
presolver field; set original.presolver to a non-default presolver_t value
(e.g., cast from an int like 2) before mapping and add an EXPECT_EQ comparing
restored.presolver to original.presolver after map_proto_to_mip_settings; update
the assertions in this test to cover presolver so map_mip_settings_to_proto and
map_proto_to_mip_settings are exercised for the presolver conversion.
Comment on lines
+20
to
+24
| The generator runs in ~100ms with no external dependencies beyond PyYAML (ships | ||
| with conda). The `--auto-number` and `--strip` options additionally require | ||
| `ruamel.yaml` (listed in the project's development dependencies). It runs | ||
| automatically before every `libcuopt` or `cuopt_grpc_server` build via | ||
| `build.sh`. |
There was a problem hiding this comment.
Describe regeneration as an explicit step, not an automatic one.
This says the generator runs before every libcuopt / cuopt_grpc_server build, but the workflow in this PR is “edit YAML → rerun codegen → review generated files”, and the build integration is a dedicated codegen step. Leaving it this way will make contributors assume a normal build refreshes committed outputs when it doesn’t.
As per coding guidelines, "docs/**/*: For documentation changes, focus on: Accuracy: Verify code examples compile and run correctly".
Also applies to: 75-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/cuopt/grpc/GRPC_CODE_GENERATION.md` around lines 20 - 24, The doc
currently states "It runs automatically before every `libcuopt` or
`cuopt_grpc_server` build via `build.sh`", which is misleading; change the
wording to describe regeneration as an explicit manual/codegen workflow step
(e.g., "Regenerate the gRPC code by running the generator as a separate
`codegen` step or via `build.sh codegen` before committing generated files") and
update the related paragraphs (including the block around lines 75–79) to
instruct contributors to run the generator after editing YAML and to re-run the
codegen step when reviewing generated files; mention the optional dependencies
(`--auto-number`/`--strip` require `ruamel.yaml`) and the runtime dependency
(`PyYAML`) so readers know prerequisites.
| When the estimated serialized problem size exceeds 75% of `max_message_bytes`, the client | ||
| splits large arrays into chunks and sends them via multiple unary RPCs: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add language tags to the diagram fences.
markdownlint MD040 is firing on both fenced blocks. text is enough for these ASCII protocol diagrams.
Also applies to: 56-56
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/cuopt/grpc/GRPC_INTERFACE.md` at line 25, Add the missing language tag
to the fenced code blocks that contain the ASCII protocol diagrams in
GRPC_INTERFACE.md so markdownlint MD040 stops firing: change the opening
triple-backticks for each ASCII diagram block to include the language tag "text"
(i.e., ```text) for both diagram fences referenced in the comment.
Comment on lines
+42
to
+49
| **Key features:** | ||
| - `StartChunkedUpload` sends a `ChunkedProblemHeader` with all scalar fields and | ||
| array metadata (`ArrayDescriptor` for each large array: field ID, total elements, | ||
| element size) | ||
| - Each `SendArrayChunk` carries one chunk of one array, identified by `ArrayFieldId` | ||
| and `element_offset` | ||
| - The server reports `max_message_bytes` so the client can adapt chunk sizing | ||
| - `FinishChunkedUpload` triggers server-side reassembly and job submission |
There was a problem hiding this comment.
ChunkedProblemHeader doesn’t carry array descriptors.
The current wording says StartChunkedUpload includes per-array metadata in the header, but the proto places field_id, element_offset, and total_elements on ArrayChunk, not on ChunkedProblemHeader. As written, this points implementers at the wrong message when they wire upload support.
As per coding guidelines, "docs/**/*: For documentation changes, focus on: Accuracy: Verify code examples compile and run correctly".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/cuopt/grpc/GRPC_INTERFACE.md` around lines 42 - 49, The docs incorrectly
claim ChunkedProblemHeader carries per-array metadata (ArrayDescriptor); update
the description for StartChunkedUpload/ChunkedProblemHeader to say it only
includes scalar fields (problem-level metadata) and not per-array descriptors,
and change the bullet about per-array metadata to reference ArrayChunk (which
contains field_id, element_offset, total_elements) as the place implementers
should read those fields; keep mention that the server reports max_message_bytes
and that FinishChunkedUpload triggers reassembly/job submission.
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
1 participant
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.
This change adds a code generator written in Python which generates the protobuf specifications and the code to map between cuopt problem, solution, and settings structures and protobuf messages. The protoc compiler is still used to build the serialization/deserialziation code and the grpc interface -- only the high-level .proto files are generated from the yaml description.
The code generator reads a lightweight yaml file which describes the cuopt structures and generates mapping functions which are included in the grpc client and server implementations.
When a field is added/changed in the problem, solution, or settings structures, a developer updates the yaml file and re-runs the code generator, then reviews the updated code.
A README.md file in the codegen directory describes the yaml format, defaults, and conventions. As much as possible has been left to defaults or naming conventions to make the yaml easy to maintain.