Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Pass, then timeout" should not be considered a spurious regression #739

Open
workingjubilee opened this issue Sep 7, 2024 · 4 comments

Comments

@workingjubilee
Copy link
Member

Issue is visible in this crater report:

Crater was being used to diagnose usage of the new trait solver, which has hang issues in some cases.

This caused rust-lang/rust#130056 to be opened after landing rust-lang/rust#121848 even though we did notice essentially all the nalgebra dependents failed this way:
nalgebra-glm, nalgebra-macros, nalgebra-mvn, nalgebra-py, nalgebra-spacetime, nalgebra_latex, nalgebra_linsys all failing with "build timed out" after the first round of tests passed

@workingjubilee
Copy link
Member Author

other possible solutions might be to find some way to introduce retrying or extended second-pass build times that make this work.

@pietroalbini
Copy link
Member

The reason why timeouts regressions are marked as spurious is because the amount of time it takes to build a crate varies wildly between builds, depending on whether the dependencies were cached or not in the target directory. If we didn't mark those as spurious every crater run would contain spurious regressions in the regressed crates list.

@Skgland
Copy link
Contributor

Skgland commented Sep 16, 2024

other possible solutions might be to find some way to introduce retrying or extended second-pass build times that make this work.

I don't think this is what you meant but since afc6496 spurious regressed crates are included in the retry-regressed-list.txt, though it looks like the crater run mentioned was from before that change.

Maybe it would help if the crater bot would mention an unusually high amount of spuriously regressed crates in addition to regressed and fixed crates, or spuriously regressed could be a sub-category under regressed?

i.e.

🎉 Experiment pr-121848-3 is completed! 📊 16 regressed and 3 fixed (501375 total) 📰 Open the full report.

🔥 Unusual high amount of spurious regressed crates 2489 (instead of ~500-600)

⚠️ If you notice any spurious failure please add them to the blacklist! ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@Skgland
Copy link
Contributor

Skgland commented Sep 16, 2024

The reason why timeouts regressions are marked as spurious is because the amount of time it takes to build a crate varies wildly between builds, depending on whether the dependencies were cached or not in the target directory. If we didn't mark those as spurious every crater run would contain spurious regressions in the regressed crates list.

This sounds like something along the lines of rust-lang/cargo#2644 might help as it would allow to make a two phase build, first deps only and then the crate itself.

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

No branches or pull requests

3 participants