Remote storage pipeline #899
Conversation
|
👋 Hi tstamler! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
tstamler
force-pushed
the
remote_storage_pipeline
branch
from
1111e18 to
c3f3cdc
Compare
tstamler
requested review from
a team,
mkhazraee,
ovidiusm and
roiedanino
as code owners
Signed-off-by: Timothy Stamler <[email protected]>
tstamler
force-pushed
the
remote_storage_pipeline
branch
from
b2c6c33 to
805df1e
Compare
Signed-off-by: Timothy Stamler <[email protected]>
tstamler
force-pushed
the
remote_storage_pipeline
branch
from
cb625ac to
93ab2a6
Compare
There was a problem hiding this comment.
Maybe it would be better to have some background for this image or change the color of the black titles, as they become almost invisible in dark mode
There was a problem hiding this comment.
+1 to @roiedanino comment
| end_time = time.time() | ||
|
|
||
| elapsed = end_time - start_time |
There was a problem hiding this comment.
| end_time = time.time() | |
| elapsed = end_time - start_time | |
| elapsed = time.time() - start_time |
| end_time = time.time() | ||
|
|
||
| elapsed = end_time - start_time |
There was a problem hiding this comment.
| end_time = time.time() | |
| elapsed = end_time - start_time | |
| elapsed = time.time() - start_time |
| mem = "DRAM" | ||
|
|
||
| if args.role == "client": | ||
| mem = "VRAM" |
There was a problem hiding this comment.
Don't we have some constants for "DRAM", "VRAM", "GDS_MT" and so on?
| nixl_mem_reg_list = [] | ||
| nixl_file_reg_list = [] | ||
|
|
||
| if mem == "VRAM": |
There was a problem hiding this comment.
Maybe it would be better to use constants for memory/storage types
|
|
||
| elapsed = end_time - start_time | ||
|
|
||
| logger.info(f"Time for {iterations} READ iterations: {elapsed} seconds") |
There was a problem hiding this comment.
| logger.info(f"Time for {iterations} READ iterations: {elapsed} seconds") | |
| logger.info("Time for %s READ iterations: %s seconds", iterations, elapsed) |
We shouldn't use f-strings in loggers, it's not optimized, pylint would warn about it
| s += 1 | ||
| continue | ||
|
|
||
| if s == iterations: |
There was a problem hiding this comment.
How can this flow happen with the s < iterations while condition?
| if s == 0: | ||
| futures.append( | ||
| executor.submit( | ||
| execute_transfer, | ||
| my_agent, | ||
| my_mem_descs, | ||
| my_file_descs, | ||
| my_agent.name, | ||
| "READ", | ||
| ) | ||
| ) | ||
| s += 1 | ||
| continue |
There was a problem hiding this comment.
Maybe we can move this line before the loop, initiating s to 1?
I think it would simplify the loop and help avoid a branch
|
|
||
| end_time = time.time() | ||
|
|
||
| logger.info(f"Time for {iterations} iterations: {end_time - start_time} seconds") |
There was a problem hiding this comment.
f-string -> %s logger formatting
| ### Pipelining | ||
|
|
||
| To improve performance of the remote storage server, we can pipeline operations to network and storage. This pipelining allows multiple threads to handle each request. However, in order to maintain correctness, the order of network and storage must happen in order for each individual remote storage operation. To do this, we implemented a simple pipelining scheme. | ||
|
|
There was a problem hiding this comment.
@tstamler please can you add some more description on how this is implemented with NIXL in this example?
|
|
||
| ### Performance Tips | ||
|
|
||
| For high-speed storage and network hardware, you may need to tweak performance with a couple of environment variables. |
There was a problem hiding this comment.
Please provide suggested list of env - vars, (like providing example configuration with CX-7, SPX and providing what UCX tuning/GDS tuning that is seen to be beneficial?
|
|
||
| For high-speed storage and network hardware, you may need to tweak performance with a couple of environment variables. | ||
|
|
||
| First, for optimal GDS performance, ensure you are using the GDS_MT backend with default concurrency. Additionally, you can use the cufile options described in the [GDS README](https://github.com/ai-dynamo/nixl/blob/main/src/plugins/cuda_gds/README.md). |
There was a problem hiding this comment.
We should also highlight the difference between the GDS backend and that GDS plugins can work in compat mode by default, and what is required for true GDS support.
There was a problem hiding this comment.
+1 to @roiedanino comment
3 participants
What?
Update the remote storage example with pipelining code and more details in README.