derivationStrict #554
derivationStrict #554
Conversation
jwiegley
requested review from
ryantrinkle and
kmicklas
and removed request for
ryantrinkle
|
This looks like great work, @kmicklas, I wonder if @shlevy and @ryantrinkle might have some time to take a look. If not, we can merge later next week and fix whatever is left to be fixed. |
|
I think you meant to tag @layus 😄 |
|
Oh, right, thank you @kmicklas ! |
src/Nix/Effects.hs
Outdated
| Right pathName -> do | ||
| -- todo: pass the hash explicitly | ||
| -- TODO: Handle the filter | ||
| res <- Store.runStore $ Store.addToStore pathName path recursive (Proxy :: Proxy Store.SHA256) (const False) repair |
There was a problem hiding this comment.
This also changed type to omit proxy so you can just do Store.addToStore @'Store.SHA256 pathName ..
|
haskell-nix/hnix-store#59 now also contains parser for There's also support for building derivations now if you want to give it a try. |
|
Build failure due to the dependencies on newer versions of the hnix-store... We should realy integrate everything in one single repo. |
|
Now buildable from Seems to work but eats all my ram 😄 Be careful as this does access real system store via daemon now (tests do as well). |
|
This PR is very inefficient. We need a way to cache hashDerivationModulo results. Otherwise we are doomed with an exponential computing time. It means that in practice computing It may also be related to the memory consumption issue a we load several times all the derivations from memory, parse everything and compute hashes. enforcing some strictness may also help. From my point of view, this state should reside in the store itself. We should considr moving some of the code to hnix-store. |
|
@layus Above message may decerve to be an idea, so it is good to preseve it in Report. |
Anton-Latukha
added
the
challenging
layus
force-pushed
the
derivationStrict
branch
2 times, most recently
from
33815b6
to
d38db1c
Compare
|
Hi, I think this has reached a mergeable state. 🎉 🎉 🎉 The main issue with exponential memory usage (in the size of the closure) is now addressed by adding a cache for hashDerivationModulo results, next to the cache of parsed nix files (see last commit). The bulk of the implementation resides in the second to last commit. Lots of code, but standalone for the most part. There are no tests, but nixpkgs.hello gives the same hash with nix and hnix. Achieving this was quite a feat 🎉 and requires the fixes to hnix-store that are idling in haskell-nix/hnix-store#64. Also, tracking down the issue with the substring builtin was quite a pain. Anyway, I think this is quite an achievement as it bridges the gap between hnix an nix. We can now correctly evaluate derivations ! 🥇 /cc @jwiegley, @shlevy, @ryantrinkle Would you mind having a look at this PR ? |
|
Pending questions to be resolved later on, in other PRs:
|
|
It's a fair bit going on, but I like it. What do you think @ryantrinkle? |
These things do further my suspicion that it's time to move to a monorepo so it's easier to experiment without how to integrate the various libraries while keeping things modular. |
|
I'm generally a fan of monorepos. |
|
Short update about performance: $ NIX_DATA_DIR=$PWD/hnix/data command time ./dist-newstyle/build/x86_64-linux/ghc-8.10.1/hnix-0.10.1/x/hnix/build/hnix/hnix --eval -E '"${(import { }).hello}"'
"/nix/store/w9yy7v61ipb5rx6i35zq1mvc2iqfmps1-hello-2.10"
22.21user 0.67system 0:22.95elapsed 99%CPU (0avgtext+0avgdata 1615464maxresident)k
0inputs+0outputs (0major+400263minor)pagefaults 0swaps
$ command time nix-instantiate --eval -E '"${(import {}).hello}"' (nix)
"/nix/store/w9yy7v61ipb5rx6i35zq1mvc2iqfmps1-hello-2.10"
0.22user 0.03system 0:00.25elapsed 100%CPU (0avgtext+0avgdata 111252maxresident)k
0inputs+0outputs (0major+25171minor)pagefaults 0swaps
22 each time :-). And yet 100 times slower. Good news is that we have some room for improvement 😂 |
|
@layus Parsing and evaluating are each still very slow. |
layus
force-pushed
the
derivationStrict
branch
from
90f8e79
to
b1f6ef2
Compare
|
@jwiegley Any known culprit for that ? |
M default.nix M hnix.cabal M src/Nix/Effects/Derivation.hs default.nix: fx overlay call
Anton-Latukha
force-pushed
the
derivationStrict
branch
from
0b9c888
to
7785917
Compare
|
Reshuffled the history, cleaned it up, and minimized transitions as much as I could. Figured-out and fixed a couple of things in config changes. Now, it looks strangely familiar: On macOS compilations fails due to non-existent IPC calls
In Nix builds fail because they use network access in tests
On GHC 8.10 GitHub decided no matter what PR is - to take a New Year break after 10 minutes of work
10 minutes... And what really rings true is the Cabal build
What would you do - every build is familiar in its own way. But only the last one is, as always, being true. If GitHub was not lazy today - the GHC 8.10 build would've told us the exact same story (since it tells it locally). It compiles, but fails on that little test. Test group defined as: genEvalCompareTests = do
td <- D.listDirectory testDir
let unmaskedFiles = filter ((==".nix") . takeExtension) td
let files = unmaskedFiles \\ maskedFiles
pure $ testGroup "Eval comparison tests" $ map (mkTestCase testDir) files
where
mkTestCase td f = testCase f $ assertEvalFileMatchesNix (td </> f)What we essentially have is a 1 test to fix. And that is a test introduced here: hnix/tests/eval-compare/nixpkgs.hello.nix Lines 1 to 22 in ac7450a I would think and also wait on thoughts on what is the thing with this test. |
|
This happens because: Dolstra recently moves/reduces the helper |
|
It is somewhere between functions |
|
Why and how it looks into it. |
|
That's a lot of small issues bundled together. Let's try them one at a time:
For this PR, we need to bundle the files correctly so that As for the
This can wait for a future PR. It is not part of the nix interface. It's implementation specific. We could also decide to bundle it in a single value just like we do for In nix, the build fails because we need network access. That was already the case before. Worth to be fixed, but not necessarily here. But they also fail because the need a working nix daemon. In hnix-store @sorki make a great work at both i) hiding these tests behind a flag and ii) wrapping everything in namespaces to provide a sandboxed nix daemon. We could import either. And possibly both. So we would have flags tho disable tests that require networking, and provide a sandboxed nix daemon for tests. It is a bit sad that the current implementation relies on the remote version of the hnix-store. a read-only version would fit the tests better. But it does not exist yet. Among other things because it would require some internal state that needs some architectural decisions about where/how to keep it. So for now we need the real nix daemon somewhere. I guess that already some stuff to grind :-) |
|
As for The link references the same nixpkgs commit as the one we fetch with fetchTarball. The solution is a proper NIX_DATA_DIR being set because that gets used as the |
|
Or rather a preperly filled |
|
|
|
Indeed. |
|
Great read. I never thought that it might be the Nixpkgs call of the file, automatically assumed it is or Haskell code or I wanted to propose simple separation of consciences: cherry-pick the test into the separate PR and merge current. And deal with fixing the envocation in the test separately. |
If they're needed, and it's not going to leave this PR in purgatory for another seven months, I think this is the most sensible option |
Ok, let's go with that. HNix does not yet support chroot stores, but runs the tests with it. It worked while we were only using the real nix for real store operations. Now it breaks. |
This reverts commit ac7450a for the test it contains requires support for chroot stores in HNix, and it is not yet implemented. See comments starting at haskell-nix#554 (comment) for details.
|
Hooray, we might be merged before the new year! Happy holidays, friends. |
|
@Anton-Latukha I think these two new changes are enough, even if not entirely satisfactory. It would be fun to merge this in late 2020. But getting the first merge of 2021 would be an achievement too. I would appreciate if you could merge it yourself after running the checks you consider necessary :-D. Cheers ! And happy new year :-) |
|
Yep, I'd already cleaned-up and merged it as #554 (comment) when I wrote it, but I actually wrote that comment from the |
Anton-Latukha
force-pushed
the
derivationStrict
branch
from
5bbcea1
to
43be29e
Compare
|
I formed that test into a separate PR and dropped its revert here. Cabal builds already worked - which is a minimal and enough requirement for the PR merges for me. There is no sense in breaking the I am not an authority, but I recommend to think about our pathways - all our work and infrastructure, Nix & Nixpkgs Haskell, Stack also - are still, in the end, depend and use Hackage and its Cabal descriptions, and so - our Hackage community releases which are Cabal builds and descriptions. That is why it is the main thing for me - to keep Cabal builds working. On top of it - that keeps the history and the code open for any person in the community, it is a minimal requirement and enough to do Haskell work. If I ever merge my or other PRs that had the Cabal builds broken but I merged them - you can beat my hands with a bamboo stick. I wish one day to be as productive as you, @layus |
|
Thank you all! |
|
That is not all, do not put a cart before a horse. We have a big API. And a big API change. |
Implement derivationStrict primOp
Closes #364