Skip to content

Improve agent documentation - #3435

Open
ethanglaser wants to merge 8 commits into
uxlfoundation:mainfrom
ethanglaser:dev/eglaser-agent-rules
Open

ethanglaser wants to merge 8 commits into
uxlfoundation:mainfrom
ethanglaser:dev/eglaser-agent-rules

Conversation

@ethanglaser

@ethanglaser ethanglaser commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Description

1. Rules taken from review comments

  • Root AGENTS.md: the same core rules as oneDAL #3812, plus type hints and numpydoc on new functions.
  • New onedal/datatypes/AGENTS.md: Python C-API reference ownership, based on 9 threads about real leaks and aliasing bugs. onedal/AGENTS.md now links to it.
  • tests/AGENTS.md:
    • the test matrix a new estimator needs (pandas and polars; with and without target_offload; dpnp, torch and array_api_strict)
    • tests must be able to fail
    • rules for deselected_tests.yaml entries
  • sklearnex/AGENTS.md: delete version checks that are dead now that the minimum scikit-learn version has moved, and keep scikit-learn conformance logic in sklearnex/ rather than onedal/.
  • doc/AGENTS.md: keep docs in sync with the code, and use the Sphinx roles and substitutions reviewers ask for.

2. Trimming

  • Root AGENTS.md (+48/−143, from 185 lines to about 70): removed install steps for users, algorithm and GPU-support lists, the "up to 100X" claim, GPU troubleshooting, and the list of runtime errors. Kept the architecture, key files, build and test commands (checked against setup.py and run_test.sh), minimum versions and code generation. It's ASCII only.

3. Drop the Copilot instruction files

  • Removed .github/copilot-instructions.md and .github/instructions/*. Copilot code review and the coding agent both read AGENTS.md, so they only duplicated it. On main they also lack applyTo: frontmatter, so Copilot has never loaded them.
  • Content not already in an AGENTS.md was merged into the nearest one: verified test commands in sklearnex/, onedal/, src/ and daal4py/ AGENTS.md, and the in-place rebuild commands in the root file. A daal4py/sklearn/tests/ path that doesn't exist was dropped.
  • Root AGENTS.md: Rules for Changes and a new Reviewing section now come first, because the root file is the one Copilot code review is documented to read. Two rules were added there: the sklearnex/-vs-onedal/ layering and PyObject* ownership.
  • .licenserc.yaml: dropped the two exemptions for the removed files.

Checklist:

Completeness and readability

  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes or created a separate PR with updates and provided its number in the description, if necessary.
  • Git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details).
  • I have resolved any merge conflicts that might occur with the base branch.

Testing

  • I have run it locally and tested the changes extensively.
  • All CI jobs are green or I have provided justification why they aren't.
  • I have extended testing suite if new functionality was introduced in this PR.

Performance

  • I have measured performance for affected algorithms using scikit-learn_bench and provided at least a summary table with measured data, if performance change is expected.
  • I have provided justification why performance and/or quality metrics have changed or why changes are not expected.
  • I have extended the benchmarking suite and provided a corresponding scikit-learn_bench PR if new measurable functionality was introduced in this PR.

🤖 Generated with Claude Code

ethanglaser and others added 3 commits September 23, 2026 16:00
…ctions

- Root AGENTS.md: replace stale version-support claims with pointers to the
  checks in setup.py; add build/test commands and cross-cutting change rules.
- New onedal/datatypes/AGENTS.md: Python C-API reference ownership rules.
- tests/, sklearnex/, doc/, src/, daal4py/: add rules drawn from recurring
  review comments; close an unclosed code fence in tests/AGENTS.md; correct
  daal4py guidance that suggested editing generated sources.
- .github/instructions: add applyTo frontmatter so Copilot loads them, and
  fix the AGENTS.md links, which resolved under .github/.
- Add .github/copilot-instructions.md routing to the nested AGENTS.md files.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Drop user-facing install, GPU troubleshooting, algorithm and speedup
lists that an agent cannot act on or that were unverified; keep
architecture, build/test commands, version floors and rules. ASCII only.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@codecov

codecov Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag Coverage Δ
azure 82.55% <ø> (+<0.01%) ⬆️
github 75.09% <ø> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ethanglaser ethanglaser changed the title Dev/eglaser agent rules Improve agent documentation Sep 25, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved review comments identify documentation accuracy and consistency issues.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 Low severity

Open (2)
What changed in this PR

This PR improves repository and Copilot guidance through consolidated AGENTS.md files, testing documentation, and instruction wiring.

Changes:

  • Streamlined repository and subsystem guidance.
  • Added testing, compatibility, ownership, and documentation rules.
  • Enabled scoped Copilot instructions and corrected links.
File Summary
tests/​AGENTS.md Adds test matrix and deselection guidance.
src/​AGENTS.md Adds C++ error-reporting guidance.
sklearnex/​AGENTS.md Adds compatibility guidance.
onedal/​datatypes/​AGENTS.md Adds C-API ownership and aliasing rules.
onedal/​AGENTS.md Links to datatype guidance.
doc/​AGENTS.md Adds documentation maintenance rules.
daal4py/​AGENTS.md Clarifies generator usage.
AGENTS.md Consolidates repository-wide guidance.
.github/​instructions/​tests.instructions.md Adds scoped instruction metadata and corrected links.
.github/​instructions/​src.instructions.md Adds scoped instruction metadata and corrected links.
.github/​instructions/​sklearnex.instructions.md Adds scoped instruction metadata and corrected links.
.github/​instructions/​onedal.instructions.md Adds scoped instruction metadata and corrected links.
.github/​instructions/​general.instructions.md Adds scoped instruction metadata and corrected links.
.github/​instructions/​daal4py.instructions.md Adds scoped instruction metadata and corrected links.
.github/​instructions/​build-config.instructions.md Adds scoped instruction metadata and corrected links.
.github/​copilot-instructions.md Adds the repository-wide Copilot entry point.
.github/​.licenserc.yaml Exempts Copilot instructions from license checks.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread AGENTS.md Outdated
Comment thread tests/AGENTS.md Outdated
ethanglaser and others added 2 commits September 24, 2026 23:00
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: ethanglaser <ethan.glaser@intel.com>
@napetrov

napetrov commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

I ran the same base-vs-head analysis I did for oneDAL #3812 on this PR (base 95ad281, head 15ca9e6). Overall this is a clear improvement: the root file is about twice as dense, almost nothing actionable is lost, and most of what was removed was wrong. A few things are worth fixing before merge.

Still wrong at head

  1. daal4py/AGENTS.md:22,37 point to daal4py/sklearn/monkeypatch/dispatcher.py, which doesn't exist; patching is in sklearnex/dispatcher.py. Lines 138 and 147 ("monkeypatch dispatcher") describe the same thing. The PR already edits this file.
  2. .ci/AGENTS.md:27 names doc/sources/quick-start.rst, which doesn't exist (installation.rst is the closest).
  3. onedal/datatypes/AGENTS.md: "Most review findings in this directory are reference leaks and aliasing bugs, not style." The review history doesn't support it. I count 11 human review threads about ownership (in Move onedal module from Cython to PyBind11 #724, ENH: Data management update to support SUA ifaces for Homogen OneDAL tables #2045, [enhancement] remove contiguous check from _check_array #2185, FIX: Fix table destructor not being called #2540, FIX: Sort indices of sparse matrices #2604, [enhancement] Add CPython free-threading support #3325, [bug] Fix reference counting and buffer ownership in the NumPy/NumericTable conversions #3361 and FIX: Fix memory leaks and memory safety issues in base1 CSR array conversions #3403), which matches your ~9, but they are 11 of 172 threads on that path. I'd drop the sentence; the rules stand on their own.
  4. The numpydoc hook excludes GL08 (missing docstring) and private names, so CI never flags a function without a docstring. "New functions get … a numpydoc docstring" is therefore review-only, and "Don't report what CI already enforces: … numpydoc validation" can be read as "don't report missing docstrings". Maybe say "numpydoc formatting" there.

Checked and correct

  • About 200 commands, paths and identifiers in the head files resolve. That includes every command in the new Verification blocks (they match conda-recipe/run_test.sh), the setup.py strings and switches, the datatypes layout, fixtures and markers (no gpu marker), the no-polars default in get_dataframes_and_queues, and the CI tool list.
  • The 7 broken ../X/AGENTS.md links and the nonexistent daal4py/sklearn/tests/ path go away with the deleted .github/instructions files.
  • Copilot docs (github/docs, checked 2026-10-01):
    • Code review has read AGENTS.md since 2026-07-13, but only the repository-root file is documented. Putting Rules and Reviewing first in the root file, with pointers to the nested files, is the right shape.
    • Code review also reads CLAUDE.md, GEMINI.md and REVIEW.md if present.
    • The 4,000-character limit was removed in June, before AGENTS.md support arrived.

What was removed
I checked 292 facts from the deleted instruction files and the trimmed root file against all head AGENTS.md files:

  • 71% are still stated somewhere.
  • The 10 that head contradicts are all corrections: oneDAL 2021.1 compatibility (the floor is 2025.0), the scikit-learn 1.0 special case (the floor is 1.6), requirements-test.txt as the version source, daal4py/sklearn/tests/, and the mpirun -n 2 python -m pytest command.
  • What is genuinely gone is mostly GPU setup and troubleshooting, which is already in doc/sources/oneapi-gpu.rst, plus links to the deleted files.
  • The small real losses are "SVM/NaiveBayes accept CSR" and the per-algorithm GPU support list.

Does it teach models anything?
For each file, this is the share of facts a model gets wrong when asked without the file (one probe per call, gpt-6-luna judge). Base → head:

file opus-5.5 sonnet-5 gpt-6-sol gpt-6-luna
root AGENTS.md (137→87 facts) 18→19% 24→50% 29→51% 21→41%
onedal/datatypes (new) 19% 41% 33% 30%
tests 24→23% 29→38% 35→41% 26→32%
sklearnex 1→7% 10→18% 7→17% 14→13%
  • With the file loaded, models answered 90–100% of those facts correctly, and 0–1 of ~33 already-known answers went wrong.
  • This probing can't catch a wrong line, because the expected answers come from the file itself. That's why items 1–3 above come from checking against the tree instead.
  • For Opus, sklearnex/AGENTS.md is still mostly things it already knows.
Method and data

Mechanical claim check with hand adjudication; review comments from the GitHub API (10,364, Copilot excluded); github/docs source and history; gpt-6-luna fact extraction and judge; four answer models, reported per model, never pooled; gpt-6-sol for the retention check. Spend about $10. Not measured: task-level effect on real coding or review runs.

@ethanglaser

Copy link
Copy Markdown
Contributor Author

@napetrov thanks for the detailed analysis, I've applied updates based on your feedback

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants