Add VRP dimension developer skill - #1233
Conversation
rg20
added
Agentic
📝 WalkthroughWalkthroughThis PR adds developer documentation for implementing VRP dimensions in cuOpt. A new ChangesVRP Dimension Implementation Guide
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (2)
skills/cuopt-developer/vrp_skills.md (2)
20-22: ⚡ Quick winConsider adding language specifiers to code blocks.
The pseudo-code blocks at lines 20-22, 52-57, 64-67, 71-74, and 78-81 lack language specifiers. Adding identifiers (e.g.,
pseudo,python, orcpp) improves syntax highlighting and rendering in documentation viewers.📝 Example fix for the combine formula block
-``` +```pseudo combine(k, k+1) = fwd_excess[k] + bwd_excess[k+1] - excess(fwd_state[k])</details> Also applies to: 52-57, 64-67, 71-74, 78-81 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@skills/cuopt-developer/vrp_skills.mdaround lines 20 - 22, Add explicit
language specifiers to each pseudo-code fenced block (e.g., ```pseudo orupdate the blocks that contain the formula "combine(k, k+1) = fwd_excess[k] + bwd_excess[k+1] - excess(fwd_state[k])" and the other pseudo-code blocks referenced at lines 52-57, 64-67, 71-74, and 78-81 by prefixing their opening fences with a language token (pseudo, python, or cpp) while leaving the block contents unchanged.
24-36: ⚡ Quick winAdd blank lines around the interface table.
Markdown best practices require blank lines before and after tables for consistent rendering across parsers.
📝 Proposed fix
### Required interface for every dimension + | Method | Description | |--------|-------------| | `calculate_forward(next)` | Propagate fwd state from `this` to `next`; update `next.fwd_excess` | + ---🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/cuopt-developer/vrp_skills.md` around lines 24 - 36, Add a blank line immediately before the table that starts with "### Required interface for every dimension" and another blank line immediately after the table so Markdown renders it consistently; locate the table by the method names listed (calculate_forward, calculate_backward, combine, get_cost, compute_cost, forward_excess, backward_excess, forward_feasible, backward_feasible) and insert a single empty line above the "### Required interface for every dimension" heading (or between the previous paragraph and the heading) and one empty line after the table before the following content/heading.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@skills/cuopt-developer/vrp_skills.md`:
- Around line 20-22: Add explicit language specifiers to each pseudo-code fenced
block (e.g., ```pseudo or ```python) so documentation renderers apply proper
highlighting; specifically update the blocks that contain the formula
"combine(k, k+1) = fwd_excess[k] + bwd_excess[k+1] - excess(fwd_state[k])" and
the other pseudo-code blocks referenced at lines 52-57, 64-67, 71-74, and 78-81
by prefixing their opening fences with a language token (pseudo, python, or cpp)
while leaving the block contents unchanged.
- Around line 24-36: Add a blank line immediately before the table that starts
with "### Required interface for every dimension" and another blank line
immediately after the table so Markdown renders it consistently; locate the
table by the method names listed (calculate_forward, calculate_backward,
combine, get_cost, compute_cost, forward_excess, backward_excess,
forward_feasible, backward_feasible) and insert a single empty line above the
"### Required interface for every dimension" heading (or between the previous
paragraph and the heading) and one empty line after the table before the
following content/heading.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 60de93ad-54b0-420b-8c46-d19b726de6b6
📒 Files selected for processing (3)
AGENTS.mdskills/cuopt-developer/SKILL.mdskills/cuopt-developer/vrp_skills.md
ramakrishnap-nv
left a comment
ramakrishnap-nv
left a comment
There was a problem hiding this comment.
Awesome @rg20, first developer skill for core work.
I would suggest review from @hlinsen and @akifcorduk as well for any correctness or improvments.
| @@ -0,0 +1,191 @@ | |||
| # cuOpt VRP Dimension Developer Skills | |||
There was a problem hiding this comment.
Lets move this file under reference/
| ### Rules | ||
| - `skills/cuopt-user-rules/` — Base rules for end users calling cuOpt (routing, LP, MILP, QP, install, server). Not for cuOpt internals — see `skills/cuopt-developer/`. Read first for user-facing tasks; choose skills from the index below by task and interface. | ||
| - `skills/cuopt-developer/` — Modify, build, test, debug, and contribute to cuOpt internals (C++/CUDA, Python, server, CI). Use for solver internals, PRs, DCO, and code conventions. | ||
| - `skills/cuopt-developer/` — Modify, build, test, debug, and contribute to cuOpt internals (C++/CUDA, Python, server, CI). Use for solver internals, PRs, DCO, and code conventions. For **VRP dimension** work (combine invariants, fwd/bwd propagation, new constraints/objectives in the routing engine), read **`skills/cuopt-developer/vrp_skills.md`** in addition to `SKILL.md`. |
There was a problem hiding this comment.
Lets update this accordingly
|
|
||
| --- | ||
|
|
||
| ## Skill 1: `cuopt-dimension-architecture` |
There was a problem hiding this comment.
Lets remove Skill X notation, and just use detail, refer to skills/cuopt-developer/resources/build_and_test.md
| ## Skill 1: `cuopt-dimension-architecture` | |
| ## cuopt-dimension-architecture |
There was a problem hiding this comment.
Same for all other instances
|
|
||
| **When to use**: After writing node/route logic, to ensure the dimension is fully integrated into the framework. | ||
|
|
||
| ### Files to create |
There was a problem hiding this comment.
Did it needed specific instruction to follow this structure ?
|
|
||
| **When to use**: After implementing a new dimension, to write tests that validate correctness end-to-end. | ||
|
|
||
| ### C++ unit tests (`cpp/tests/routing/`) |
There was a problem hiding this comment.
Would C++ test structure and expectation be same as python, may be can we collapse it in to 1 ?
akifcorduk
left a comment
akifcorduk
left a comment
There was a problem hiding this comment.
Thanks Rajesh. Approving with some comments.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/cuopt-developer/vrp_skills.md`:
- Around line 69-72: Two fenced code blocks are missing language identifiers;
update the two blocks containing the invariant formula "total = prev.fwd_excess
+ next.bwd_excess - boundary_correction(prev.fwd_state)" and the expression
"get_cost(prev, this) == combine(prev, this)" by annotating their opening
triple-backticks with a language (e.g., use "text") so they become "```text" to
satisfy the MD040 lint rule.
🪄 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: Enterprise
Run ID: 746ff7ca-7b04-43e1-a1f8-7122e7264b4e
📒 Files selected for processing (1)
skills/cuopt-developer/vrp_skills.md
| Write out the invariant formula and verify it equals the total route cost for a complete route: | ||
| ``` | ||
| total = prev.fwd_excess + next.bwd_excess - boundary_correction(prev.fwd_state) | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks.
Two fenced blocks are missing a language, which triggers MD040 warnings. Please annotate them (for example, text) to keep docs lint-clean.
Suggested fix
-```
+```text
total = prev.fwd_excess + next.bwd_excess - boundary_correction(prev.fwd_state)@@
- +text
get_cost(prev, this) == combine(prev, this)
Also applies to: 80-82
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 70-70: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/cuopt-developer/vrp_skills.md` around lines 69 - 72, Two fenced code
blocks are missing language identifiers; update the two blocks containing the
invariant formula "total = prev.fwd_excess + next.bwd_excess -
boundary_correction(prev.fwd_state)" and the expression "get_cost(prev, this) ==
combine(prev, this)" by annotating their opening triple-backticks with a
language (e.g., use "text") so they become "```text" to satisfy the MD040 lint
rule.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
AGENTS.md (1)
11-11: 💤 Low valueRemove bold formatting for consistency with existing path and term formatting.
The use of bold markdown (
**VRP dimension**andpath) is inconsistent with the rest of the file, where paths use only backticks and domain terms are not bolded. For example, lines 10 and 12 use backticks alone for paths.As per coding guidelines, maintain consistent structure and formatting in agent documentation.
📝 Proposed fix to align with existing formatting conventions
-- `skills/cuopt-developer/` — Modify, build, test, debug, and contribute to cuOpt internals (C++/CUDA, Python, server, CI). Use for solver internals, PRs, DCO, and code conventions. For **VRP dimension** work (combine invariants, fwd/bwd propagation, new constraints/objectives in the routing engine), read **`skills/cuopt-developer/resources/vrp_skills.md`** in addition to `SKILL.md`. +- `skills/cuopt-developer/` — Modify, build, test, debug, and contribute to cuOpt internals (C++/CUDA, Python, server, CI). Use for solver internals, PRs, DCO, and code conventions. For VRP dimension work (combine invariants, fwd/bwd propagation, new constraints/objectives in the routing engine), read `skills/cuopt-developer/resources/vrp_skills.md` in addition to `SKILL.md`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AGENTS.md` at line 11, The line containing `skills/cuopt-developer/` uses bold markdown for "VRP dimension" and a path; remove the surrounding ** bold markers so the phrase reads VRP dimension (no bold) and the path remains in backticks (`skills/cuopt-developer/resources/vrp_skills.md`), matching the existing style used for `SKILL.md` and other paths; update the text in that sentence to use plain text for domain terms and backticks only for filesystem paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@AGENTS.md`:
- Line 11: The line containing `skills/cuopt-developer/` uses bold markdown for
"VRP dimension" and a path; remove the surrounding ** bold markers so the phrase
reads VRP dimension (no bold) and the path remains in backticks
(`skills/cuopt-developer/resources/vrp_skills.md`), matching the existing style
used for `SKILL.md` and other paths; update the text in that sentence to use
plain text for domain terms and backticks only for filesystem paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 59966e36-44fd-4b32-85e1-c40ce332f7ad
📒 Files selected for processing (3)
AGENTS.mdskills/cuopt-developer/SKILL.mdskills/cuopt-developer/resources/vrp_skills.md
✅ Files skipped from review due to trivial changes (2)
- skills/cuopt-developer/resources/vrp_skills.md
- skills/cuopt-developer/SKILL.md
|
/ok to test b1da9d0 |
|
/merge |
4 participants
Description
Issue
Checklist