Fix pr-comparison flow: reuse run.sh, drop broken validate-only preflight - #32
Merged
Conversation
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>
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
1 participant
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.
Found live while testing #31 (benchmarking scikit-learn PR #34793): the "Validate config" step used
-e reporting(andrun.sh's own recently-added preflight used-e default), butimplementations_for_pixi_env()rejects anyPIXI_ENVIRONMENT_NAMEoutside its real-env allowlist - so this preflight was broken for any config calling it (all_models.pyand anything derived from it), unconditionally, regardless of which real environment would go on to run.run.sh: drop the recently-added--validate-onlypreflight block and its now-unusedextract_configshelper (it's broken the same way for its own callers, e.g.run-benchmarks.yml's default configconfigs/all_models.py).scripts/classify_bench_session.py: drop the now-dead special-case for run.sh's removed--validate-onlyheader line..github/workflows/pr-comparison.yml: reuserun.shdirectly (one invocation with twoenv@owner:refentries) instead of duplicatingsetup_sklearn_ref.sh+sklbenchcalls per side - matches CONTRIBUTING.md's documented PR-vs-base comparison pattern exactly, and collapses two log files/classify calls into one. Keeps a config preflight, but scoped to this workflow with a real, always-installable env (sklearn-pypi) instead of a broken one.scripts/resolve_sklearn_pr.py: output the fork'sowner(matchingrun.sh'senv@owner:refshorthand) instead of a full clone URL, and validate the fork's repo is actually namedscikit-learn(what that shorthand assumes) rather than silently mishandling a differently-named fork.Note: this bundles fixes discovered while testing #31, but #31 itself can't pick these up until this merges -
gate_and_resolvedeliberately checks outbase.sha(trusted-code-only), so a PR's own changes to these scripts never take effect on that same PR.