Refactor Store Api into client side and driver side #935

allada · 2024-05-27T23:38:22Z

Refactors the Store api into the driver (backend) implementation and a client Store/StoreLike api. Store & StoreLike have their sizes known at compile-time. This enables us to add templates to the client-side to make it easier to work with, for example, we no longer need to Pin the store and we'll be able to add things like digest: impl Into<DigestInfo> to make the callers life much easier.

towards: #934

This change is

allada

+@adam-singer

fyi: @zbirenbaum

Note: This is for an early review. I still need to figure out a better way to do inner_store, as it turns out this method is not a good way.

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale, windows-2022 / stable, and 1 discussions need to be resolved (waiting on @adam-singer)

a discussion (no related file):
note: This PR is waiting for this patch to land:
#932

allada

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Vercel, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, vale (waiting on @adam-singer)

a discussion (no related file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

note: This PR is waiting for this patch to land:
#932

Done.

allada

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, and 1 discussions need to be resolved (waiting on @adam-singer)

deployment-examples/docker-compose/Dockerfile line 32 at r4 (raw file):

        apt-get install --no-install-recommends -y \
            npm=8.5.1~ds-1 \
            git=1:2.34.1-1ubuntu1.11 \

there was a security vulnerability in previous git versions, so ubuntu dropped the old versions.

adam-singer

Look great, I really like this approach

Reviewed 19 of 47 files at r2, 1 of 21 files at r3, 7 of 25 files at r4, 21 of 21 files at r5, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and 6 discussions need to be resolved

nativelink-store/src/ac_utils.rs line 40 at r5 (raw file):

/// Attempts to fetch the digest contents from a store into the associated proto.
pub async fn get_and_decode_digest<'a, T: Message + Default>(

Why is a declaration needed here of an implementation reference vs dynamic trait dispatch? Would it be possible to remove and hope the "monomorphization" of rust compiler to figure it out?

nativelink-store/src/default_store_factory.rs line 52 at r5 (raw file):

) -> Pin<FutureMaybeStore<'a>> {
    Box::pin(async move {
        let store: Arc<dyn StoreDriver> = match backend {

Wondering out loud here.. what other "tricks" could be done to reduce this to a impl vs dyn for instance creation? I assume the StoreDriver traits would need to change. From a code/understanding pov, going towards templating the code looks much better vs having to pin/dyn everything. Wondering if some mix of using macros to define the factory methods and as templates for defining the StoreDriver.

nativelink-store/tests/completeness_checking_store_test.rs line 81 at r5 (raw file):

        let tree_digest = serialize_and_upload_message(
            &tree,
            cas_store.as_pin(),

Does as_pin() need to be called on each pass? Guessing that is to please the borrowing of a new Pin vs reusing an existing assignment?

nativelink-worker/src/running_actions_manager.rs line 367 at r5 (raw file):

fn upload_directory<'a, P: AsRef<Path> + Debug + Send + Sync + Clone + 'a>(
    cas_store: Pin<&'a impl StoreLike>,

Is lifetime still needed here?

nativelink-worker/tests/running_actions_manager_test.rs line 217 at r5 (raw file):

            download_to_directory(
                cas_store.as_ref(),
                Pin::new(fast_store.as_ref()),

don't the as_pin class of functions hang off of this? Same for other download_to_directory calls below

allada

Reviewable status: 0 of 1 LGTMs obtained, and 4 discussions need to be resolved (waiting on @adam-singer)

nativelink-store/src/ac_utils.rs line 40 at r5 (raw file):