Fix recv_buffer issue in LLaMA3 DP training - #279
Conversation
Summary of ChangesHello @whollo, 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 resolves a 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
|
![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 improves the user experience by making environment variable checks case-insensitive through the use of strcasecmp. This is a good enhancement for robustness. I have one suggestion to improve portability by replacing the POSIX-specific strcasecmp with a custom helper function. The PR also includes a minor formatting fix and adds a missing newline at the end of a file, which is good practice for tool compatibility. Overall, these are positive changes for the codebase.
| } | ||
| return flagcxSuccess; | ||
| } No newline at end of file | ||
| } |
There was a problem hiding this comment.
Thank you for adding a newline at the end of the file. It's a good practice that ensures file integrity and compatibility with various tools that expect text files to end with a newline character.
|
@gemini-code-assist please review this pull request in details |
There was a problem hiding this comment.
Code Review
This pull request primarily introduces formatting changes via clang-format and enhances the robustness of environment variable checks by making them case-insensitive. While adding checks for alternative capitalizations (e.g., True alongside TRUE) is a good step, I've suggested using strcasecmp for a more comprehensive case-insensitive comparison. This would handle all variations (e.g., true, TRUE, True) and improve both user experience and code maintainability. The other changes are minor formatting adjustments that improve code readability.
| if (algorithm != NULL && (strcmp(algorithm, "RING_PIPELINED") == 0 || | ||
| strcmp(algorithm, "Ring_pipelined") == 0)) { |
There was a problem hiding this comment.
For better robustness and to handle any case variations (e.g., ring_pipelined, Ring_Pipelined), it's recommended to use a case-insensitive string comparison function like strcasecmp. This will make the environment variable check more user-friendly.
You'll need to include <strings.h> for strcasecmp.
if (algorithm != NULL && strcasecmp(algorithm, "RING_PIPELINED") == 0) {| if (algorithm != NULL && (strcmp(algorithm, "XML_INPUT") == 0 || | ||
| strcmp(algorithm, "Xml_input") == 0)) { |
There was a problem hiding this comment.
To handle all case variations of XML_INPUT (like xml_input, Xml_Input, etc.), consider using strcasecmp for a case-insensitive comparison. This improves robustness.
Remember to include <strings.h>.
if (algorithm != NULL && strcasecmp(algorithm, "XML_INPUT") == 0) {| (strcmp(enableTopoDetect, "TRUE") == 0 || | ||
| strcmp(enableTopoDetect, "True") == 0)) { |
There was a problem hiding this comment.
Using strcasecmp for a case-insensitive comparison against "TRUE" would be more robust, as it would also match "true" and "tRuE". This improves the user experience when setting environment variables.
Make sure to include <strings.h>.
strcasecmp(enableTopoDetect, "TRUE") == 0) {| FLAGCXCHECK(flagcxNetInit(comm)); | ||
| INFO(FLAGCX_INIT, "Using network %s", comm->netAdaptor->name); | ||
| if (env && strcmp(env, "TRUE") == 0) { | ||
| if (env && (strcmp(env, "TRUE") == 0 || strcmp(env, "True") == 0)) { |
There was a problem hiding this comment.
Using strcasecmp for a case-insensitive comparison against "TRUE" would be more robust, as it would also match "true" and "tRuE". This improves the user experience when setting environment variables.
Make sure to include <strings.h>.
| if (env && (strcmp(env, "TRUE") == 0 || strcmp(env, "True") == 0)) { | |
| if (env && strcasecmp(env, "TRUE") == 0) { |
| (strcmp(enable_topo_detect, "TRUE") == 0 || | ||
| strcmp(enable_topo_detect, "True") == 0)) { |
There was a problem hiding this comment.
Using strcasecmp for a case-insensitive comparison against "TRUE" would be more robust, as it would also match "true" and "tRuE". This improves the user experience when setting environment variables.
Make sure to include <strings.h>.
strcasecmp(enable_topo_detect, "TRUE") == 0) {| if (enableTopoDetect && (strcmp(enableTopoDetect, "TRUE") == 0 || | ||
| strcmp(enableTopoDetect, "True") == | ||
| 0)) { // safety check nic distance is only |
There was a problem hiding this comment.
Using strcasecmp for a case-insensitive comparison against "TRUE" would be more robust, as it would also match "true" and "tRuE". This improves the user experience when setting environment variables.
Make sure to include <strings.h>.
if (enableTopoDetect && strcasecmp(enableTopoDetect, "TRUE") == 0) { // safety check nic distance is only
037cb2c to
363831d
Compare
--------- Co-authored-by: MC952-arch <MC952-arch@qq.com>
3 participants
Fix the recv_buffer issue encountered when using the DP (Data Parallel) strategy during LLaMA3 training.