Skip to content

Conversation

@AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Apr 29, 2025

Summary

This PR sets up a CI job that uses the py-fuzzer script to check whether a PR introduces new panics to red-knot.

The py-fuzzer script still finds lots of panics in red-knot generally (see astral-sh/ty#228 for a tracking issue), so running it with the default arguments in CI would lead to a lot of red CI jobs. However, the py-fuzzer script also has a --only-new-bugs CLI flag! If specified, it allows you to compare two binaries and check whether any new panics have been introduced by the PR. If any have been introduced, the script fails; if there are no new panics relative to the baseline binary, however, the script exits with code 0.

The CI job here ends up looking like a cross between the fuzz-parser CI job in ci.yaml (which already runs the py-fuzzer script in CI for us, but which runs it with the ruff binary to fuzz the parser rather than red-knot) and the ruff-ecosystem CI job in ci.yaml (which, in a similar way to this new CI job, downloads two binaries and compares them to see if there's any differences).

The CI job is currently failing because there's no baseline red-knot binary for the job to download (this PR adds the CI job that will start uploading the red-knot binary for future CI jobs to download). I'm not totally sure how to address this -- I suppose I could split it up into two PRs, one that adds the job uploading the binary and then a second one that adds the job to actually use the binary? I'll wait for review on the general idea here before going ahead with that, though.

Test Plan

I did the following:

  • Built the binary on main with --target-directory=target/main

  • Switched to a branch that had more panics and ran cargo build -p red_knot

  • Ran the following command from my terminal and observed that the script only reported new panics that did not exist on main:

    uvx \
        --python="3.12" \
        --from=./python/py-fuzzer \
        fuzz \
        --test-executable="./target/debug/red_knot" \
        --baseline-executable="./target/main/debug/red_knot" \
        --only-new-bugs \
        --bin=red_knot \
        0-500
    

@AlexWaygood AlexWaygood added ci Related to internal CI tooling ty Multi-file analysis & type inference labels Apr 29, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Apr 29, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@AlexWaygood AlexWaygood marked this pull request as ready for review April 29, 2025 18:43
@dhruvmanila
Copy link
Member

The CI job is currently failing because there's no baseline red-knot binary for the job to download (this PR adds the CI job that will start uploading the red-knot binary for future CI jobs to download). I'm not totally sure how to address this -- I suppose I could split it up into two PRs, one that adds the job uploading the binary and then a second one that adds the job to actually use the binary? I'll wait for review on the general idea here before going ahead with that, though.

Or, you could create a dummy PR with either an empty commit or a random change (if it's required) to trigger CI which is stacked on top of this PR.

@AlexWaygood AlexWaygood force-pushed the alex/panic-regrtest branch from f0fa072 to d9c6708 Compare April 29, 2025 20:04
@AlexWaygood AlexWaygood changed the base branch from main to alex/upload-redknot-binary April 29, 2025 20:04
@AlexWaygood AlexWaygood reopened this Apr 29, 2025
@AlexWaygood AlexWaygood marked this pull request as draft April 29, 2025 20:15
@AlexWaygood AlexWaygood force-pushed the alex/panic-regrtest branch from 8973c1e to 25666ad Compare April 29, 2025 20:39
@AlexWaygood AlexWaygood force-pushed the alex/panic-regrtest branch from 25666ad to 426ca70 Compare April 29, 2025 20:44
@AlexWaygood AlexWaygood marked this pull request as ready for review April 29, 2025 20:50
@AlexWaygood
Copy link
Member Author

Or, you could create a dummy PR with either an empty commit or a random change (if it's required) to trigger CI which is stacked on top of this PR.

I split this PR into two -- this change is now stacked on top of #17720 (so that there is a baseline binary to download), and CI is passing on the new job 🎉 https://github.com/astral-sh/ruff/actions/runs/14740981444/job/41378854533?pr=17719#step:6:539

Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Base automatically changed from alex/upload-redknot-binary to main April 29, 2025 21:15
@AlexWaygood AlexWaygood reopened this Apr 29, 2025
@AlexWaygood AlexWaygood enabled auto-merge (squash) April 29, 2025 21:15
Comment on lines +645 to +646
# Only runs on pull requests, since that is the only we way we can find the base version for comparison.
if: ${{ !contains(github.event.pull_request.labels.*.name, 'no-test') && github.event_name == 'pull_request' && needs.determine_changes.outputs.red_knot == 'true' }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, and it's most useful to run it on a PR. We also don't run the py-fuzzer on parser on main.

@AlexWaygood AlexWaygood merged commit 549ab74 into main Apr 29, 2025
29 of 30 checks passed
@AlexWaygood AlexWaygood deleted the alex/panic-regrtest branch April 29, 2025 21:19
@codspeed-hq
Copy link

codspeed-hq bot commented Apr 29, 2025

CodSpeed Performance Report

Merging #17719 will improve performances by 4.1%

Comparing alex/panic-regrtest (8c563cc) with main (8c68d30)

Summary

⚡ 1 improvements
✅ 32 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
parser[numpy/ctypeslib.py] 924.5 µs 888 µs +4.1%

@AlexWaygood
Copy link
Member Author

Merging #17719 will improve performances by 4.1%

hmmmmm

dcreager added a commit that referenced this pull request Apr 30, 2025
* main:
  [red-knot] Use 'full' salsa backtrace output that includes durability and revisions (#17735)
  [red-knot] Initial support for protocol types (#17682)
  [red-knot] Computing a type ordering for two non-normalized types is meaningless (#17734)
  [red-knot] Include salsa backtrace in check and mdtest panic messages (#17732)
  [red-knot] Fix control flow for `assert` statements (#17702)
  [red-knot] Fix recording of negative visibility constraints (#17731)
  [red-knot] Update salsa (#17730)
  [red-knot] Support overloads for callable equivalence (#17698)
  [red-knot] Run py-fuzzer in CI to check for new panics (#17719)
  Upload red-knot binaries in CI on completion of linux tests (#17720)
  [`flake8-use-pathlib`] Fix `PTH123` false positive when `open` is passed a file descriptor from a function call (#17705)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci Related to internal CI tooling ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants