Reuse run.sh in pr-comparison.yml; remove broken validate-only preflight

Confirmed live on PR #31: the "Validate config" step used -e reporting
(and run.sh's own preflight used -e default), but implementations_for_pixi_env()
rejects any PIXI_ENVIRONMENT_NAME outside its real-env allowlist - so this
preflight was broken for any config calling it (all_models.py and anything
derived from it, e.g. this PR's own all_models_ridge_only.py), unconditionally,
regardless of which real environment would go on to run.

- run.sh: drop the recently-added --validate-only preflight block and its
  now-unused extract_configs helper.
- scripts/classify_bench_session.py: drop the now-dead special-case for
  run.sh's removed --validate-only header line.
- .github/workflows/pr-comparison.yml: reuse run.sh directly (one
  invocation with two env@owner:ref entries) instead of duplicating
  setup_sklearn_ref.sh + sklbench calls per side - matches
  CONTRIBUTING.md's documented PR-vs-base comparison pattern exactly, and
  collapses two log files/classify calls into one. Keep a config
  preflight, but scoped to this workflow with a real, always-installable
  env (sklearn-pypi) instead of a broken one - still worth catching a
  typo'd config before a many-minutes sklearn-dev clone+build.
- scripts/resolve_sklearn_pr.py: output the fork's `owner` (matching
  run.sh's env@owner:ref shorthand) instead of a full clone_url, and
  validate the fork's repo is actually named "scikit-learn" (what that
  shorthand assumes) rather than silently mishandling a differently-named
  fork.
- tests/test_config_loading.py: register all_models_ridge_only.py in
  ENV_SENSITIVE_CONFIGS alongside the other all_models.py-derived configs.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Author: cakedev0

.github/workflows/pr-comparison.yml

@@ -22,7 +22,7 @@ jobs:
       config: ${{ steps.parse.outputs.config }}
       runners: ${{ steps.parse.outputs.runners }}
       sklearn_pr_number: ${{ steps.parse.outputs.sklearn_pr_number }}
-      sklearn_clone_url: ${{ steps.resolve.outputs.clone_url }}
+      sklearn_owner: ${{ steps.resolve.outputs.owner }}
       sklearn_head_ref: ${{ steps.resolve.outputs.head_ref }}
     steps:
       - name: Check out repository (base, trusted)
@@ -138,7 +138,7 @@ jobs:
       PR_NUMBER: ${{ github.event.pull_request.number }}
       CONFIG: ${{ needs.gate_and_resolve.outputs.config }}
       SKLEARN_PR: ${{ needs.gate_and_resolve.outputs.sklearn_pr_number }}
-      SKLEARN_CLONE_URL: ${{ needs.gate_and_resolve.outputs.sklearn_clone_url }}
+      SKLEARN_OWNER: ${{ needs.gate_and_resolve.outputs.sklearn_owner }}
       SKLEARN_HEAD_REF: ${{ needs.gate_and_resolve.outputs.sklearn_head_ref }}
     steps:
       - name: Set scratch directories
@@ -174,7 +174,10 @@ jobs:
         uses: prefix-dev/setup-pixi@5185adfbffb4bd703da3010310260805d89ebb11 # v0.9.6
         with:
           pixi-version: v0.75.0
-          environments: reporting
+          # sklearn-pypi (lightweight, always installable, no sklearn-src/
+          # path dependency) is used to validate the config below before
+          # committing to the much slower sklearn-dev clone+build.
+          environments: reporting sklearn-pypi
           locked: false
           frozen: true
           cache: false
@@ -185,35 +188,31 @@ jobs:
         run: rm -rf "${SKLBENCH_DATA_CACHE:-data_cache}/preprocessed"
 
       - name: Validate config
-        run: pixi run --frozen -e reporting python -m sklbench --config "$CONFIG" --validate-only
-
-      - name: Set up scikit-learn PR branch
-        run: ./scripts/setup_sklearn_ref.sh --remote "$SKLEARN_CLONE_URL" --ref "$SKLEARN_HEAD_REF" --env sklearn-dev
-
-      - name: Run benchmark (PR branch)
-        continue-on-error: true
-        run: |
-          pixi run --frozen -e sklearn-dev python -m sklbench --config "$CONFIG" --results-dir "$RESULTS_DIR" \
-            2>&1 | tee "$RUNNER_TEMP/bench-pr.log"
-
-      - name: Set up scikit-learn main
-        run: ./scripts/setup_sklearn_ref.sh --remote https://github.com/scikit-learn/scikit-learn.git --ref main --env sklearn-dev
+        # A prior version of this check lived in run.sh itself, using
+        # `-e default` - broken for any config calling
+        # implementations_for_pixi_env() (e.g. all_models.py and anything
+        # derived from it), since "default" isn't an accepted
+        # PIXI_ENVIRONMENT_NAME. Removed there; kept here, scoped to this
+        # workflow, using a real accepted env instead - catching a typo'd
+        # config path/estimator name before a many-minutes sklearn-dev
+        # clone+build is still worth it.
+        run: pixi run --frozen -e sklearn-pypi python -m sklbench --config "$CONFIG" --validate-only
 
-      - name: Run benchmark (main)
+      - name: Run comparison benchmark
+        # Reuses run.sh exactly as CONTRIBUTING.md's "Running Against
+        # scikit-learn Branches" documents for a PR-vs-base comparison:
+        # sequential setup_sklearn_ref.sh + sklbench run per env@owner:ref
+        # entry, one shared sklearn-src/ checkout re-pointed between them.
         continue-on-error: true
         run: |
-          pixi run --frozen -e sklearn-dev python -m sklbench --config "$CONFIG" --results-dir "$RESULTS_DIR" \
-            2>&1 | tee "$RUNNER_TEMP/bench-main.log"
-
-      - name: Classify PR-branch run
-        id: classify_pr
-        continue-on-error: true
-        run: pixi run --frozen -e reporting python scripts/classify_bench_session.py "$RUNNER_TEMP/bench-pr.log"
+          ./run.sh "sklearn-dev@${SKLEARN_OWNER}:${SKLEARN_HEAD_REF}" sklearn-dev@scikit-learn:main \
+            --config "$CONFIG" --results-dir "$RESULTS_DIR" \
+            2>&1 | tee "$RUNNER_TEMP/bench.log"
 
-      - name: Classify main run
-        id: classify_main
+      - name: Classify benchmark session
+        id: classify
         continue-on-error: true
-        run: pixi run --frozen -e reporting python scripts/classify_bench_session.py "$RUNNER_TEMP/bench-main.log"
+        run: pixi run --frozen -e reporting python scripts/classify_bench_session.py "$RUNNER_TEMP/bench.log"
 
       - name: Generate comparison dashboard
         # working-directory is RESULTS_DIR's parent, so its bare `results/`
@@ -248,8 +247,7 @@ jobs:
       - name: Build and post comment
         if: always()
         env:
-          CLASSIFY_PR: ${{ steps.classify_pr.outcome }}
-          CLASSIFY_MAIN: ${{ steps.classify_main.outcome }}
+          CLASSIFY_OUTCOME: ${{ steps.classify.outcome }}
           DEPLOY_OUTCOME: ${{ steps.deploy.outcome }}
           GITHUB_TOKEN: ${{ github.token }}
         run: |
@@ -260,8 +258,7 @@ jobs:
               echo "**${{ matrix.runner }}**: scikit-learn PR #$SKLEARN_PR vs \`main\`, config \`$CONFIG\`"
               echo
               echo "Dashboard: $url"
-              [ "$CLASSIFY_PR" != "success" ] && echo "- Caveat: PR-branch run session did not complete cleanly."
-              [ "$CLASSIFY_MAIN" != "success" ] && echo "- Caveat: main run session did not complete cleanly."
+              [ "$CLASSIFY_OUTCOME" != "success" ] && echo "- Caveat: one or both benchmark sessions did not complete cleanly."
               echo
               echo "Full run: $run_url"
             } > "$RUNNER_TEMP/comment.md"

run.sh

@@ -3,27 +3,6 @@
 script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
 source "$script_dir/scripts/pixi_env_check.sh"
 
-# Collects --config/-c values from a sklbench arg list into the array named
-# by $1 (argparse nargs='+' semantics: consume tokens up to the next
-# '-'-prefixed flag).
-extract_configs() {
-    local -n out_ref="$1"
-    shift
-    local i=0
-    local rest=("$@")
-    while [ "$i" -lt "${#rest[@]}" ]; do
-        if [ "${rest[$i]}" = "--config" ] || [ "${rest[$i]}" = "-c" ]; then
-            i=$((i + 1))
-            while [ "$i" -lt "${#rest[@]}" ] && [[ "${rest[$i]}" != -* ]]; do
-                out_ref+=("${rest[$i]}")
-                i=$((i + 1))
-            done
-        else
-            i=$((i + 1))
-        fi
-    done
-}
-
 usage() {
     echo "Usage: $0 env1 [env2 ...] [sklbench args...]" >&2
     echo "  Runs 'pixi run -e <env> python -m sklbench <args...>' for each" >&2
@@ -72,16 +51,6 @@ if [ "${#envs[@]}" -eq 0 ]; then
     exit 2
 fi
 
-configs=()
-extract_configs configs "${args[@]}"
-if [ "${#configs[@]}" -gt 0 ]; then
-    echo "=== pixi run --frozen -e default python -m sklbench --config ${configs[*]} --validate-only ===" >&2
-    if ! pixi run --frozen -e default python -m sklbench --config "${configs[@]}" --validate-only; then
-        echo "error: config validation failed, aborting before running any environment" >&2
-        exit 1
-    fi
-fi
-
 status=0
 for env_spec in "${envs[@]}"; do
     env="$env_spec"

scripts/classify_bench_session.py

@@ -40,14 +40,6 @@ def split_by_env(log_text: str) -> dict[str, list[str]]:
     for line in log_text.splitlines():
         header = ENV_HEADER_RE.match(line)
         if header:
-            # run.sh also prints a header in this same "=== pixi run ... ==="
-            # form for its pre-flight `--validate-only` config check, which
-            # isn't a real per-environment benchmark session and never emits
-            # progress output. Treat it as a comment rather than a session
-            # boundary so it doesn't get misclassified as a crashed env.
-            if "--validate-only" in line:
-                current_env = None
-                continue
             current_env = header.group(1)
             sessions.setdefault(current_env, [])
             continue

scripts/resolve_sklearn_pr.py

@@ -1,14 +1,15 @@
 """
-Resolve a scikit-learn PR number to its source fork's clone URL and branch,
-via the GitHub REST API directly (urllib, stdlib only) - same rationale as
+Resolve a scikit-learn PR number to its source fork's owner and branch, via
+the GitHub REST API directly (urllib, stdlib only) - same rationale as
 scripts/create_pr.py: this org restricts workflows to an allowlist of
 GitHub Actions, so there's no dedicated action for this either.
 
-Returns the fork's own clone_url verbatim rather than reconstructing one
-from its owner login, since run.sh's `env@owner:ref` shorthand assumes the
-fork repo is named "scikit-learn", which isn't guaranteed for an arbitrary
-PR's fork - callers should use scripts/setup_sklearn_ref.sh directly with
-this clone_url instead of going through run.sh's shorthand.
+Outputs just `owner`, matching run.sh's `env@owner:ref` shorthand, which
+hardcodes the fork's repo name as "scikit-learn" and reconstructs the
+remote as `https://github.com/<owner>/scikit-learn.git` itself. Since a
+PR's fork isn't guaranteed to be named "scikit-learn", this explicitly
+checks the fork's repo name and fails clearly (ok=false) rather than
+silently handing run.sh an owner whose repo it can't actually find.
 """
 
 import argparse
@@ -69,8 +70,20 @@ def main() -> int:
         )
         return 0
 
+    if fork_repo["name"] != "scikit-learn":
+        write_output(args.github_output, "ok", "false")
+        write_output(
+            args.github_output,
+            "error",
+            f"the source fork for {args.repo} PR #{args.pr_number} is named "
+            f"{fork_repo['name']!r}, not 'scikit-learn' - not supported, since "
+            "this flow runs the comparison via run.sh's env@owner:ref "
+            "shorthand, which assumes the fork repo is named scikit-learn",
+        )
+        return 0
+
     write_output(args.github_output, "ok", "true")
-    write_output(args.github_output, "clone_url", fork_repo["clone_url"])
+    write_output(args.github_output, "owner", fork_repo["owner"]["login"])
     write_output(args.github_output, "head_ref", head["ref"])
     return 0
 

tests/test_config_loading.py

@@ -22,6 +22,7 @@
     Path("configs/all_models_fast.py"): ARRAY_API_ENVS,
     Path("configs/all_models.py"): ARRAY_API_ENVS,
     Path("configs/all_models_scaling.py"): GENERAL_ENVS,
+    Path("configs/all_models_ridge_only.py"): ARRAY_API_ENVS,
 }