Repository navigation
Sam/win sdk speedup - #3675
Closed
sxlijin wants to merge 10 commits into
Closed
Sam/win sdk speedup#3675sxlijin wants to merge 10 commits into
sxlijin wants to merge 10 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBuild a shared Python wheel once into ChangesSDK test setup and CI wiring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a78fba3da2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sed -i.bak '/^\[tool\.uv\.sources\]$/,/^$/d' "$fixture_dir/pyproject.toml" | ||
| rm -f "$fixture_dir/pyproject.toml.bak" | ||
| echo "==> uv sync in $fixture_dir" | ||
| (cd "$fixture_dir" && "$uv_bin" sync) |
There was a problem hiding this comment.
When rerunning the Python SDK tests in the same checkout after changing the Rust/Python native code, this plain uv sync can leave the existing fixture .venv using the previously installed baml_core wheel: the setup script rebuilds a wheel with the same package version and filename, but no longer uses the editable install that maturin develop updated in place. I checked uv sync --help, and --reinstall-package is the option that forces a package to be reinstalled regardless of whether it is already installed; without that here (and in the PowerShell twin), local incremental test runs can exercise a stale extension even though setup just rebuilt the wheel.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
baml_language/sdk_tests/harness_setup/src/python_pydantic2.rs (1)
13-21: ⚡ Quick winModule docs still reference the old
--reinstall-package baml_corerebuild path.Lines 18-21 state setup.sh passes
--reinstall-package baml_coreso the maturin-built.sois rebuilt on incremental edits. With approach B (per the new comment at Line 212), setup now installs a prebuilt wheel and no longer rebuilds via that flag. Align this module doc with the wheel-based flow.🤖 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 `@baml_language/sdk_tests/harness_setup/src/python_pydantic2.rs` around lines 13 - 21, The module doc in python_pydantic2.rs still claims setup.sh passes `--reinstall-package baml_core` to rebuild the maturin .so; update that top-level comment to reflect the wheel-based flow: remove or replace the `--reinstall-package baml_core` claim and state that setup.sh now installs a prebuilt wheel (no maturin rebuild on incremental Rust edits), and optionally reference the alternative approach described later (the "approach B" note) for clarity. Target the module doc block and the mention of setup.sh and `--reinstall-package baml_core` so the comment aligns with the new wheel installation behavior.baml_language/sdk_tests/crates/python_pydantic2/setup.ps1 (1)
9-18: ⚡ Quick winStale header comment contradicts the new wheel-based flow.
Lines 9-18 still describe the old "
uv synceditable-links baml_core, then a singlematurin developrebuilds the shared extension" approach, which the EXPERIMENT block at Line 38 replaces. Update this header so the rationale matches approach B.🤖 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 `@baml_language/sdk_tests/crates/python_pydantic2/setup.ps1` around lines 9 - 18, The header comment in setup.ps1 still describes the old "uv sync editable-links baml_core then a single maturin develop" flow; update it to describe the new wheel-based flow (Approach B) used by the EXPERIMENT block referenced at Line 38: remove references to editable-links and a single maturin develop against sdks/python/.venv, and instead explain that each fixture uses prebuilt/installable wheels (or a single wheel cache) to populate venvs and that this eliminates the ephemeral-interpreter/rebuild fingerprint problem; keep the brief rationale about performance and why the wheel approach is preferred.baml_language/sdk_tests/crates/python_pydantic2/setup.sh (1)
10-47: ⚡ Quick winStale header comment describes the removed
maturin develop/--reinstall-packageflow.This large header block (responsibilities split, the "Why not
uv sync --reinstall-package baml_core" rationale, "step 2 owns rebuilds") no longer matches the wheel-based approach B introduced at Line 66. Refresh it to avoid misleading future maintainers.🤖 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 `@baml_language/sdk_tests/crates/python_pydantic2/setup.sh` around lines 10 - 47, Update the stale header in setup.sh to reflect the new wheel-based workflow: remove or rewrite the sections that reference "maturin develop", "--reinstall-package baml_core", and the old two-step rationale (the "step 2 owns rebuilds" / "Why not uv sync --reinstall-package baml_core" paragraphs) and replace them with a concise description of the current behavior (uv sync still creates per-fixture .venv installs, while the build step now produces/installes a wheel into the pinned venv to keep pyo3 fingerprint stable), calling out the performance and reproducibility benefits of the wheel approach compared to the old maturin/editable path; keep mentions of "uv sync" and the intent to re-run after bridge_python Rust changes so maintainers can locate the relevant logic.
🤖 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 `@baml_language/sdk_tests/crates/python_pydantic2/setup.ps1`:
- Around line 73-103: The script unconditionally strips the `[tool.uv.sources]`
section and sets $env:UV_FIND_LINKS = $WheelDir; change this so both actions run
only when the win-sdk-speedup experiment is active (e.g., when UV_FIND_LINKS is
already provided or when a flag like WIN_SDK_EXP == 'true'); wrap the deletion
loop that looks for '^\[tool\.uv\.sources\]$' and the `$env:UV_FIND_LINKS =
$WheelDir` assignment in a guard that checks `$env:UV_FIND_LINKS -or
($env:WIN_SDK_EXP -eq 'true')`, so normal CI leaves the `[tool.uv.sources]`
block intact and does not override resolution unless the experiment is signaled.
---
Nitpick comments:
In `@baml_language/sdk_tests/crates/python_pydantic2/setup.ps1`:
- Around line 9-18: The header comment in setup.ps1 still describes the old "uv
sync editable-links baml_core then a single maturin develop" flow; update it to
describe the new wheel-based flow (Approach B) used by the EXPERIMENT block
referenced at Line 38: remove references to editable-links and a single maturin
develop against sdks/python/.venv, and instead explain that each fixture uses
prebuilt/installable wheels (or a single wheel cache) to populate venvs and that
this eliminates the ephemeral-interpreter/rebuild fingerprint problem; keep the
brief rationale about performance and why the wheel approach is preferred.
In `@baml_language/sdk_tests/crates/python_pydantic2/setup.sh`:
- Around line 10-47: Update the stale header in setup.sh to reflect the new
wheel-based workflow: remove or rewrite the sections that reference "maturin
develop", "--reinstall-package baml_core", and the old two-step rationale (the
"step 2 owns rebuilds" / "Why not uv sync --reinstall-package baml_core"
paragraphs) and replace them with a concise description of the current behavior
(uv sync still creates per-fixture .venv installs, while the build step now
produces/installes a wheel into the pinned venv to keep pyo3 fingerprint
stable), calling out the performance and reproducibility benefits of the wheel
approach compared to the old maturin/editable path; keep mentions of "uv sync"
and the intent to re-run after bridge_python Rust changes so maintainers can
locate the relevant logic.
In `@baml_language/sdk_tests/harness_setup/src/python_pydantic2.rs`:
- Around line 13-21: The module doc in python_pydantic2.rs still claims setup.sh
passes `--reinstall-package baml_core` to rebuild the maturin .so; update that
top-level comment to reflect the wheel-based flow: remove or replace the
`--reinstall-package baml_core` claim and state that setup.sh now installs a
prebuilt wheel (no maturin rebuild on incremental Rust edits), and optionally
reference the alternative approach described later (the "approach B" note) for
clarity. Target the module doc block and the mention of setup.sh and
`--reinstall-package baml_core` so the comment aligns with the new wheel
installation behavior.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 27b2007b-1ade-4871-a498-e270fc3f182b
📒 Files selected for processing (6)
.github/workflows/cargo-tests.reusable.yamlbaml_language/sdk_tests/crates/python_pydantic2/setup.ps1baml_language/sdk_tests/crates/python_pydantic2/setup.shbaml_language/sdk_tests/crates/typescript_node/setup.ps1baml_language/sdk_tests/crates/typescript_node/setup.shbaml_language/sdk_tests/harness_setup/src/python_pydantic2.rs
Comment on lines
+73
to
+103
| # 2. Per fixture: install deps + the prebuilt baml_core wheel. No | ||
| # per-fixture cdylib build. Strip the editable `[tool.uv.sources] | ||
| # baml_core` block so baml_core resolves as a normal dependency from | ||
| # UV_FIND_LINKS (set to $WheelDir; the local 0.1.3 wheel outranks any | ||
| # public release). Plain `uv sync`/`uv run` then installs the prebuilt | ||
| # wheel + dev tools without ever building baml_core. | ||
| if (-not $env:UV_FIND_LINKS) { $env:UV_FIND_LINKS = $WheelDir } | ||
| Get-ChildItem -Directory | ForEach-Object { | ||
| $generated = Join-Path $_.FullName 'generated' | ||
| if (Test-Path $generated) { | ||
| # Delete the [tool.uv.sources] section (header → next blank line). | ||
| $pp = Join-Path $generated 'pyproject.toml' | ||
| $lines = Get-Content $pp | ||
| $out = New-Object System.Collections.Generic.List[string] | ||
| $skip = $false | ||
| foreach ($line in $lines) { | ||
| if ($line -match '^\[tool\.uv\.sources\]$') { $skip = $true; continue } | ||
| if ($skip) { if ($line -match '^\s*$') { $skip = $false }; continue } | ||
| $out.Add($line) | ||
| } | ||
| Set-Content -Path $pp -Value $out | ||
| Write-Host "==> uv sync in $($_.Name)/generated" | ||
| Push-Location $generated | ||
| try { | ||
| & $UvBin sync | ||
| if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } | ||
| } finally { | ||
| Pop-Location | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether these setup scripts run on non-experiment branches (canary)
# and whether any branch/env gating exists around the wheel approach.
fd -t f 'nextest.toml' --hidden | xargs -I{} sh -c 'echo "== {} =="; cat "{}"'
rg -nP 'WIN_SDK_EXP|UV_FIND_LINKS|win-sdk-speedup' .github/workflows/cargo-tests.reusable.yamlRepository: BoundaryML/baml
Length of output: 4414
Gate [tool.uv.sources] stripping + UV_FIND_LINKS fallback to the win-sdk-speedup experiment
- In
baml_language/sdk_tests/crates/python_pydantic2/setup.ps1(lines 73-103), the script always deletes the[tool.uv.sources]block and sets$env:UV_FIND_LINKS = $WheelDirwhenUV_FIND_LINKSis not already set. - In
.github/workflows/cargo-tests.reusable.yaml,UV_FIND_LINKSis populated only forsam/win-sdk-speedup; normal runs intentionally leave it empty, so this script overrides that intent and changes resolution away from the editable source approach. - Fix by conditioning both the strip and the
$env:UV_FIND_LINKSdefault on the experiment signal (e.g., only when the workflow providesUV_FIND_LINKS/WIN_SDK_EXP == 'true'), so non-experiment CI preserves the editable[tool.uv.sources]behavior.
🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[warning] Missing BOM encoding for non-ASCII encoded file 'setup.ps1'
(PSUseBOMForUnicodeEncodedFile)
🤖 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 `@baml_language/sdk_tests/crates/python_pydantic2/setup.ps1` around lines 73 -
103, The script unconditionally strips the `[tool.uv.sources]` section and sets
$env:UV_FIND_LINKS = $WheelDir; change this so both actions run only when the
win-sdk-speedup experiment is active (e.g., when UV_FIND_LINKS is already
provided or when a flag like WIN_SDK_EXP == 'true'); wrap the deletion loop that
looks for '^\[tool\.uv\.sources\]$' and the `$env:UV_FIND_LINKS = $WheelDir`
assignment in a guard that checks `$env:UV_FIND_LINKS -or ($env:WIN_SDK_EXP -eq
'true')`, so normal CI leaves the `[tool.uv.sources]` block intact and does not
override resolution unless the experiment is signaled.
… + pnpm store) Cumulative experiment HEAD on branch sam/win-sdk-speedup. - A: -p PKG on both nextest commands (run 26877027313: Py 12m47→7m28, TS 10m32→5m34). - C/D: sdk-tests rust-cache cache-targets:true + save-if:true under a per-SDK key (windows-sdk-exp-<sdk>) on the experiment branch only; persists target/ (warms maturin/napi cold cdylib link sccache can't cache) and the pnpm store under target/pnpm-store. Needs one populating run, then a warm run to measure. Normal CI (workflow_call) keeps cache-targets:false / save-if:false.
…ries rust-cache's default cleanup deletes non-cargo dirs (target/pnpm-store), so the pnpm store came back cold each run (pnpm install ~79s, reused 0). Add it to cache-directories (cached as-is, no cleanup) so it warms. Needs populate+warm.
…-negative) D dropped: caching the ~hundreds-MB pnpm store via rust-cache cost more in restore (+40s..+190s) than the ~79s cold pnpm install it saved. Keeping A (-p scope), C (persist target/ per-SDK), B (prebuilt baml_core wheel + UV_FIND_LINKS). This is a populate run for the post-D cache key; warm run measures the final state. ci(experiment): warm run — measure final A+C+B
sxlijin
force-pushed
the
sam/win-sdk-speedup
branch
from
June 3, 2026 22:15
a78fba3 to
60eac50
Compare
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
Binary size checks passed✅ 7 passed
Generated by |
bridge_nodejs is self-contained: standalone pnpm-lock.yaml, no workspace: deps, fixtures depend on it via file: path. Step 2 (pnpm install --ignore-workspace in bridge_nodejs) already populates its node_modules. The repo-root 'pnpm install' (step 1) only pulled the whole 39-project workspace (~2928 pkgs: canvas, turbo, vercel, vscode-ext, codemirror grammar build...) — none needed by sdk_tests. Drop it.
…che hits) With sccache, most compile units are cache lookups/fetches from R2 (I/O-bound, ~0.2s each) rather than CPU work, so cargo's default parallelism (= core count, only 4 on the Windows runner) leaves the CPU idle waiting on the network. Bump build parallelism to 16 workflow-wide so cache-hit fetches overlap. On cold/miss -heavy builds this oversubscribes the 4 cores, but the steady state is hits.
…evert target/ persistence Productionize the SDK-test speedups: keep A (-p scope nextest), B (prebuilt baml_core wheel + UV_FIND_LINKS), H (skip repo-root pnpm install). Remove the experiment scaffolding (push trigger, per-job if-gates, windows-only matrix filter, WIN_SDK_EXP gating). Revert C (target/ persistence) — GHA cache hit rate is poor. Bump CARGO_BUILD_JOBS to 32 to overlap sccache I/O-bound hits.
sxlijin
force-pushed
the
sam/win-sdk-speedup
branch
from
June 4, 2026 05:30
db80794 to
9afdecc
Compare
sxlijin
force-pushed
the
sam/win-sdk-speedup
branch
from
June 4, 2026 05:43
9afdecc to
ea04bf9
Compare
sxlijin
force-pushed
the
sam/win-sdk-speedup
branch
from
June 4, 2026 06:13
ea04bf9 to
932d09b
Compare
sxlijin
force-pushed
the
sam/win-sdk-speedup
branch
from
June 4, 2026 06:16
932d09b to
3c84202
Compare
sxlijin
force-pushed
the
sam/win-sdk-speedup
branch
from
June 4, 2026 06:30
3c84202 to
139ad10
Compare
sxlijin
force-pushed
the
sam/win-sdk-speedup
branch
from
June 4, 2026 06:42
139ad10 to
a7f5c7d
Compare
2 arms on cargo test (windows), --all-features: cc-default vs cc-sccache (CC routed through the baml-sccache R2 shim, default cc builder). Run 1 = cold (populate C cache); a 2nd run measures the warm aws-lc-sys run-custom-build drop vs cc-default. Confirms the Approach M mechanism (research: cc-rs supports the CC shim; CMAKE_*_COMPILER_LAUNCHER not forwarded by aws-lc-sys). M is inferior to ring (which deletes aws-lc-sys) — interim/fallback only. build-caching/04 §5.3 M.
sxlijin
force-pushed
the
sam/win-sdk-speedup
branch
from
June 4, 2026 06:55
a7f5c7d to
4a8cd08
Compare
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Chores
Documentation