[feat:] Parallelize Layer Packing and Unpacking #525
Conversation
Signed-off-by: Akash Singh <akashsingh2210670@gmail.com>
Signed-off-by: Akash Singh <akashsingh2210670@gmail.com>
Signed-off-by: Akash Singh <akashsingh2210670@gmail.com>
Signed-off-by: Akash Singh <akashsingh2210670@gmail.com>
|
@gorkem Your requested changes were made |
There was a problem hiding this comment.
Thanks for the contribution! Looks great so far.
I have a couple of concerns, mostly in the packing process; we need the generated OCI manifest to match the (per-type) order in the Kitfile (fixing this is something I'm considering for #489)
pkg/lib/kitfile/local-storage.go
Outdated
| mu.Lock() | ||
| layers = append(layers, layer) | ||
| mu.Unlock() |
There was a problem hiding this comment.
I think this will cause some issues; currently the only way we have to match layer descriptors is through ordering, and this can result in a varying order for layers.
We'll need to come up with an approach that ensures the layers list matches the order of elements in the kitfile (i.e. all datasets in the same order, etc.)
There was a problem hiding this comment.
What I mean here is that in the unpack process, iterate through each of the lists of items in the kitfile, matching indices (the second dataset we see in the manifest should be the second dataset in the kitfile)
With this approach, if, for example, we first pack a large dataset, then pack a smaller dataset, the smaller dataset may be added to layers before the large one.
There was a problem hiding this comment.
What we can do to solve this is to implement a simple layer indexing logic. A simple counter layerIndex can be incremented after each item is added to maintain the order.
Each goroutine is given a layerIndex that corresponds to its position in the Kitfile, ensuring the order is preserved when layers are added to the layers slice.
There was a problem hiding this comment.
Good idea, this should work 👍
Co-authored-by: Angel Misevski <misevskia@gmail.com>
Signed-off-by: Akash Singh <akashsingh2210670@gmail.com>
|
@amisevsk Your requested changes have been made |
Signed-off-by: Akash <akashsingh2210670@gmail.com>
There was a problem hiding this comment.
The PR looks good, but you'll see there's test failures in the CI.
This is because the current filesystem.IgnorePaths uses PatternMatcher, which is not thread-safe.
We can maybe work around this by implementing an IgnorePaths.Clone() method to not share the patternmatcher between the threads, but I haven't tested it out.
To run the failing tests locally, use go test ./testing/...
|
Hey @amisevsk , I have implemented a here is the relevant log: |
|
@SkySingh04 There's a bug somewhere in the pack/unpack logic. The test in question is creating a file If you'd like to reproduce the issue locally (and are using a Unix-like OS), you can use the following bash script (Note this script depends on https://github.com/kislyuk/yq) #!/bin/bash
TEMP_DIR=./TEMP
TEMP_UNPACK_DIR=./TEMP_UNPACK
mkdir -p $TEMP_DIR
for file in $(yq -r '.ignored + .files | .[]' testing/testdata/pack-unpack/test_pack-ignores-basic.yaml); do
mkdir -p "$TEMP_DIR/$(dirname $file)"
echo $file > $TEMP_DIR/$file
done
yq -r '.kitfile' testing/testdata/pack-unpack/test_pack-ignores-basic.yaml > $TEMP_DIR/Kitfile
yq -r '.kitignore' testing/testdata/pack-unpack/test_pack-ignores-basic.yaml > $TEMP_DIR/.kitignore
./kit pack -t test:latest $TEMP_DIR
./kit unpack test:latest -d $TEMP_UNPACK_DIR(this basically reproduces what the test does locally -- you can see that TEMP_UNPACK does not contain If you'd like to step-debug the issue, you can replace dlv debug --listen=:2345 --headless=true --api-version=2 ./main.go -- pack -t test:latest TEMP |
Overview:
This PR addresses issue #254, which seeks to optimize the
packandunpackcommands by parallelizing the processing of layers. Previously, both commands handled layers sequentially, which slowed down operations significantly, especially with large files and multiple layers. This enhancement introduces multithreading for both packing and unpacking, leading to improved performance and efficiency.Parallel Layer Packing: The packing process has been modified to handle each layer (model parts, code, datasets, documentation) concurrently using goroutines. A
sync.WaitGroupis used to ensure that all layers are processed before the operation completes, and a mutex guarantees thread-safe access to shared data structures.Parallel Layer Unpacking: Similarly, unpacking is now processed in parallel. Layers are unpacked concurrently, reducing the time required for large models with multiple layers. The
runUnpackRecursivefunction has been updated to handle this using goroutines and error channels to ensure safe and efficient operation.Key Changes:
saveKitfileLayers(packing) andrunUnpackRecursive(unpacking) commands:sync.WaitGroupandsync.Mutexto ensure thread-safe operations.This PR resolves issue #254 by introducing multithreading for both
packandunpackcommands, offering a more efficient and faster solution for handling large models with multiple layers.