Add agent skills for building and testing - #8034
Conversation
csadorf
added
improvement
| /AGENTS.md @rapidsai/cuml-python-codeowners @rapidsai/cuml-cpp-codeowners | ||
| /.agents/ @rapidsai/cuml-python-codeowners @rapidsai/cuml-cpp-codeowners |
There was a problem hiding this comment.
I think we should consider creating a dedicated @rapidsai/cuml-agent-instructions-owners team.
📝 WalkthroughWalkthroughThis PR introduces foundational agent instruction materials: an onboarding document (AGENTS.md) that navigates agents to dedicated build and test skill pages, comprehensive runbooks for building and testing cuML workflows, ownership rules for the new agent documentation structure, and CI workflow updates to exclude agent documentation from unrelated job triggers. ChangesAgent Instruction Documentation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 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.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.agents/build-cuml/SKILL.md (2)
356-362: Run codespell (and markdownlint) on.agents/docs before merge.Given the amount of new markdown content, I recommend running:
codespellacross.agents/(per coding guidelines: “Check for spelling mistakes using codespell”)- markdown linting to catch any remaining MD028/MD051 style issues.
If you want, I can draft the exact commands for your repo’s existing tooling.
Based on learnings, doc/offboarding materials are expected to be robust and precise for agent workflows, so catching lint/spelling early reduces friction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/build-cuml/SKILL.md around lines 356 - 362, Run codespell and a markdown linter over the new .agents docs and fix any findings before merging; specifically run codespell across the .agents directory and run your repo's markdownlint configuration against .agents/build-cuml/SKILL.md (check the "Additional resources" section and other new headings for MD028/MD051 and spelling errors), then commit corrected markdown and spelling fixes so the PR contains lint-clean documentation.
81-88: Optional style: “Pick the right env file” → “Pick the correct env file”.This is flagged as a redundancy (“right” used nearby). It’s not functionally important, but easy to polish.
🛠️ Proposed diff
-#### Step 1: Pick the right env file +#### Step 1: Pick the correct env file🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/build-cuml/SKILL.md around lines 81 - 88, Replace the heading text "Pick the right env file" with "Pick the correct env file" in SKILL.md to remove the redundancy; update the section title string (the heading symbol) so the new wording appears exactly where the current "Pick the right env file" heading is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/build-cuml/SKILL.md:
- Around line 21-32: The blockquote under "Important (sm_121 / new arch + conda
RAPIDS libs)" contains a stray blank line that breaks markdownlint MD028; remove
the blank line so the quoted paragraph remains contiguous (ensure the closing
``` of the inline code example is immediately followed by the next quoted line
like "> Bug details: …"), and make sure the following "Note on env layout"
paragraph is a separate blockquote (each quoted line prefixed with "> ") without
stray characters or hyphens.
- Around line 46-54: Update the broken markdown link fragment: replace the
reference `#step-3-exclude-the-env-from-git-and-the-editor` with
`#step-2-configure-exclusions` so the `.conda-env` exclusion link points to the
actual heading "#### Step 2: Configure exclusions"; ensure the link in the
paragraph describing the per-worktree conda env (the sentence containing
`.conda-env/` and the existing anchor) is the one changed.
In @.agents/test-cuml/SKILL.md:
- Around line 91-105: In the "Dask tests (multi-GPU)" section update the
misspelled protocol name "UCXX" to "UCX" everywhere—including the prose and the
example lines that mention the `--run_ucx` flag and the canonical entry point
`ci/run_cuml_dask_pytests.sh`—so the documented flag and wording consistently
use `UCX` (single "X") instead of "UCXX".
---
Nitpick comments:
In @.agents/build-cuml/SKILL.md:
- Around line 356-362: Run codespell and a markdown linter over the new .agents
docs and fix any findings before merging; specifically run codespell across the
.agents directory and run your repo's markdownlint configuration against
.agents/build-cuml/SKILL.md (check the "Additional resources" section and other
new headings for MD028/MD051 and spelling errors), then commit corrected
markdown and spelling fixes so the PR contains lint-clean documentation.
- Around line 81-88: Replace the heading text "Pick the right env file" with
"Pick the correct env file" in SKILL.md to remove the redundancy; update the
section title string (the heading symbol) so the new wording appears exactly
where the current "Pick the right env file" heading is used.
🪄 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: ceec5084-eb7d-4a96-8fc4-a639b89046a0
📒 Files selected for processing (4)
.agents/build-cuml/SKILL.md.agents/test-cuml/SKILL.md.github/CODEOWNERSAGENTS.md
mgrauer
left a comment
mgrauer
left a comment
There was a problem hiding this comment.
Generally content looks good and we can always iterate.
To support progressive disclosure (allowing an agent to be token efficient when holding the skill in the context window, but not invoking the skill) can you split the skills into skill+references? I am biased towards leaner skills.
I think the split should roughly be:
- skill - general context, when to invoke the skill - supports the decision of "should I invoke this skill?"
- references - specific commands, technical details - anything that can be deferred until the "invoke this skill" decision is made
See here as an example of referring to references
|
First off, I think we should have an One unresolved problem for me is how to have skills or opinionated things in As a comparison, I have been using these lines in my local This seems to be more than enough for the agent to (a) use "the right" existing env (which might or might not contain specially installed versions that I hand picked for the current task) and (b) reliably build the project. But even these ten lines are somehow opinionated. Maybe we could make I think we need to iterate a bit to find the smallest common denominator that most people can agree to and include a mechanism to customise things. For example, maybe including "Check My full |
|
@betatim I think we can and should try to make the skill robust against alternative approaches for conda env management, but I consider your suggested date-based approach for conda env naming significantly more opinionated than what I suggested. Bottom-line is that development environment management is typically the thing that my agents get hung upon and the approach outlined here seems to work. Have you tried this skill in combination with your AGENTS.md yet? |
|
I'll attempt to make the skills more modular and generic to accommodate alternative environment management conventions. I think it will require a bit of tuning to get that right, but I think it's important to get it right since it's essentially the crux of the entire "build cuml" flow. |
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 @.agents/setup-dev-environment/SKILL.md:
- Around line 132-141: The script reads DRIVER_CUDA but never enforces the
documented R580 rule; parse DRIVER_CUDA to extract a numeric major driver
version (e.g., DRIVER_CUDA_MAJOR from DRIVER_CUDA by splitting on '.' and
converting to an integer) and include it in the selection logic: set
CUDA_TAG=129 if DRIVER_CUDA_MAJOR < 580 OR if CC < 7.5 (the existing Volta
check), otherwise set CUDA_TAG=131. Update the conditional that currently
references CC to also check DRIVER_CUDA_MAJOR and use those variables
(DRIVER_CUDA, DRIVER_CUDA_MAJOR, CC, CUDA_TAG) so old drivers (<R580) on
non-Volta GPUs correctly choose cuda-129.
🪄 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: be814abb-99d2-40aa-a4c7-8d2f342c4a07
📒 Files selected for processing (6)
.agents/build-cuml/SKILL.md.agents/setup-dev-environment/SKILL.md.agents/test-cuml/SKILL.md.github/CODEOWNERS.github/workflows/pr.yamlAGENTS.md
✅ Files skipped from review due to trivial changes (3)
- .github/CODEOWNERS
- AGENTS.md
- .agents/test-cuml/SKILL.md
|
If you are able to get a revised version of this up by Friday, I could give it a test drive with claude and codex. Are there specific prerequisites you have in mind? They may be written somewhere, but just to double check, e.g., can I assume just a clean repo checkout and no conda / pip env setup as a starting point? |
|
I've added these two skills to my local checkout. I'll report back what happens as I do work. Maybe a way forward is to adjust the skills so that an agent can tell that they don't need to use them. As a human on a "foreign" system I will look around if mamba is available or if there is a I agree that using dates in the env name is something many people would describe as "wonderfully weird". The reason I do it is that setting up a new env takes a long time, building cuml in it takes a while on top. So I try to re-use my existing env as much as possible. |
|
n.b. my version of Cursor is convinced that |
|
I'm considering to pull the env management out of this PR and get at least the build and test skills merged. I can probably come up with a testing framework that lets us evaluate how well the env management works with in combination with some other custom setups. |
61adf89 to
62957a4
Compare
|
I've stripped the env management from this PR. |
|
@betatim Did you get a chance to test this? Or any other thoughts? |
4 participants
Adds AGENTS.md with general guidelines and reference to the two new skills: build-cuml and test-cuml
The build-cuml skill is somewhat opinionated about how to setup the build environment. That's intentional to avoid ambiguity and make sure that multiple agents don't interfere with each other (or at least make that less likely).
The test-cuml skill is fairly comprehensive, but that's because there are many different ways to run tests.
Closes #8103