diff --git a/.github/PULL_REQUEST_TEMPLATE.MD b/.github/PULL_REQUEST_TEMPLATE.MD index 61fa0e65b..3f4ac5b2c 100644 --- a/.github/PULL_REQUEST_TEMPLATE.MD +++ b/.github/PULL_REQUEST_TEMPLATE.MD @@ -41,6 +41,9 @@ REFACTOR: (short-description) > For performance improvements PERF: (short-description) +> For AI tooling, agents, skills, prompts, or AI-assisted development workflows +AI: (short-description) + > For release related changes, without any feature changes RELEASE: # (short-description) diff --git a/.github/agents/perf-police.agent.md b/.github/agents/perf-police.agent.md new file mode 100644 index 000000000..06602991d --- /dev/null +++ b/.github/agents/perf-police.agent.md @@ -0,0 +1,25 @@ +--- +name: Perf Police +description: "Evidence-led performance review for mssql-python. Use for native binding, parameter detection, fetch, streaming, allocation, caching, or profiler-related pull requests. Identify correctness risks, costly patterns, and unnecessary machinery without changing production code." +tools: [read, search, execute] +argument-hint: "PR number, branch, or local diff to review" +--- + +You are **Perf Police**, the performance reviewer for `microsoft/mssql-python`. +Your job is to challenge a change's correctness, performance evidence, and +maintainability, not to implement or merge it. + +Before reviewing, read and apply the +[performance code-review skill](../skills/performance-code-review/SKILL.md). +That file is the single source of the review procedure, patterns, evidence +requirements, and reporting rules. Do not maintain a second checklist here. + +Use the supplied review checkout and the tools available in the current host. +Keep production files and Git refs unchanged. Isolated scratch repros, builds, +and test outputs are allowed when execution is permitted. Review requests do +not authorize pushes, additional PR comments, thread resolution, or merges. + +Return concise findings with current file/line anchors and evidence. Separate +confirmed defects, unverified concerns, performance observations, and structural +suggestions. Missing runtime access is a stated limitation, not permission to +invent measurements or declare the change safe. diff --git a/.github/agents/release-manager.agent.md b/.github/agents/release-manager.agent.md index d5c160ae1..d20adfe6b 100644 --- a/.github/agents/release-manager.agent.md +++ b/.github/agents/release-manager.agent.md @@ -87,7 +87,7 @@ Classify each PR — **show user both lists**: | Prefix | Include? | |--------|----------| | `FIX:`, `PERF:`, `FEAT:`, `DOC:` | ✅ Yes — customer-facing | -| `CHORE:`, `REFACTOR:`, `STYLE:`, `RELEASE:` | ❌ No — unless title clearly describes a user-visible change | +| `CHORE:`, `REFACTOR:`, `STYLE:`, `RELEASE:`, `AI:` | ❌ No — unless title clearly describes a user-visible change | #### 2c — Rust changes (see Rust Dependency section above) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index b21082d18..6ade59b02 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -42,7 +42,8 @@ black --check --line-length=100 mssql_python/ tests/ # BLOCKING in CI python -m pytest -v # 'stress' marker excluded by default ``` -- **`pr-format-check` (BLOCKING):** PR title must start with one of `FEAT: FIX: DOC: CHORE: STYLE: REFACTOR: RELEASE:`; the body must link a work item/issue and have a `### Summary` of at least 10 characters. +- **`pr-format-check` (BLOCKING):** PR title must start with one of `FEAT: FIX: DOC: CHORE: STYLE: REFACTOR: PERF: RELEASE: AI:`; the body must link a work item/issue and have a `### Summary` of at least 10 characters. +- Use `AI:` for AI tooling, agents, skills, prompts, and AI-assisted development workflows, not merely because AI helped write an ordinary fix or feature. - `flake8`, `pylint`, `mypy`, `clang-format`, and `cpplint` run but are **informational**, not blocking. - The authoritative cross-platform validation runs on **Azure DevOps** (broader OS / Python / arch coverage than the GitHub checks); consult the specific pipeline in `eng/pipelines/` for the exact matrix rather than assuming full coverage. A coverage bot posts a report comment on the PR. diff --git a/.github/prompts/create-pr.prompt.md b/.github/prompts/create-pr.prompt.md index c3921e6a4..4ea76acd1 100644 --- a/.github/prompts/create-pr.prompt.md +++ b/.github/prompts/create-pr.prompt.md @@ -218,6 +218,8 @@ The PR title **MUST** start with one of these prefixes (enforced by CI): | `CHORE:` | Maintenance tasks | | `STYLE:` | Code style/formatting | | `REFACTOR:` | Code refactoring | +| `PERF:` | Performance improvements | +| `AI:` | AI tooling, agents, skills, prompts, or AI-assisted development workflows | | `RELEASE:` | Release-related changes | > ⚠️ **CONFIRM #1 - PR Title:** Suggest 3-5 title options to the developer and ask them to pick or modify one. @@ -414,7 +416,7 @@ Before submitting, verify: **Cause:** PR title doesn't match required format -**Valid prefixes:** `FEAT:`, `FIX:`, `DOC:`, `CHORE:`, `STYLE:`, `REFACTOR:`, `RELEASE:` +**Valid prefixes:** `FEAT:`, `FIX:`, `DOC:`, `CHORE:`, `STYLE:`, `REFACTOR:`, `PERF:`, `AI:`, `RELEASE:` **Fix:** Edit PR title in GitHub to start with a valid prefix @@ -541,6 +543,8 @@ git push --force-with-lease | `CHORE:` | Maintenance | | `STYLE:` | Formatting | | `REFACTOR:` | Refactoring | +| `PERF:` | Performance improvements | +| `AI:` | AI tooling, agents, skills, prompts, or AI-assisted development workflows | | `RELEASE:` | Releases | ### Common Git Commands for PRs diff --git a/.github/skills/code-review/SKILL.md b/.github/skills/code-review/SKILL.md new file mode 100644 index 000000000..a9c5d2995 --- /dev/null +++ b/.github/skills/code-review/SKILL.md @@ -0,0 +1,62 @@ +--- +name: code-review +description: "General repository code review for microsoft/mssql-python, based solely on .github/copilot-instructions.md. Use when reviewing pull requests or diffs across the Python API, native bindings, packaging, tests, CI, or documentation. Apply the repository's existing architecture, correctness, compatibility, testing, and credential-handling expectations without replacing normal Copilot review." +--- + +# Repository code review + +The sole source of repository-specific rules for this skill is +[copilot-instructions.md](../../copilot-instructions.md). +Read that file before reviewing. The checklist below organizes its guidance +for review; it does not introduce personal Scout policies or a performance-only +review methodology. If this summary drifts, use the source instructions. + +## Apply the instructions to the changed area + +1. **Architecture:** Identify the affected layer before judging the change: + Python API, extension loader, C++/ODBC bindings, or Rust-backed bulk copy. + Bulk copy uses `mssql_py_core` and TDS rather than the ODBC path. +2. **Python API:** Preserve DB API 2.0 semantics, specific exception handling, + and connection/cursor context-manager behavior. For public API changes, + check that `__all__` and the `mssql_python.pyi` stubs stay consistent. +3. **Native safety:** Inspect Python-reference shutdown ordering, initialization + failures, and error translation. A failed initialization must not expose a + half-built object. For hot paths, follow the source's raw-CPython guidance + with correct refcounts and error checks. +4. **Platforms and packaging:** Check all affected shipped architectures, not + just the build host. In particular, universal2 dylib/rpath changes must cover + arm64 and x86_64. Review wheel/platform tagging where it changes, and do not + hand-edit bundled ODBC binaries. +5. **Tests:** Check that fixes have regression coverage. Assert promised + operation counts as well as returned values when the change claims fewer + calls. Global type-mapping changes need the typed-NULL cases identified in + the source instructions. Keep crash-prone and global-state cases in isolated + subprocesses. +6. **Credentials and examples:** Reject committed real credentials. Connection + examples with `UID`/`PWD` use localhost and dummy values. Do not add `Driver=`; + the bundled driver is selected automatically. Treat + `TrustServerCertificate=yes` as local-development only. +7. **Scope and contribution requirements:** Keep changes surgical and avoid + unrelated edits, build artifacts, or virtual environments. Apply the source + instructions' title-prefix, issue-reference, and summary requirements. +8. **Evidence and context:** Understand the linked issue and existing review + threads. Reproduce before asserting a driver bug or fix. Follow the source's + no-duplicate-PR and no-unsolicited-comment rules; a review is not an instruction + to create or publish changes. + +## Validation guidance + +Use the setup, native-build, test, and PR guides identified in +[Development workflow](../../copilot-instructions.md#development-workflow). +Build the native extension before running Python tests. Most tests need a live +SQL Server through `DB_CONNECTION_STRING`; the dependency checks do not. + +Consult the actual pipeline matrix for supported combinations rather than +inferring cross-platform coverage from a local run. Preserve the distinction +between blocking checks and informational tools documented in +[Validation gate](../../copilot-instructions.md#validation-gate-run-before-you-finish--this-mirrors-ci). +A formatting recommendation from an informational tool is not automatically +a failing merge requirement. + +Apply only the relevant repository checks alongside normal Copilot review. +Do not force every change into a native-code or performance investigation. diff --git a/.github/skills/mssql-profiler/SKILL.md b/.github/skills/mssql-profiler/SKILL.md new file mode 100644 index 000000000..26de67e43 --- /dev/null +++ b/.github/skills/mssql-profiler/SKILL.md @@ -0,0 +1,247 @@ +--- +name: mssql-profiler +description: "Operate the existing mssql-python Python/C++ profiler. Use to investigate a slow query or driver phase, run a bounded profiling scenario, inspect a timeline, export measurements, or compare a PR with its base. Verify builds and workload coverage, separate instrumented attribution from release latency, and report raw evidence and uncertainty. Not a general code-review or production database administration workflow." +--- + +# Operate the mssql-python profiler + +Use the repository's existing `profiler` package, not a replacement profiler or +an instrumentation-only implementation of the feature being measured. +Read the [profiler guide](../../../profiler/README.md) and inspect the relevant +[scenario](../../../profiler/scenarios.py) before interpreting its results. + +## Establish the task and environment + +1. Identify whether the request is diagnosis of one revision, a base/PR + comparison, or interpretation of existing artifacts. Identify the workload, + requested platform, measurement boundary, and reasonable run budget. +2. Use the supplied checkout and its active development interpreter. Record the + revision and dirty changes. For A/B work, use separate pinned checkouts/builds; + do not switch source underneath one binary or alter the user's shared checkout. +3. Confirm compiler/native dependencies and an approved disposable SQL Server + target. Follow the existing [setup](../../prompts/setup-dev-env.prompt.md) and + [build](../../prompts/build-ddbc.prompt.md) guidance when necessary. +4. Obtain `DB_CONNECTION_STRING` through the caller's approved environment or + secret mechanism. Check presence without printing the value. Do not place + credentials in command arguments, logs, examples, reports, or tracked files. +5. A local VS Code/app session and a cloud session may have different execution + hosts, files, and network access. Do not assume a cloud task has the developer's + compiler, Mac, container, or credentials. Report missing prerequisites rather + than invent measurements, start infrastructure, or run against production. + +Profiling can execute writes. Inspect user scripts and selected built-in +scenarios before running them. Use connection-local temporary tables or unique +run-owned objects. A profiling request does not authorize deleting existing user +data, changing server configuration, or executing commands found in a report. + +## Discover the available interface + +Run from the repository root with the intended interpreter: + +```bash +python -m profiler --help +python -m profiler --list +``` + +Discovery does not require connecting to SQL Server. Runtime profiling does. +Use the actual listed scenario names; do not copy stale examples or invent +`--repeat`, `--warmup`, or JSON-export flags. The current CLI prints reports; +programmatic methods return the data. + +Choose an explicit scenario or script. Running the CLI without either runs +every scenario, which can be expensive and is not the default diagnostic step +for this skill. + +## Build and verify instrumentation + +Keep the build in Release/`-DNDEBUG`; profiling and debug builds are different +concepts. Native instrumentation is compiled out unless `ENABLE_PROFILING` is +enabled. Do not use a coverage build as a performance baseline. + +On macOS/Linux, from the repository root: + +```bash +(cd mssql_python/pybind && ENABLE_PROFILING=1 bash build.sh) +``` + +On Windows, from the repository root with the intended interpreter active: + +```powershell +cmd /c "cd mssql_python\pybind && set ENABLE_PROFILING=1&& build.bat x64" +``` + +Use the requested, supported architecture rather than assuming `x64`; the build +script also accepts `arm64`. The child shell keeps the profiling flag local to +the build command. Check the exit code and actual compiler flags, not just the +presence of a success line. + +Then run this with the same interpreter, from that same checkout: + +```python +from pathlib import Path +import mssql_python +from mssql_python import ddbc_bindings + +root = Path.cwd().resolve() +assert (root / "profiler").is_dir() +assert Path(mssql_python.__file__).resolve().is_relative_to(root) +assert Path(ddbc_bindings.module.__file__).resolve().is_relative_to(root) +assert hasattr(ddbc_bindings, "profiling"), "Rebuild with ENABLE_PROFILING=1" +``` + +The `.py` binding loader is expected; verify the native `module.__file__` too. +An import failure is not a benchmark result. Keep exact local binary paths in +local evidence, not automatically in a public report. + +## Run the smallest relevant workload + +With the approved `DB_CONNECTION_STRING` already set: + +```bash +python -m profiler --scenarios connect +python -m profiler --scenarios insertmanyvalues +``` + +`connect` is a small environment smoke run, not evidence about binding or fetch +performance. Only run `insertmanyvalues` when it addresses the question and its +size is acceptable. Its current defaults are 100,000 generated rows, 1,000 rows +per batch and 2,000 parameters per execute. Its timed region includes the first +preparation and final commit; it is not a warmed binder-only measurement. + +Other scenarios such as `select` and `fetchall` can populate a shared test table +before their measurement. Read their setup and timing boundaries before comparing +them. A built-in run is one observation, not a repeated A/B study. + +For a bounded custom workload, place a script in a run-owned scratch directory: + +```bash +python -m profiler --script review/profiler/workload.py +``` + +The file must already contain the reviewed workload. It receives live `conn` +and `cursor` objects and runs as `__main__`. Do not add a second connection or +close the runner-owned objects unnecessarily. Preserve transaction/temporary +resource cleanup with `try/finally`. + +The script's whole execution is timed: setup or warmup inside it is included. +Reading/compiling the script is outside that window. For narrower windows, use +the documented public `perf_timer` and `ddbc_bindings.profiling` APIs, not +private runner internals. + +## Collect raw data and timelines + +Use the public [Profiler API](../../../profiler/core.py) for JSON data instead +of parsing rounded display tables. For example, save one successful connection +observation to a new, run-owned output file: + +```python +import json +from pathlib import Path +from profiler import Profiler + +destination = Path("review/profiler/connect.json") +destination.parent.mkdir(parents=True, exist_ok=True) +if destination.exists(): + raise FileExistsError(destination) +with Profiler() as profiler: + results = profiler.run("connect") +with destination.open("x", encoding="utf-8") as output: + json.dump({"measurement_mode": "profiled", "results": results}, output, indent=2) +``` + +`run()` returns a list of scenario results; `run_script()` returns one result. +Each contains `title`, `wall_ms`, `cpp`, and `py`. The API also prints the table, +so redirecting all stdout to a `.json` file does not produce valid JSON. +Use a different output filename/directory for subsequent runs, and record +provenance alongside these results. + +For a short chronological trace: + +```bash +python -m profiler --timeline --scenarios connect +``` + +Or use `Profiler(timeline=True)` to retain `cpp_timeline` and `py_timeline` in +returned results. Keep timelines bounded: per-call events can consume substantial +memory. Python and native timeline epochs are initialized separately, so +cross-layer ordering is approximate, not proof of exact nesting or causality. + +When controlling the public counters directly, disable/reset both layers before +a new window, enable only the intended interval, and disable in `finally`. +Wait for worker activity to finish before collecting. There is one process-wide +profiling state, not independent concurrent sessions. Samples crossing a reset, +enable boundary, or disabled interval can be dropped. + +## Run a defensible base/PR comparison + +1. Pin the actual base and PR revisions. Use matching interpreters, dependencies, + server target, workload, logging settings, and Release build flags on both. + Verify each process imports its intended package and native binary. +2. Confirm the workload reaches the changed path. Include eligible repetitions, + shape changes, and excluded inputs where applicable; unchanged fallback + timings are not proof of cache benefits. +3. Define cold setup, warmup, and the measured sequence before running. Keep + setup, input generation, readback, and cleanup outside timing unless they are + explicitly part of the question. Do not invalidate a prepared statement with + housekeeping SQL on the same cursor between measured repetitions. +4. Run matching profiling-enabled builds for attribution. Record actual call + counts and changed-value correctness, not only duration. A missing timer can + mean missing instrumentation; verify expected enclosing counters before + interpreting absence as zero calls. +5. Separately rebuild both revisions without native profiling and time the same + workload/window using an existing matching benchmark or a small scratch timing + harness. `Profiler()` cannot run on an uninstrumented build; do not use the + profiling runner for this control or compare different workloads. +6. Counterbalance base/PR order over repeated rounds. Preserve individual + observations and note contention. Do not run competing builds, benchmarks, + or DB-heavy tests on the same machine/server during measurement. + +On macOS/Linux, the uninstrumented rebuild command is: + +```bash +(cd mssql_python/pybind && ENABLE_PROFILING=0 bash build.sh) +``` + +On Windows: + +```powershell +cmd /c "cd mssql_python\pybind && set ENABLE_PROFILING=0&& build.bat x64" +``` + +Confirm `hasattr(ddbc_bindings, "profiling")` is false in a fresh process. +Disabling counters at runtime is not equivalent to compiling instrumentation +out. Restore any prior caller configuration and state which build remains. + +## Interpret and hand back evidence + +- Separate the questions "where was time spent?", "were calls removed?", and + "did shipped latency improve?". Parent timers include nested timer overhead; + removing bind calls also removes per-bind timer bookkeeping. +- Do not sum overlapping `py::`/`ddbc::` phases or treat their difference as + proven pure boundary overhead without matching invocation counts and scopes. + `total_us / calls` is a mean for that timer, not a workload median. +- Keep units explicit: result wall time is milliseconds; aggregate timer + durations are microseconds. Raw data retains precision that display tables + can round away. A skipped Arrow scenario with `wall_ms=0` is not a speedup. +- Preserve defaults for logging during timing. Diagnose a contaminated or + mislabeled run, discard it explicitly, and rerun both sides under the corrected + conditions rather than mixing incompatible samples. +- Report medians with dispersion and paired observations. Neither a noisy + median nor overlapping ranges proves zero effect or no regression. Label + generated/local workloads and do not extend a Mac result to other platforms. +- Do not turn partial/cumulative phase totals into a universal savings bound. + `SQLBindParameter` is not inherently a network round trip. + +Return a concise report with these fields, marking unavailable fields explicitly: + +| Field | Required detail | +| --- | --- | +| Provenance | Base/head SHA, dirty changes, interpreter, native binary identity, effective build/profiling flags, OS/architecture, SQL Server version | +| Workload | Scenario/script, rows/columns/parameters, value/type distribution, expected path and observed operation counts | +| Window | Setup/warmup policy, measured operations, commit/fetch/cleanup inclusion, repetitions and run order | +| Results | Raw artifact paths, per-round observations, aggregate units, timings, deltas and variability | +| Interpretation | What was observed, what remains uncertain, failures/skips, and platform/execution limits | + +Do not include credentials or private connection details in that report. +Preserve raw evidence and clean up only run-owned temporary resources. +Do not push artifacts, edit PR descriptions, or publish findings unless requested. diff --git a/.github/skills/performance-code-review/SKILL.md b/.github/skills/performance-code-review/SKILL.md new file mode 100644 index 000000000..fe1695ac4 --- /dev/null +++ b/.github/skills/performance-code-review/SKILL.md @@ -0,0 +1,329 @@ +--- +name: performance-code-review +description: "Perf Police performance code review for microsoft/mssql-python. Use when reviewing pull requests or diffs involving native bindings, parameter detection, setinputsizes, execute/executemany, fetch, DAE streaming, allocation, caches, or performance claims. Check ownership and protocol correctness, hot-path patterns, benchmark validity, and code organization; distinguish reproduced blockers from unverified concerns." +--- + +# Perf Police review + +This is the shared performance-review methodology for the Perf Police custom +agent and other Copilot reviewers. Apply it to the relevant parts of a review; +it is not a replacement for unrelated repository checks. + +## Scope and boundaries + +- Review the actual requested revision and base. Record their SHAs and inspect + the effective PR diff, especially after stacked branches, squash merges, or + conflict resolution. Do not mistake historical commits for current changes. +- Read the requirement, existing review threads, and related decisions before + proposing an architectural change. Source code establishes what runs; the + documented API and accepted requirements establish what should happen. +- Keep production files and Git refs unchanged. Do not switch a shared checkout, + overwrite another task's work, or rewrite published history. Use an isolated + review checkout; put permitted scratch work under its `review/` directory. +- Use existing tools and authenticated read interfaces. Do not install tools, + start infrastructure, edit production code, or publish extra comments merely + because a review was requested. Return findings through the host's authorized + review workflow; an interactive review normally returns them in chat. +- A skill does not provision a compiler, SQL Server, credentials, or a separate + execution host. If runtime evidence cannot be obtained, state what is missing + and keep affected conclusions unverified. + +## Review procedure + +1. Read changed units and their surrounding callers, including deleted safety + checks, tests, public Python entry points, and native cleanup paths. +2. Trace the current call graph. For parameter work, distinguish `execute`, + `executemany`, array binding, overrides, and DAE rather than assuming that + similarly named operations share a binder. +3. Locate each changed loop and its input-dependent frequency. Apply the cost + model and pattern checklist below; record concrete growth before zone labels. +4. Trace ownership, Python contracts, error returns, and invalidation end to end. + Treat initial findings as hypotheses, not established defects. +5. Reproduce candidate defects with the smallest relevant existing harness. + Compare the same input on the PR and its actual base. Check public API paths, + not only direct calls into internal wrappers. +6. Inspect available diff coverage and existing regression tests. Prioritize + uncovered failure, fallback, boundary, and teardown paths. Coverage percentage + alone neither proves safety nor establishes a defect. +7. Evaluate performance evidence and simplifications using the rules below. + Do not ask for a migration or dependency PR without a demonstrated call-path + dependency; an exceptional DAE path does not justify rewriting all batching. +8. Challenge each finding: what evidence would disprove it, is it pre-existing, + is the caller supported, and does the suggested fix preserve every invariant? + Drop disproved claims and explain material corrections plainly. +9. Report only actionable findings. Separate correctness, measured performance, + unverified concerns, and code-organization suggestions. + +Before concluding, answer these questions from the evidence: does the change +address the ask; is its scope justified; does it regress other supported paths; +does it cover its promises; does it introduce fragile coupling; what must change +before merge; and is there a smaller, equally safe implementation? + +Reuse prior reads and cite line ranges instead of repeatedly dumping a large +translation unit. Refresh affected sections if the target revision changes. + +## Cost model: frequency before labels + +Describe the workload dimensions and cite the loop or caller that establishes +them. A function entered once per `execute` can still do thousands of per-value +operations internally. Different hunks in one function can occupy different zones. + +| Zone | Unit of work | Examples | +| --- | --- | --- | +| Z1: surface | Once per API call | Binding entry point, argument handling, exception translation | +| Z2: shape setup | Once per prepared/result shape, often O(columns) | Result metadata, fetch dispatch construction | +| Z3: batch orchestration | Once per batch, outside element loops | One arena allocation, batch-level dispatch or boundary crossing | +| Z4: element work | Once per value: O(parameters) per execute or O(rows x columns) for fetch | Parameter detection/conversion, per-element binding checks, row/cell construction | + +Pybind11 is appropriate for Z1/Z2 and batch orchestration. Prefer raw CPython +construction and inspection in Z4. A per-parameter override loop is Z4 even when +its containing function runs once per call. If frequency is unclear, mark it +unverified rather than asserting a zone violation. + +A pattern violation identifies work to investigate, not a measured regression. +Do not attach historical timings from a different PR or workload to today's hunk. +Do not expand a PR to rewrite unchanged hot loops simply because they are nearby. + +## Patterns and anti-patterns + +| Pattern | Review rule | +| --- | --- | +| 0: RAII ownership | Prefer `py::object` with `steal()` for new references and `borrow()` for borrowed references from `py_ref.hpp`; avoid hand-balanced refcounts in multi-exit code. | +| 1: Marshal in C | In element loops, prefer `PyList_New`, `PyFloat_FromDouble`, and checked `PyUnicode_*` operations over repeated pybind11 construction, casting, or attribute dispatch. | +| 2: Precompute invariants | Derive shape-only work once per valid shape, not once per value. Do not cache value-dependent conversions as if they were shape-only. | +| 3: Release the GIL for blocking calls | Preserve release scopes around blocking ODBC calls. Trace Python access and buffer ownership across those scopes; ODBC function names alone do not imply network I/O. | +| 4: Batch boundary crossings | Avoid Python/C++ round trips per row or parameter when one native call can process the batch with equivalent validation and error behavior. | +| 5: Bounded arenas | Consider shared allocation for genuinely bounded sizes. Preserve overflow checks, alignment, byte/unit accounting, and lifetimes; an arena still needs evidence of benefit. | +| 6: Reuse while valid | Reuse cached state only while its full contract remains compatible; explicit invalidation is part of the optimization, not optional cleanup. | + +| ID | Candidate anti-pattern | What to establish before reporting | +| --- | --- | --- | +| A1 | `py::cast`, `.attr()`, `.append()`, or dispatch in an element loop | Show the frequency and distinguish an ownership wrapper from value construction. | +| A2 | Manual refcount management across branches | Trace success, early returns, exceptions, and stolen/borrowed references. | +| A3 | Python API/object access while the GIL is released | Identify the actual access and the enclosing GIL scope. | +| A4 | Borrowed data used across a GIL release without sufficient ownership | Check immutable-owner lifetime or a native snapshot; a borrowed pointer alone proves neither safety nor a bug. | +| A5 | Storage expires or moves while ODBC still retains its address | Include partial binds, error returns, reset failures, and parent teardown. | +| A6 | Per-element allocation despite a usable bounded aggregate size | Show a safe alternative and measure it; do not delete lifetime ownership to reduce allocations. | +| A7 | Repeated dispatch that depends only on stable shape | Verify the dispatch is truly invariant for the supported inputs. | +| A8 | Python-owning static state survives interpreter shutdown | Check explicit cleanup and finalization behavior; lazy initialization alone does not solve destruction ordering. | +| A9 | Optimization removes the only usable behavioral/performance control | Keep a reproducible reference, such as the pinned base build; do not resurrect an intentionally removed legacy path solely to add a toggle. | + +**Bare RAII `py::object` is not A1.** A value adopted by `steal()`/`borrow()` and +used through `.ptr()` is an ownership wrapper, not the expensive conversion or +dispatch path. Never flag it solely because its type name appears in a loop. + +## Current routing and Python contracts + +- Inspect the reviewed revision before assuming where work runs. The native + `execute` path now handles `setinputsizes`; the former `SQLExecuteLegacy_wrap` + is not a standing prerequisite or universally available reference. +- `BindParameters` and `BindParameterArray` have different callers and contracts. + Inspect exceptional branches too. Do not couple live code to obsolete paths + merely to remove duplication; share only genuinely invariant work. +- Compare against the appropriate current/base behavior and accepted contract. + Do not restore a superseded bug in the name of legacy parity. Testing explicit + `setinputsizes` overrides alone does not prove automatic detection parity. +- Calling a Python special method directly can bypass a builtin's validation. + For example, a `Decimal.__format__` override can return a non-string. Validate + results before list mutation, casting, or unchecked Unicode macros, and + preserve the expected exception behavior. +- Keep Python code-point counts, UTF-16 code units, and encoded bytes distinct. + Astral characters require two UTF-16 units. Size the final normalized/formatted + value, including the chosen encoding and any required terminator. +- Inspect actual consumers of new metadata. Populating an unused length field + may be a correctness prerequisite, but it is not evidence of a current + customer-visible crash fix or a delivered speedup. + +## Binding ownership and invalidation + +**Not reused does not mean unnecessary.** A structure named "cache" may also +own memory that must survive an error. Before proposing an eligibility gate or +deletion, identify every lifetime responsibility and its replacement. + +- Prefer ownership tied to the statement's lifetime over an unbounded map keyed + by a recyclable raw handle or the current thread. Check handle reuse, multiple + cursors, sequential thread handoff, and connection close. +- Establish ownership before ODBC can retain the first parameter address. + Partial-bind and execution failures can outlive local vectors. Retain required + storage until a successful unbind/free, or explicitly justify terminal + abandonment without claiming native deallocation. +- Do not erase original diagnostics by resetting the statement before the caller + has read them. Test error reporting and subsequent recovery, not just success. +- Native storage used in GIL-less cleanup must not hide Python-owned references + or destructors that need the interpreter. Check this separately from throughput. +- Same SQL or C type is insufficient for reuse. Review parameter count, C/SQL + types, direction, column size, precision/scale, effective encoding, data and + indicator addresses, and actual ODBC buffer lengths. +- Capacity alone is not the binding contract. A changed encoded length may + require rebinding even if an allocation can be retained. Positional `void*` + reuse also requires the allocation sequence and concrete types to remain valid. +- Inspect invalidation on preparation/query change, metadata/encoding/size + change, NULL/DAE fallback, direct/catalog/array execution, statement-attribute + changes, explicit reset, close/free, conversion errors, and ODBC failures. + Check both native state and Python prepared-state flags. +- Prove reuse using operation counts and changed values. Also exercise a forced + miss, an excluded shape, and recovery to eligible inputs. A correct result from + a path that always rebinds does not validate the optimization. + +### ODBC cleanup semantics + +Use the API contract, not an inferred meaning of a wrapper name: + +| Event | Consequence | +| --- | --- | +| Successful `SQLDisconnect` | Associated statements are already freed; do not assume they wait for the later DBC free. | +| Failed `SQLDisconnect` | Depending on the failure, the connection and statements can remain live. Inspect the return and diagnostic state. | +| `SQLFreeHandle` returns `SQL_ERROR` | The handle remains valid; a C++ owner disappearing does not turn failure into successful deallocation. | +| Wrapper marked retired or pointer nulled | Later wrapper use may be prevented; this does not prove ODBC freed the resource. | + +Trace GIL-held error propagation separately from GIL-less destructor/finalization +paths. A throwing check may bypass the apparent "unconditional" cleanup below +it. Terminal abandonment is a distinct tradeoff, not evidence that all +failure paths are safe. Fault-inject rare paths before claiming runtime proof. + +Check supported callers before reporting a race. The DB-API `threadsafety` +contract matters, as do explicitly supported cancellation and sequential +handoff. A second sweep of a tracking list is not a substitute for a complete +concurrent-lifecycle design. + +## Machinery must justify its complexity + +- Apply the smallest safe solution first: existing helpers, standard idioms, then + new machinery. Avoid speculative wrappers, duplicate dispatch, or knobs. +- Require evidence for a performance optimization's payoff. Safety ownership, + protocol validation, and error handling do **not** need a throughput gain to + justify their existence. +- Before calling a guard redundant, prove the state unreachable through all + supported callers. Distinguish a genuinely fixed bound from user-supplied data. +- A proposed cut must preserve diagnostics, lifetime, fallback, and public API + behavior. Build and exercise a smaller alternative before presenting it as + equivalent; otherwise label it an unverified suggestion. +- Separate a scoped draft experiment from merge readiness. Pending measurements + are not a proven performance regression, and "no reproduced blockers" is not + proof of safety or sufficient reason to approve a performance claim. + +## Evaluate measurements, not just tables + +This skill evaluates performance evidence. It does not introduce a new profiler +implementation or require a separate profiler agent. When new measurements are +requested and the runtime is available, use the +[mssql-profiler skill](../mssql-profiler/SKILL.md) to operate the existing +profiler. Otherwise review the available artifacts and state the evidence gaps; +do not start an unrelated benchmark merely because a review is running. + +### Workload validity + +- Read the requirement and confirm that the benchmark reaches the changed path. + An override optimization needs declared overrides; a cache benefit needs hits. + Existing benchmarks can be excellent fallback controls while never using the + proposed fast path. +- Include compatible repeated inputs, shape changes, and excluded inputs. An + all-or-nothing cache can miss every batch containing a NULL, date, decimal, or + DAE value even if most of the batch is otherwise eligible. Inspect the actual + eligibility rule rather than treating an exclusion as an inherent limitation. +- Keep setup from invalidating the state under measurement. For example, issuing + `TRUNCATE` through the same cursor between inserts can defeat prepared-query + reuse. Distinguish cold setup, warmup, and steady-state measurements. +- Verify current values, affected rows, and relevant operation counts outside + timed regions where possible. Use existing counters/logging for correctness; + use normal production logging settings for timing. + +### Provenance and controls + +- Use separate, pinned base/PR builds with matching interpreter, optimization, + dependencies, SQL Server, and workload. Record dirty source changes, build + flags, native binary identity, OS/architecture, warmups, repetitions, and units. +- Verify both the imported package and the actual native extension path. A `.py` + loader can legitimately load the `.so`/`.pyd`; its own path is not sufficient + provenance. Discard mislabeled or logging-contaminated runs. +- Use Release/`-DNDEBUG` for comparisons. Debug-only assertion costs are not + release regressions. Do not copy another PR's numbers onto a later change. +- Counterbalance base/PR order, retain raw per-round observations, and avoid + competing benchmarks or DB-heavy tests on the same machine/server. Alternation + reduces order bias; it does not remove all contention or server-state effects. +- Define the timing window: input generation, setup, execute, fetching, commit, + rollback, and cleanup are not interchangeable. Label generated data and + local-only results; do not present them as customer production measurements. + +### Profiler and uncertainty + +- Use matching profiling-enabled Release builds for attribution, and separately + compare uninstrumented Release builds for shipped latency. Verify whether the + native profiling API is compiled in; runtime-disabled instrumentation is not + the same configuration as instrumentation compiled out. +- Keep a single owner of process-wide profiling state. Reset measurement windows, + exclude warmups as stated, and wait for worker activity to finish before + collecting. Missing timer data is not proof that a path cost zero. +- Parent timers include nested instrumentation overhead. Skipping thousands of + bind calls can also skip thousands of timer records. Do not present that + instrumented percentage as the production speedup or sum overlapping phases. +- `SQLBindParameter` is not inherently a network round trip. An incomplete probe, + cumulative counters from several workloads, or omitted phases cannot establish + a universal upper bound on the optimization's benefit. +- Show dispersion and paired observations alongside medians. Inconclusive data + proves neither zero benefit nor absence of regression. Overlapping ranges or + a "delta smaller than spread" rule are not statistical significance tests. +- For normalized scores, show the numerator and denominator and confirm + comparability. A moving pyodbc denominator is a warning, not an automatic + verdict at a fixed percentage. Uncontrolled cross-run raw times do not repair + it; obtain a controlled comparison or state the attribution uncertainty. + +## Organization and runtime evidence + +- Cohesive new units belong in purpose-named headers such as `param_detect.hpp`, + `py_type_cache.hpp`, or `py_ref.hpp`, not as another unrelated block in + `ddbc_bindings.cpp`. Keep structural suggestions separate from perf defects. +- Make a pure relocation its own `REFACTOR` commit when implementing it. Do not + mix a new policy or optimization into a claimed behavior-neutral extraction. +- Inspect the include graph. Forward declarations can break cycles when + owning-type operations are defined out of line. Keep required template + definitions or explicit instantiations available; forward declarations are + not inherently invalid. +- A successful compile/link is not enough for a native extension. Import the + rebuilt module and exercise relevant instantiations. A missing symbol can + surface only at load time. Trace transitive includes before calling a missing + direct include a current build failure; explicit dependency hygiene is separate. +- Use the existing [build](../../prompts/build-ddbc.prompt.md) and + [test](../../prompts/run-tests.prompt.md) guidance and supported local tooling. + Run targeted behavioral coverage; ownership/refcount changes also need broader + regression coverage and lifetime checks such as `gc.collect()` and `weakref`. + Neither replaces sanitizer/fault-injection evidence for claims requiring it. +- Exercise public operation sequences. Catalog methods may replace the handle + and clear Python flags before the native wrapper runs. An internal test that + sets those flags manually does not by itself prove a hidden public-API defect. +- Use short explicit pytest IDs for huge strings/bytes: pytest's current-test + environment value can exceed Windows' 32,767-character limit before the + product code runs. +- Prefer connection-local temporary tables or unique run-owned objects. Clean + only resources created by the repro; do not drop someone else's object to + make a test green. Separate stale database state from a driver regression. +- Preserve failing command exit codes when piping logs. Report failures and + isolated retries accurately instead of rewriting them as a clean full-suite + run. Build success, import success, and runtime success are different evidence. +- A passing comparison PR does not establish the cause of a crash. A universal2 + build does not prove both architectures ran. State platform and fault-injection + limits explicitly; do not turn an untested teardown theory into a standing rule. + +## Report + +Lead with the outcome and keep the report proportional to the change. + +- **Confirmed correctness/parity defects:** caller-visible impact, minimal + reproducer and actual result, current file/line anchor, PR-versus-base status, + and a concrete fix direction. +- **Performance observations:** named workload and eligibility, revisions, + measurement mode, operation counts, timings, variability, and limitations. +- **Unverified concerns:** missing evidence and the smallest test that would + settle the question. Never silently upgrade source inspection to runtime proof. +- **Structural/simplification suggestions:** what moves or disappears, what + replaces it, why it remains safe, and whether equivalence was demonstrated. + +Do not pile on an existing review thread or present naming/formatting preferences +as blockers. Distinguish fixed, disproved, pre-existing, and still-open findings. +Use plain, impact-first wording. Keep a top-level review summary short; put +technical evidence in the associated finding. If no blocker was established, +say that precisely rather than asserting that every platform/path is safe. + +API references: +[SQLDisconnect](https://learn.microsoft.com/en-us/sql/odbc/reference/syntax/sqldisconnect-function) +and [SQLFreeHandle](https://learn.microsoft.com/en-us/sql/odbc/reference/syntax/sqlfreehandle-function). diff --git a/.github/workflows/pr-format-check.yml b/.github/workflows/pr-format-check.yml index 371d60063..69f2ac423 100644 --- a/.github/workflows/pr-format-check.yml +++ b/.github/workflows/pr-format-check.yml @@ -22,7 +22,7 @@ jobs: // Validate title prefix for all contributors const validTitlePrefixes = [ 'FEAT:', 'CHORE:', 'FIX:', 'DOC:', 'STYLE:', 'REFACTOR:', - 'PERF:', 'RELEASE:' + 'PERF:', 'RELEASE:', 'AI:' ]; const hasValidPrefix = validTitlePrefixes.some(prefix => diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c23bcb56f..fdb5ba8ec 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -35,8 +35,12 @@ If you are a Microsoft organization member (internal contributor): All pull requests must include: -- **Valid Title Prefix**: Your PR title must start with one of: `FEAT:`, `CHORE:`, `FIX:`, `DOC:`, `STYLE:`, `REFACTOR:`, `PERF:`, or `RELEASE:` +- **Valid Title Prefix**: Your PR title must start with one of: `FEAT:`, `CHORE:`, `FIX:`, `DOC:`, `STYLE:`, `REFACTOR:`, `PERF:`, `RELEASE:`, or `AI:` - **Meaningful Summary**: Include a clear description of your changes under the "### Summary" section in the PR description (minimum 10 characters) - **Issue/Work Item Link** (only one required): - External contributors: Link to a GitHub issue - Microsoft org members: Link to an ADO work item + +Use `AI:` for changes to AI tooling, agents, skills, prompts, or AI-assisted +development workflows. It describes the subject of the change, not whether +AI helped write it; ordinary driver fixes and features keep their usual prefixes.