-
Notifications
You must be signed in to change notification settings - Fork 217
docs(skills): add GitHub etiquette rules and evals to cuopt-developer #1619
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
06d62df
80f261e
f3b4af6
cde97e1
2b87348
b641714
0ac412f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,66 @@ | |
| "Mentions committing the regenerated files together with dependencies.yaml" | ||
| ] | ||
| }, | ||
| { | ||
| "id": "dev-eval-004-no-push-to-protected-branch", | ||
| "question": "I just fixed a one-line typo in ci/utils/pr_test_summary.py. It is a tiny change — can I just run 'git push origin main' to get it in quickly?", | ||
| "expected_skill": "cuopt-developer", | ||
| "expected_script": null, | ||
| "ground_truth": "No. Pushing directly to main (or any release/* branch) is never allowed, regardless of how small the change is. main and release/* are protected branches that require a pull request, passing status checks, and DCO sign-off. A direct push bypasses all of these. The correct approach is: (1) Create a feature branch ('git checkout -b fix/short-description'). (2) Commit the change with DCO sign-off ('git commit -s -m \"...\"'). (3) Run pre-commit on the changed file. (4) Push the feature branch to your fork or to the upstream repo (following the fork/upstream rules for the file type changed). (5) Open a draft PR ('gh pr create --draft').", | ||
| "expected_behavior": [ | ||
| "Refuses to push directly to main", | ||
| "States that main and release/* are protected branches requiring a PR", | ||
| "Instructs to create a feature branch first", | ||
| "Mentions DCO sign-off ('git commit -s') on the commit", | ||
| "Mentions running pre-commit before pushing", | ||
| "Instructs to open a draft PR after pushing the feature branch", | ||
| "Does not make an exception for small or trivial changes" | ||
| ] | ||
| }, | ||
| { | ||
| "id": "dev-eval-005-skills-pr-must-use-upstream-branch", | ||
| "question": "I want to update a cuOpt skill file under skills/cuopt-developer/. Should I push the branch to my personal GitHub fork and open a PR from there, the same way I would for a code change?", | ||
| "expected_skill": "cuopt-developer", | ||
| "expected_script": null, | ||
| "ground_truth": "No — skills PRs are an exception to the fork workflow. NVSkills CI validation requires the PR to originate from a branch in the upstream NVIDIA/cuopt repository, not a personal fork. Fork-based PRs are not supported for changes under skills/. The correct approach is: create a feature branch in the upstream repo, push the branch there (not to your fork), and open a PR from that branch. Everything else still applies: never push to main or release/*, use a feature branch, include DCO sign-off, and open a draft PR.", | ||
| "expected_behavior": [ | ||
| "States that the fork workflow does NOT apply to skills/ changes", | ||
| "Explains that NVSkills CI requires the PR to come from a branch in NVIDIA/cuopt", | ||
| "Instructs to push the branch to the upstream repo, not a personal fork", | ||
| "Still requires a feature branch — not pushing directly to main", | ||
| "Still requires DCO sign-off and a draft PR" | ||
| ] | ||
| }, | ||
| { | ||
| "id": "dev-eval-006-add-solver-parameter", | ||
| "question": "I want to expose a new MIP cut toggle called 'flow_cover_cuts' to cuOpt users. The internal solver already reads a field 'flow_cover_cuts' from the MIP settings struct. What are all the steps required to fully expose it through the C API, Python, CLI, gRPC server, and docs — and what is the most common mistake people make when doing this?", | ||
| "expected_skill": "cuopt-developer", | ||
| "expected_script": null, | ||
| "ground_truth": "Since the solver already reads the field, the settings struct layer is done — verify the field and its default exist before proceeding. The remaining five layers still need to be wired, and missing any one silently drops the parameter from that interface. Use a sibling parameter (e.g. 'clique_cuts') as the template and grep for it across the repo to find every location. The steps are: (1) Settings struct — verify 'flow_cover_cuts' exists in cpp/include/cuopt/.../solver_settings.hpp with the correct default (-1 for automatic cut toggles); add it only if absent. (2) C constant — add '#define CUOPT_MIP_FLOW_COVER_CUTS \"mip_flow_cover_cuts\"' in cpp/include/cuopt/linear_programming/constants.h next to related parameters. (3) String-parameter registry — add a tuple to the int_parameters table in cpp/src/math_optimization/solver_settings.cu: {CUOPT_MIP_FLOW_COVER_CUTS, &mip_settings.flow_cover_cuts, min, max, default}. This single registry drives CLI parsing, set_parameter/get_parameter, and Python auto-discovery — no .pyx change is needed. (4) gRPC/server — add the field under the right message in cpp/src/grpc/codegen/field_registry.yaml with the next free field_num, then regenerate with 'python cpp/src/grpc/codegen/generate_conversions.py'. The generated .proto and .inc files are auto-generated — never hand-edit them. (5) Docs — add a doxygendefine directive to the C API rst, a settings section to mip-settings.rst, and the server schema in cuopt_spec.yaml. (6) Tests — add C++ and Python coverage. The most common mistake is hand-editing the auto-generated proto or .inc files instead of running the codegen script, or adding the constant but forgetting to add the registry tuple (which means the parameter is defined but silently ignored).", | ||
| "expected_behavior": [ | ||
| "Recognizes the settings struct field already exists and says to verify it rather than unconditionally adding it", | ||
| "Identifies the remaining five layers to wire: C constant, string-parameter registry, gRPC codegen, docs, tests", | ||
| "States the string-parameter registry in solver_settings.cu is the single source driving CLI, set_parameter/get_parameter, and Python — no .pyx change needed", | ||
| "States that the gRPC .proto and .inc files are auto-generated and must NOT be hand-edited; the codegen script must be run", | ||
| "Recommends using a sibling parameter (e.g. clique_cuts) as a template and grepping for it", | ||
| "Names the most common mistake: hand-editing generated files or adding the constant but omitting the registry tuple" | ||
| ] | ||
| }, | ||
| { | ||
| "id": "dev-eval-007-merge-conflict-resolution", | ||
| "question": "I am resolving a merge conflict in a C++ file. Branch A added a new field 'pending_tasks_' of type 'std::atomic<int>' to a class and Branch B refactored the same class to use an OpenMP task model and removed that field entirely. The conflict markers show Branch A's version has more lines and looks like a superset. Should I just pick Branch A's hunk since it seems more complete?", | ||
| "expected_skill": "cuopt-developer", | ||
| "expected_script": null, | ||
| "ground_truth": "No — picking the 'bigger hunk' is the wrong approach and will likely produce a broken merge that compiles but silently misbehaves. The correct approach is to reconstruct what each branch actually did before choosing a side. Diff the conflicting symbols across both branches and the merge base ('git show <branch>:<path>' and 'git merge-base A B') — not just the conflict hunks. Check how already-merged non-conflicted callers use the symbol — if a caller was auto-merged to Branch B's signature, the conflicted file must conform to Branch B. The resolution depends on what the inspection reveals: if Branch B's OpenMP refactor is the intended baseline (e.g. callers already use the new model), the correct resolution is to adopt Branch B's removal and re-port any genuinely new functionality from Branch A onto the new mechanism. Keeping Branch A's 'std::atomic<int>' field alongside Branch B's OpenMP model would leave a member that is never set and a synchronization guard that never fires. If the inspection instead shows Branch A is the intended baseline, re-port Branch B's changes onto it. The key principle is to determine which branch represents the correct baseline from history and callers — never pick a side based on line count. A wrong merge resolution frequently compiles cleanly and fails silently — compilation is not evidence of a correct merge.", | ||
| "expected_behavior": [ | ||
| "Refuses to pick Branch A's hunk based on line count alone", | ||
| "Instructs to reconstruct what each branch did using 'git show <branch>:<path>' and 'git merge-base'", | ||
| "Instructs to check already-merged non-conflicted callers to determine which branch's API the resolution must conform to", | ||
| "States the resolution is conditional on inspection — not mandated to always be Branch B", | ||
| "Explains that keeping std::atomic alongside the OpenMP model would leave a field that is never set and a guard that never fires", | ||
| "Warns that a wrong merge resolution frequently compiles cleanly and fails silently" | ||
|
Comment on lines
+76
to
+87
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Make the merge resolution conditional on the actual baseline. The question does not establish that Branch B is newer or is the merge target, yet 🤖 Prompt for AI Agents |
||
| ] | ||
| }, | ||
| { | ||
| "id": "dev-eval-003-cuda-memory-and-error-handling", | ||
| "question": "I am adding a new C++ function to cuOpt that allocates a GPU buffer and calls a CUDA kernel. A colleague wrote the allocation as 'int* d_buf = new int[N];' and error-checked the kernel with 'if (cudaGetLastError() != cudaSuccess) return;'. What is wrong with both, and what should they be replaced with?", | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.