-
Notifications
You must be signed in to change notification settings - Fork 67
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
local evaluation: use nixpkgs ci.eval parallel implementation #440
base: master
Are you sure you want to change the base?
Conversation
f1062d7
to
87d78c6
Compare
3a148ac
to
42dd4c6
Compare
For now, I initialize the parameters following the recommendations of the
I conducted early and naive benchmarks on the following system:
I ran the evaluation for the four supported systems (
So unfortunately, it appears that this method is slower than our current one... Maybe tuning the hyper-parameters can give better results, but as we are maxing out the CPU, I don't expect a lot of gains. Note: using cc @infinisil, maybe you will have more insights. |
You should be able to get the same speed as the legacy |
How evenly is the max memory usage spread across different chunks? |
I don't really know. The overall RAM usage fluctuates and peaks very briefly at ~39GB (using |
It feels like we need to solve this in nix. i.e. track RAM usage there and restart the interpreter after a threshold is breached. |
So, what should we do about this ? |
Maybe the new method could be a different branch for the time being. Some people might have machines with enough resources, where it actually speeds up things. I am wondering if we can leverage |
Did you try my suggestion of having a single huge chunk, so that it should effectively be the same single-threaded performance as before? |
42dd4c6
to
fef5490
Compare
fef5490
to
308620a
Compare
Sorry, I forgot to follow up. Here is the benchmark I made:
Well...it OOMed :'(
And it was not done with the first (pre-change) evaluation... |
I can only imagine that nixpkgs-review doesn't do the same eval then |
This PR aims at improving local evaluation.
Currently, it uses a (single-threaded)
nix-env
call per system to evaluate.This new patch leverages the recent
ci.eval
parallel evaluation implementation innixpkgs
itself.Current limitations:
--allow
as we used to (maybe it is possible...)--max-jobs
,--cores
andchunkSize
parameters ofci.full
We need to expose new options for those and provide sensible (automatic ?) default values.
stdout
is now limited to listing all "rebuilds" (i.e. added + changed packages). Before, it was more detailed (removed, changed and added I think). This information is not available in the output of theeval.compare
derivation. If needed, we could change the upstream implem...TODO
--allow
user preferenceseval.full
parameters (--max-jobs
,--cores
and--arg chunkSize
)cc @Mic92 @zowoq @khaneliman