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

Separate stores for evaluation, building and storing the result #5025

Open
edolstra opened this issue Jul 19, 2021 · 5 comments · Fixed by #5048
Open

Separate stores for evaluation, building and storing the result #5025

edolstra opened this issue Jul 19, 2021 · 5 comments · Fixed by #5048

Comments

@edolstra
Copy link
Member

For several use cases it would be useful to distinguish between the stores used to store .drv files, to build derivations, and to store the resulting closure.

For example:

  • nix path-info --store https://cache.nixos.org nixpkgs#hello doesn't work because the evaluation of nixpkgs#hello tries to create .drv files in cache.nixos.org.
  • For NixOps, we want to do evaluation on the local machine, do builds on some arbitrary machines (could be the local machine, the destination machine, or some other builder) and store the result in the destination machine.
  • If /nix/store is a high-latency remote store (as with flox), then copying lots of little .drv files to the remote is slow. So it's better to evaluate locally and only copy .drv closures to the remote as needed and in a single round-trip.
@edolstra edolstra self-assigned this Jul 19, 2021
edolstra added a commit to tweag/nix that referenced this issue Jul 19, 2021
With this, we don't have to copy the entire .drv closure to the
destination store ahead of time (or at all). Instead, buildPaths()
reads .drv files from the eval store and copies inputSrcs to the
destination store if it needs to build a derivation.

Issue NixOS#5025.
edolstra added a commit to tweag/nix that referenced this issue Jul 22, 2021
With this, we don't have to copy the entire .drv closure to the
destination store ahead of time (or at all). Instead, buildPaths()
reads .drv files from the eval store and copies inputSrcs to the
destination store if it needs to build a derivation.

Issue NixOS#5025.
@domenkozar
Copy link
Member

Another use case is #3946

@edolstra
Copy link
Member Author

So the fully general solution would be that buildPaths is no longer a Store method, but a static function that takes a stores configuration like this:

struct Stores {
  // The store where the build result should appear.
  ref<Store> destStore;

  // The store that contains the .drv file to be built.
  ref<Store> evalStore;

  // The stores that can build stuff. (This replaces the `build-remote` hook.)
  std::vector<ref<Store>> buildStores;
}

@rickynils
Copy link
Member

If something like --build-store is ever implemented, couldn't that entirely replace the concept of remote builders too? It would be great to not have the two closely related but separate concepts of "remote builders" and "remote stores". So instead of having --builders you could maybe just add multiple --build-store arguments?

And while I'm here wishing for stuff, I could just as well throw in something like --fetch-store to replace substituters too. I think it would be awesome to "trickle up" the internal store abstraction like this, since it makes the concepts simpler for the user, and at the same time more flexible which opens up for new interesting usage scenarios in distributing builds, caches and deployments.

@Ericson2314
Copy link
Member

@rickynils

When @edolstra writes

(This replaces the build-remote hook.)

This means it is doing exactly what you want!

Substitution and remote building in fact does use the store abstraction already. Hooray! The fact that remote building first shells to the build-remote exe which then talks to the actual remote builder in the usual Store interface way is just a historical artifact of the old perl days. This is what the above would do away with.

(In fact, we could perhaps have a "store with hooked building" adapter for backwards compat if someone really wants it!)

@stale stale bot added the stale label Oct 30, 2022
@stale stale bot removed the stale label Dec 5, 2022
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Apr 13, 2023
More progress on issue NixOS#5729

The method trivially generalizes to be store-implementation-agnostic, in
fact.

However, we force it to continue to be unimplemented with `RemoteStore`
and `LegacySSHStore` because the implementation we'd get via the
generalization is probably not the one users expect. This keeps our
hands untied to do it right going forward.

For more about the tension between the scheduler logic being
store-type-agnostic and remote stores doing their own scheduling, see
issues NixOS#5025 and NixOS#5056.
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Apr 14, 2023
More progress on issue NixOS#5729

The method trivially generalizes to be store-implementation-agnostic, in
fact.

However, we force it to continue to be unimplemented with `RemoteStore`
and `LegacySSHStore` because the implementation we'd get via the
generalization is probably not the one users expect. This keeps our
hands untied to do it right going forward.

For more about the tension between the scheduler logic being
store-type-agnostic and remote stores doing their own scheduling, see
issues NixOS#5025 and NixOS#5056.
@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/peer-to-peer-binary-cache-rfc-working-group-poll/29568/43

Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Dec 11, 2023
I don't love the way this code looks. There are two larger problems:

- eval, build/scratch, destination stores (NixOS#5025) should have different
  types to reflect the fact that they are used for different purposes
  and those purposes correspond to different operations. It should be
  impossible to "use the wrong store" in my cases.

- Since drvs can end up in both the eval and build/scratch store, we
  should have some sort of union/layered store (not on the file sytem
  level, just conceptual level) that allows accessing both. This would
  get rid of the ugly "check both" boilerplate in this PR.

Still, it might be better to land this now / soon after minimal cleanup,
so we have a concrete idea of what problem better abstractions are
supposed to solve.
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Dec 11, 2023
I don't love the way this code looks. There are two larger problems:

- eval, build/scratch, destination stores (NixOS#5025) should have different
  types to reflect the fact that they are used for different purposes
  and those purposes correspond to different operations. It should be
  impossible to "use the wrong store" in my cases.

- Since drvs can end up in both the eval and build/scratch store, we
  should have some sort of union/layered store (not on the file sytem
  level, just conceptual level) that allows accessing both. This would
  get rid of the ugly "check both" boilerplate in this PR.

Still, it might be better to land this now / soon after minimal cleanup,
so we have a concrete idea of what problem better abstractions are
supposed to solve.
@edolstra edolstra removed their assignment Apr 26, 2024
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 a pull request may close this issue.

6 participants