Skip to content

ci(integrated-benchmark): scenarios without lockfiles#11838

Merged
zkochan merged 5 commits into
mainfrom
claude/magical-euler-NLjCu
May 22, 2026
Merged

ci(integrated-benchmark): scenarios without lockfiles#11838
zkochan merged 5 commits into
mainfrom
claude/magical-euler-NLjCu

Conversation

@KSXGitHub

@KSXGitHub KSXGitHub commented May 21, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Chores
    • Updated benchmarking workflow to include two additional performance scenarios: "Clean Install" and "Full Resolution", with expanded summary output showing trimmed JSON results.
    • Improved benchmark fixture workspace handling to force single-project installs when project globs are omitted, reducing edge-case inconsistencies.

Review Change Stack

claude added 2 commits May 21, 2026 19:32
Wires `clean-install`, `full-resolution`, `peek`, and `gvs-warm` into
`pacquet-integrated-benchmark.yml` so per-PR runs cover the same scenario
set the manual `benchmark.yml` workflow already exercises via
`benchmarks/bench.sh`. Requested for #11837, where the perf delta affects
the resolution-bound scenarios (`firstInstall`, `withWarmCache`,
`withWarmModules`, `updatedDependencies`) that the prior two-scenario set
did not measure.

Each scenario gets its own step with a 10 min hyperfine timeout (same
rationale as the existing steps) and writes per-scenario report copies
that the summary step concatenates into `SUMMARY.md`.
Keep only the two new no-lockfile scenarios (`clean-install`,
`full-resolution`) on top of the existing `frozen-lockfile` and
`frozen-lockfile-hot-cache`. #11837's perf change is in the
fresh-lockfile install path, which only runs when resolution runs — i.e.,
exactly the no-lockfile scenarios. `peek` mutates an existing lockfile
and `gvs-warm` is a frozen-lockfile variant; neither exercises the
affected path, and including them only costs per-PR CI wall time.
@coderabbitai

coderabbitai Bot commented May 21, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 7c14fd55-c7cc-45b1-801a-c63acc923128

📥 Commits

Reviewing files that changed from the base of the PR and between bc0920c and d130e0f.

📒 Files selected for processing (1)
  • pacquet/tasks/integrated-benchmark/src/cli_args.rs
📜 Recent review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Agent
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/*.rs: When porting a function that fires pnpm:<channel> events through globalLogger, logger.debug(), or streamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's.
Declare a newtype wrapper for branded string types. Do not collapse the brand into a plain String or &str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only via TryFrom<String> and/or FromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallible From<String> (and From<&str> when convenient).
If upstream occasionally constructs without validation, expose from_str_unchecked as an escape hatch alongside the validating constructor.
Match upstream serde behavior for branded types that cross JSON, YAML, or INI boundaries. Use #[serde(try_from = "String")] for deserialization and #[serde(into = "String")] for serialization.
Use #[derive(derive_more::From)] and #[derive(derive_more::Into)] for mechanical conversion impls. Fall back to manual impl only when conversion needs custom logic.
String-literal unions should become enums, not newtype wrappers. Model closed sets of valid string values as enums.
Template literal types should be treated as branded strings with validation discipline from rules 2-5.
Choose owned vs. borrowed parameters to minimize copies. Widen to the most encompassing type (&Path over &PathBuf, &str over &String) when it doesn't force extra copies.
Prefer Arc::clone(&x) / Rc::clone(&x) over x.clone() for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming conventions.
Do not use star imports inside module bodies. Write use super::{Foo, bar} instead of use super::*;. Two forms stay allowed: external-crate preludes like use rayon::prelude::*; and root-of-module re-...

Files:

  • pacquet/tasks/integrated-benchmark/src/cli_args.rs
🧠 Learnings (1)
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.

Applied to files:

  • pacquet/tasks/integrated-benchmark/src/cli_args.rs
🔇 Additional comments (1)
pacquet/tasks/integrated-benchmark/src/cli_args.rs (1)

131-131: LGTM!


📝 Walkthrough

Walkthrough

Adds CI steps for Clean Install and Full Resolution benchmarks and embeds their trimmed JSON reports into the generated summary; introduces an optional packages field in workspace manifest and forces ["."] when absent to restrict workspace discovery for benchmark installs.

Changes

Benchmark Execution and Reporting

Layer / File(s) Summary
Benchmark execution steps
.github/workflows/pacquet-integrated-benchmark.yml
Adds "Benchmark: Clean Install" and "Benchmark: Full Resolution" steps that invoke just integrated-benchmark with their respective scenarios and copy generated markdown and JSON reports to scenario-specific bench-work-env artifacts.
Summary reporting for new scenarios
.github/workflows/pacquet-integrated-benchmark.yml
Extends the "Generate summary" step to include "Scenario: Clean Install" and "Scenario: Full Resolution" sections with rendered markdown reports and embedded JSON blocks (with .results[].exit_codes removed) from the scenario report files.

Workspace Manifest and Workspace Creation

Layer / File(s) Summary
MinimalWorkspaceManifest: optional packages field
pacquet/tasks/integrated-benchmark/src/workspace_manifest.rs
Adds packages: Option<Vec<String>> to MinimalWorkspaceManifest with serde skip-if-none and documentation describing enforced workspace glob behavior for benchmarks.
create_pnpm_workspace enforces single-project install
pacquet/tasks/integrated-benchmark/src/work_env.rs
When manifest.packages is absent, sets manifest.packages = ["."] before serializing the workspace manifest, constraining pnpm workspace project discovery to the repository root for benchmark installs.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • pnpm/pnpm#11824: Updates FullResolution handling to stop passing --no-frozen-lockfile, aligning CLI/workflow behavior with the new scenario mapping.
  • pnpm/pnpm#11741: Prior integrated-benchmark consolidation and workspace handling changes that this PR builds upon.

Suggested reviewers

  • zkochan

Poem

🐰 I hopped through workflows, tidy and fleet,
Added clean installs and full resolution neat,
Manifest whispers "packages: ["."]" to stay,
Reports trimmed and nested in markdown display,
Metrics hop out bright — a rabbit's small feat.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'ci(integrated-benchmark): scenarios without lockfiles' directly reflects the main change: adjusting benchmark scenarios to handle no-lockfile cases by pinning workspace packages and adjusting timeouts.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/magical-euler-NLjCu

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@KSXGitHub KSXGitHub marked this pull request as ready for review May 21, 2026 19:43
@KSXGitHub KSXGitHub requested a review from zkochan as a code owner May 21, 2026 19:43
Copilot AI review requested due to automatic review settings May 21, 2026 19:43

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.

Pull request overview

Enables the pacquet integrated benchmark workflow to run and report additional scenarios that do not rely on lockfiles, expanding CI benchmark coverage beyond the existing frozen-lockfile scenarios.

Changes:

  • Re-enabled the clean-install integrated-benchmark scenario in CI, with a step-level timeout and artifact copies.
  • Added a new CI step for the full-resolution integrated-benchmark scenario, also with a timeout and artifact copies.
  • Extended the generated SUMMARY.md to include both new scenarios (markdown + JSON details).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@KSXGitHub KSXGitHub marked this pull request as draft May 21, 2026 20:44
The integrated-benchmark clones each pacquet revision's source tree into
`<bench_dir>/pacquet/`, which on the pnpm/pnpm monorepo includes upstream
test fixtures like
`workspace/project-manifest-reader/__fixtures__/invalid-package-json/package.json`
— intentionally malformed JSON used to exercise pnpm's manifest reader.

Without a `packages:` field, both pnpm's `findPackages.ts:28` and
pacquet's `crates/workspace/src/projects.rs:128` default to `[".", "**"]`,
so the fresh-resolve install path's `find_workspace_projects` walk
descends into the cloned source tree and trips on the bad fixture:

  Error: pacquet_package_manifest::serialization_error
    × installing dependencies
    ╰─▶ expected `,` or `}` at line 3 column 3

The walk only runs on the fresh-lockfile branch (`install.rs:628-630`),
which is why frozen-lockfile and frozen-lockfile-hot-cache stay green
while clean-install and full-resolution fail every time.

Pin `packages: ['.']` in the synthesized manifest so enumeration stays
at the workspace root. The benchmark's installs are single-project,
so this doesn't narrow anything the install actually needed to see.
Fixtures supplied via `--fixture-dir` that already declare `packages:`
keep their own value.
@github-actions

github-actions Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.00      8.0±0.10ms   543.5 KB/sec    1.02      8.1±0.48ms   532.6 KB/sec

@codecov-commenter

codecov-commenter commented May 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.50%. Comparing base (9eb632b) to head (d130e0f).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11838      +/-   ##
==========================================
- Coverage   87.51%   87.50%   -0.01%     
==========================================
  Files         204      204              
  Lines       24165    24371     +206     
==========================================
+ Hits        21149    21327     +178     
- Misses       3016     3044      +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Clean-install and full-resolution go through pacquet's fresh-resolve
install path, which is currently ~3-5x slower than pnpm on the
`alotta-files` fixture (#11832). Hyperfine's default 1 warmup
+ 10 timed runs across three benchmark targets (pacquet@HEAD,
pacquet@main, system pnpm) projects to ~13 min wallclock for these
two scenarios, putting the previous 10 min cap right on the edge.
Doubling to 20 min keeps the per-step timeout meaningful as a stuck-
install detector without losing CI time when the bench is healthy.

The frozen-lockfile steps stay at 10 min — they don't traverse the
slower fresh-resolve path.
@KSXGitHub KSXGitHub marked this pull request as ready for review May 22, 2026 00:36
Copilot AI review requested due to automatic review settings May 22, 2026 00:36

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.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@KSXGitHub KSXGitHub marked this pull request as draft May 22, 2026 01:13
Pacquet doesn't expose `--no-frozen-lockfile` (only `--frozen-lockfile`,
`--prefer-frozen-lockfile`, and `--no-prefer-frozen-lockfile`). Passing
it makes clap reject the install:

  error: unexpected argument '--no-frozen-lockfile' found
    tip: a similar argument exists: '--frozen-lockfile'

The flag was redundant for this scenario anyway: full-resolution starts
every iteration with no lockfile on disk (init() skips the lockfile when
`lockfile_enabled()` is false; cleanup removes it; `lockfile=false` in
the synthesized npmrc/workspace prevents writing one). With no lockfile
present the frozen path is unreachable regardless of the flag, so both
tools take fresh resolution by definition. Fold full-resolution into
clean-install's bare `install` arm.
@github-actions

Copy link
Copy Markdown
Contributor

Integrated-Benchmark Report (Linux)

Scenario: Frozen Lockfile

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 3.432 ± 0.513 2.751 4.307 1.00
pacquet@main 3.537 ± 0.955 2.639 5.851 1.03 ± 0.32
pnpm 4.692 ± 0.306 4.460 5.399 1.37 ± 0.22
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 3.432137791,
      "stddev": 0.5131474158415051,
      "median": 3.4728318377000003,
      "user": 2.2141706599999997,
      "system": 2.81793552,
      "min": 2.7508148867,
      "max": 4.3067551947,
      "times": [
        3.0623568537,
        3.6228358757,
        3.9016572137,
        4.3067551947,
        3.8663068227000004,
        3.3327167787,
        2.7508148867,
        2.8408256107,
        3.0241617767,
        3.6129468967
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 3.5374191417,
      "stddev": 0.955396071191703,
      "median": 3.1557366637,
      "user": 2.19220246,
      "system": 2.7995113199999997,
      "min": 2.6386430677000003,
      "max": 5.8513465057000005,
      "times": [
        3.9379318077,
        3.2755520987,
        4.2931606247000005,
        3.4410383697,
        5.8513465057000005,
        3.0060125027,
        2.6386430677000003,
        2.8661582027000003,
        3.0284270087,
        3.0359212287
      ]
    },
    {
      "command": "pnpm",
      "mean": 4.6919697311,
      "stddev": 0.3064921195362401,
      "median": 4.5391302327,
      "user": 6.19275546,
      "system": 3.0948177199999996,
      "min": 4.4595908297,
      "max": 5.3993428467,
      "times": [
        4.8065853177,
        4.4595908297,
        4.4665542807000005,
        4.5566583027,
        4.8314335437,
        5.3993428467,
        4.939259502700001,
        4.4647337997,
        4.5216021627,
        4.4739367247
      ]
    }
  ]
}

Scenario: Frozen Lockfile (Hot Cache)

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 1.065 ± 0.241 0.935 1.728 1.00
pacquet@main 1.375 ± 0.681 0.919 2.700 1.29 ± 0.70
pnpm 3.085 ± 0.915 2.219 4.816 2.90 ± 1.08
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 1.0645509687,
      "stddev": 0.2413425384534893,
      "median": 0.9662441136,
      "user": 0.31625896,
      "system": 1.2475041799999997,
      "min": 0.9347396261,
      "max": 1.7280357271,
      "times": [
        1.0326715241,
        0.9347396261,
        0.9704129311,
        1.0809227101,
        0.9620752961,
        0.9371096761,
        0.9451971141,
        0.9458568211,
        1.7280357271,
        1.1084882611
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 1.3754049080000001,
      "stddev": 0.6810200221010172,
      "median": 0.9717427330999999,
      "user": 0.31944016,
      "system": 1.2542702799999998,
      "min": 0.9189297301,
      "max": 2.6995864591000003,
      "times": [
        2.6995864591000003,
        2.4855410731000003,
        1.6172381091,
        1.2897171001,
        0.9980939551,
        0.9385917991,
        0.9350819971,
        0.9258773461,
        0.9453915111,
        0.9189297301
      ]
    },
    {
      "command": "pnpm",
      "mean": 3.084938983,
      "stddev": 0.915259332031806,
      "median": 2.7510851601,
      "user": 2.3868880599999995,
      "system": 1.64292318,
      "min": 2.2192529021,
      "max": 4.8155520831,
      "times": [
        4.8155520831,
        2.7691458211,
        2.6214956331,
        2.2192529021,
        2.7330244991000003,
        2.4690061301,
        2.4362879021,
        4.6525392950999995,
        3.2882912481,
        2.8447943161
      ]
    }
  ]
}

Scenario: Clean Install

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 30.477 ± 1.658 28.320 33.508 4.56 ± 0.37
pacquet@main 30.078 ± 1.310 27.491 31.706 4.50 ± 0.34
pnpm 6.681 ± 0.406 6.222 7.587 1.00
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 30.47734548176,
      "stddev": 1.6576371408923283,
      "median": 30.32630003036,
      "user": 37.143677620000005,
      "system": 6.436595079999999,
      "min": 28.31951969686,
      "max": 33.50848208986,
      "times": [
        28.31951969686,
        30.45625080886,
        32.34147678686,
        30.19634925186,
        28.99876924586,
        33.50848208986,
        31.39359800786,
        29.085849294859997,
        31.29807714386,
        29.17508249086
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 30.07770865406,
      "stddev": 1.3096417988378966,
      "median": 30.40315272736,
      "user": 36.730923819999994,
      "system": 6.43413398,
      "min": 27.49071027586,
      "max": 31.706271241859998,
      "times": [
        27.49071027586,
        28.742119391859998,
        29.07686431086,
        30.798967282859998,
        30.399892320859998,
        29.80396184786,
        30.40641313386,
        31.38445084786,
        30.96743588686,
        31.706271241859998
      ]
    },
    {
      "command": "pnpm",
      "mean": 6.6810260132599995,
      "stddev": 0.40647312340317987,
      "median": 6.52462765986,
      "user": 8.84956532,
      "system": 3.5214553800000004,
      "min": 6.22174590486,
      "max": 7.58677980286,
      "times": [
        6.93556216286,
        6.92150659086,
        6.22174590486,
        6.52445564486,
        6.43459128186,
        6.31546062386,
        6.52479967486,
        7.58677980286,
        6.88434815986,
        6.46101028586
      ]
    }
  ]
}

Scenario: Full Resolution

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 29.391 ± 2.382 26.927 34.359 7.04 ± 1.37
pacquet@main 26.497 ± 1.338 24.220 28.495 6.35 ± 1.16
pnpm 4.174 ± 0.736 3.714 6.164 1.00
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 29.390766097939995,
      "stddev": 2.381549581403614,
      "median": 28.586513870939996,
      "user": 35.94428694,
      "system": 5.41284692,
      "min": 26.92729011794,
      "max": 34.35905947994,
      "times": [
        34.35905947994,
        32.30388097894,
        30.40443093994,
        28.856626388939997,
        26.92729011794,
        27.897087176939998,
        29.73119255894,
        27.19584418194,
        27.915847802939997,
        28.316401352939998
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 26.49727573494,
      "stddev": 1.3379078094887558,
      "median": 26.463745728939998,
      "user": 32.15257064000001,
      "system": 5.35436672,
      "min": 24.22016352394,
      "max": 28.49459493094,
      "times": [
        28.49459493094,
        26.589255834939998,
        27.567866696939998,
        28.015991541939997,
        26.73843733794,
        26.338235622939997,
        24.22016352394,
        24.749807145939997,
        25.94716726394,
        26.31123744994
      ]
    },
    {
      "command": "pnpm",
      "mean": 4.17401299524,
      "stddev": 0.7363504401433941,
      "median": 3.92708770094,
      "user": 4.269346239999999,
      "system": 1.9943813199999993,
      "min": 3.7136983849400003,
      "max": 6.16373130594,
      "times": [
        3.8270794279400002,
        4.01078211594,
        3.8446492559400003,
        4.34985362394,
        3.7136983849400003,
        3.74822444194,
        6.16373130594,
        4.33323201594,
        4.00952614594,
        3.73935323394
      ]
    }
  ]
}

@KSXGitHub KSXGitHub marked this pull request as ready for review May 22, 2026 02:33
Copilot AI review requested due to automatic review settings May 22, 2026 02:33

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.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

@zkochan zkochan merged commit 815e507 into main May 22, 2026
32 checks passed
@zkochan zkochan deleted the claude/magical-euler-NLjCu branch May 22, 2026 06:03
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.

5 participants