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

Feat: add basic parallel support for listing packages #422

Merged
merged 3 commits into from
Oct 9, 2024

Conversation

GaetanLepage
Copy link
Contributor

When ofborg's evaluation results are not available or when the user specifies --eval local, local evaluation is used to list packages affected by a PR.
This process is quite time-consuming and could be done in parallel when evaluating for multiple systems.

Ultimately, the performance of nix-eval-jobs could be leveraged to perform this task.
Unfortunately, as it is currently, nix-eval-jobs cannot be used as a drop-in replacement in this code base.

As an intermediary situation, I suggest to ship a simple parallel solution.
This approach can be very memory-intensive.
Running it with 4 parallel processes (one for each system) consumes approximately 55GB of RAM.
Nonetheless, it unsurprisingly reduces the time spent on evaluation almost linearly.

I did a naive test running nixpkgs-review --systems all (4 systems) on a PR I had already reviewed (all build artifacts were cached on my system).
The total wall-time was 5:17 with 4 parallel processes and 16:51 with a single process.

An important decision to make is about how to parametrize the number of threads.
#419 introduced --num-procs-eval which is currently used to control how many nix eval commands were ran in parallel.
Although it could be convenient to use this single flag for both processes, the implication in terms of memory usage are significantly different.

How would you see the configuration for this new parallel feature ?

  • A new flag ? If so, do we keep the current name ?
  • Having two flags ? (Renaming the old one in the meantime ?)

@Mic92
Copy link
Owner

Mic92 commented Oct 6, 2024

If you eval the whole of nixpkgs for different systems than this approach is probably even faster because it overlaps less duplicate computation compared to nix-eval-jobs.
Each system is independent from the others.

@GaetanLepage
Copy link
Contributor Author

If you eval the whole of nixpkgs for different systems than this approach is probably even faster because it overlaps less duplicate computation compared to nix-eval-jobs.
Each system is independent from the others.

Yes, I have been using it lately and it works fine.

However, I am still wondering what we should do w.r.t the user interface.
Should we have two flags to separate the nix-env parallelization and the nix eval one ?

@Mic92
Copy link
Owner

Mic92 commented Oct 9, 2024

@mergify rebase

Copy link
Contributor

mergify bot commented Oct 9, 2024

rebase

✅ Branch has been successfully rebased

nixpkgs_review/review.py Outdated Show resolved Hide resolved
@GaetanLepage GaetanLepage changed the title [WIP] Feat: add basic parallel support for listing packages Feat: add basic parallel support for listing packages Oct 9, 2024
@GaetanLepage GaetanLepage force-pushed the parallel-eval branch 5 times, most recently from 3a636a9 to 124f611 Compare October 9, 2024 07:17
@Mic92
Copy link
Owner

Mic92 commented Oct 9, 2024

@mergify queue

Copy link
Contributor

mergify bot commented Oct 9, 2024

queue

🛑 Command queue cancelled because of a new queue command with different arguments

@GaetanLepage
Copy link
Contributor Author

Is it stuck ?

@Mic92
Copy link
Owner

Mic92 commented Oct 9, 2024

Yeah. Mergify is buggy lately.

@Mic92
Copy link
Owner

Mic92 commented Oct 9, 2024

@mergify queue

@Mic92 Mic92 merged commit 4049c27 into Mic92:master Oct 9, 2024
3 checks passed
Copy link
Contributor

mergify bot commented Oct 9, 2024

queue

🟠 Waiting for conditions to match

  • -closed [📌 queue requirement]
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed
  • any of: [🔀 queue conditions]
    • all of [📌 queue conditions of queue default]

@GaetanLepage
Copy link
Contributor Author

Yeah. Mergify is buggy lately.

Ok, thanks for the careful review :)

@GaetanLepage GaetanLepage deleted the parallel-eval branch October 9, 2024 19:42
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