add nccl env intercept example code - #269
Conversation
Summary of ChangesHello @yzhang35, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust system for intercepting and controlling NCCL environment variables, enhancing the flexibility of NCCL parameter configuration. It includes the core C++ logic for environment variable interception, an example for a specific NCCL parameter, and integrates this new capability into the existing Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
env_verify.log |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds an example of how to intercept NCCL environment variables by preloading a shared library. The changes include a new Makefile, the C++ source for the shared library, and a fix in flagcx_tuner.cc to correctly classify an environment variable type.
My review focuses on the new C++ code. I've suggested several improvements to nccl_param.cc to make it more robust, idiomatic with modern C++, and easier to maintain. This includes using std::mutex for thread safety, simplifying logic, and cleaning up headers. I also pointed out a typo in a comment that could be confusing for developers using this example.
| @@ -0,0 +1,33 @@ | |||
| // This is a example of intercept NCCL environment getter functions. | |||
| #include <stdio.h> | |||
There was a problem hiding this comment.
The <stdio.h> header is not used in this file and can be removed to keep the includes clean. Additionally, for better C++ code style, consider using the C++-style headers <cstdlib> and <cstring> instead of <stdlib.h> and <cstring>.
| static void ncclLoadParam(char const* env, int64_t deftVal, int64_t* value) { | ||
| static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER; | ||
| pthread_mutex_lock(&mutex); | ||
| const char* str = getenv(env); | ||
| *value = deftVal; | ||
| if (str && strlen(str) > 0) { | ||
| try { | ||
| *value = std::stoll(str); | ||
| } catch (const std::exception& e) { | ||
| *value = deftVal; | ||
| } | ||
| } | ||
| pthread_mutex_unlock(&mutex); | ||
| } |
There was a problem hiding this comment.
This function can be improved for better C++11 style and clarity:
- Consider using
std::mutexandstd::lock_guardinstead of pthreads for thread synchronization. This is more idiomatic C++ and safer as it uses RAII to ensure the mutex is always unlocked. This would require including<mutex>and removing<pthread.h>. - The assignment
*value = deftVal;inside thecatchblock is redundant because*valuehas already been set todeftValon line 13.
Here is a suggested refactoring that applies both points:
static void ncclLoadParam(char const* env, int64_t deftVal, int64_t* value) {
static std::mutex param_mutex;
std::lock_guard<std::mutex> lock(param_mutex);
const char* str = getenv(env);
*value = deftVal;
if (str && strlen(str) > 0) {
try {
*value = std::stoll(str);
} catch (const std::exception& e) {
// If parsing fails, *value keeps deftVal assigned earlier.
}
}
}| return value; \ | ||
| } | ||
|
|
||
| // In order to intercept a NCCL environment getter function, add an additional line of NCCL_PRARM here. |
There was a problem hiding this comment.
There's a typo in this explanatory comment. NCCL_PRARM should be NCCL_PARAM to match the macro defined above. This will help users who read this example.
// In order to intercept a NCCL environment getter function, add an additional line of NCCL_PARAM here.
3 participants
No description provided.