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

Wait for GHA eval results instead of falling back to local evaluation #451

Merged
merged 3 commits into from
Feb 3, 2025

Conversation

GaetanLepage
Copy link
Collaborator

@GaetanLepage GaetanLepage commented Jan 5, 2025

The default evaluation mode (github) attempts at fetching the evaluation results from the GitHub actions result.
Those will only be available a few minutes after the PR has been opened as they are not trivial to compute.

Currently, if the results are not available right away, nixpkgs-review falls back to local evaluation.
This PR changes this behavior. Now it will keep re-trying until the results are available.

Note: It is still possible to force local evaluation by using --eval local.

Motivation: Local evaluation is very resource-intensive (especially RAM). It should be an opt-in behavior.
This new approach is more predictable.

Questions:

  • Does this work (at all) ?
  • Is 10s a good value for the timeout ? -> Seems good, yes.
  • Should we add a maximum amount of retries ?
    -> I have set a maximum waiting time of 10min:

image

  • Handle Ctrl-C correctly (i.e. properly delete the worktree on exit)
    -> There is no worktree at this point. Just quit gracefully.

@GaetanLepage
Copy link
Collaborator Author

Maybe we should silence the warning:

image

@GaetanLepage
Copy link
Collaborator Author

This implementation is quite naive. We could eventually wait within the function and thus not restarting the fetching process from scratch each time.

nixpkgs_review/review.py Outdated Show resolved Hide resolved
nixpkgs_review/review.py Outdated Show resolved Hide resolved
@GaetanLepage
Copy link
Collaborator Author

Maybe we should silence the warning:

I removed them all. Do you have a better idea ?

@GaetanLepage GaetanLepage force-pushed the wait branch 4 times, most recently from 471bd4f to 0247365 Compare January 5, 2025 14:08
nixpkgs_review/review.py Outdated Show resolved Hide resolved
@GaetanLepage
Copy link
Collaborator Author

I confirm that printing is fixed:
image

@GaetanLepage
Copy link
Collaborator Author

Should we add a tip:
"Use --eval local if you prefer running the evaluation locally."
?

@Mic92 Mic92 merged commit 61784f4 into Mic92:master Feb 3, 2025
3 checks passed
@GaetanLepage GaetanLepage deleted the wait branch February 3, 2025 08:31
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.

2 participants