-
Notifications
You must be signed in to change notification settings - Fork 371
feat(redis): add Redis backend with async transfers and nixlbench support #1791
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
base: main
Are you sure you want to change the base?
Changes from all commits
02ac5ee
a190d3c
a2eb4f7
75d1190
a0064ba
45c71dd
327e9fd
b3643b1
bf5dc6e
e3b6d2d
9233e92
025eb19
3ce356b
099be21
c997861
d851589
d97b413
ce0c697
7c8d886
c1b6fc6
0657e6a
c5d5ca1
936d97b
f59892b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -94,6 +94,7 @@ | |
| #define XFERBENCH_BACKEND_MOONCAKE "Mooncake" | ||
| #define XFERBENCH_BACKEND_HF3FS "HF3FS" | ||
| #define XFERBENCH_BACKEND_OBJ "OBJ" | ||
| #define XFERBENCH_BACKEND_REDIS "REDIS" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial | 💤 Low value Prefer Per repository conventions, new runtime/backend string constants should use ♻️ Suggested refactor-#define XFERBENCH_BACKEND_REDIS "REDIS"
+constexpr inline std::string_view xferbench_backend_redis = "REDIS";Note: This would require updating all usages to use 🤖 Prompt for AI AgentsSource: Learnings |
||
| #define XFERBENCH_BACKEND_GUSLI "GUSLI" | ||
| #define XFERBENCH_BACKEND_UCCL "UCCL" | ||
| #define XFERBENCH_BACKEND_AZURE_BLOB "AZURE_BLOB" | ||
|
|
@@ -376,6 +377,8 @@ class xferBenchUtils { | |
| static bool | ||
| putObj(size_t buffer_size, const std::string &name); | ||
| static bool | ||
| putRedis(size_t buffer_size, const std::string &key); | ||
| static bool | ||
| getObj(const std::string &name); | ||
| static bool | ||
| rmObj(const std::string &name); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Don't make hiredis mandatory for every nixlbench build.
utils.cppnow pulls in hiredis unconditionally, and adding it here asrequired: truemeans even UCX/POSIX/OBJ-only benchmark builds fail at configure time whenlibhiredis-devis absent. The Dockerfile/build script updates only cover the container path, so local/source builds lose the previous non-Redis build path. Please gate the Redis helpers behind a Meson option or move them into a Redis-only translation unit so non-Redis nixlbench builds still configure cleanly.🤖 Prompt for AI Agents