Conversation
Signed-off-by: yiliu30 <yi4.liu@intel.com>
Signed-off-by: yiliu30 <yi4.liu@intel.com>
Signed-off-by: yiliu30 <yi4.liu@intel.com>
Signed-off-by: yiliu30 <yi4.liu@intel.com>
Signed-off-by: yiliu30 <yi4.liu@intel.com>
|
👋 Hi! Thank you for contributing to llm-compressor. Please add the ready label when the PR is ready for review. Note: This is required to complete the testing suite, please only add the label once the PR is code complete and local testing has been performed. |
Summary of ChangesHello @yiliu30, 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 Distributed Data Parallel (DDP) capabilities to the AutoRound quantization process within the Highlights
Changelog
Activity
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 introduces a proof-of-concept for Distributed Data Parallel (DDP) support, primarily for AutoRound quantization. It adds a new DDP example, modifies dataset utilities to support distributed sampling, and updates the AutoRound modifier to be DDP-aware.
My review focuses on cleaning up the proof-of-concept code. I've identified several instances of dead or commented-out code in the new example files that should be removed for clarity. I also suggest replacing a print statement with a proper logger call in the AutoRoundModifier and removing an unused debugging utility class. These changes will improve the code's readability and maintainability as it moves from a PoC to a more permanent feature.
| MODEL_ID = "meta-llama/Meta-Llama-3-8B-Instruct" | ||
|
|
||
| MODEL_ID = "/storage/yiliu7/Qwen/Qwen3-30B-A3B-Instruct-2507-FP8" |
There was a problem hiding this comment.
The MODEL_ID variable is defined on line 8 and then immediately overwritten on line 9. The first assignment is now dead code and can be confusing. Please remove the unused assignment.
| MODEL_ID = "meta-llama/Meta-Llama-3-8B-Instruct" | |
| MODEL_ID = "/storage/yiliu7/Qwen/Qwen3-30B-A3B-Instruct-2507-FP8" | |
| MODEL_ID = "/storage/yiliu7/Qwen/Qwen3-30B-A3B-Instruct-2507-FP8" |
| model_id = "Qwen/Qwen3-235B-A22B" | ||
| model_id = "Qwen/Qwen3-8B" |
There was a problem hiding this comment.
The model_id variable is defined on line 14 and then immediately overwritten on line 15. The first assignment is dead code. Please remove it to improve clarity.
| model_id = "Qwen/Qwen3-235B-A22B" | |
| model_id = "Qwen/Qwen3-8B" | |
| model_id = "Qwen/Qwen3-8B" |
| # except Exception as e: | ||
| # logger.info(f"[Rank {rank}] Error during quantization: {e}") | ||
| # raise | ||
|
|
||
| # finally: | ||
| # # Cleanup DDP |
There was a problem hiding this comment.
This commented-out try-except-finally block appears to be leftover from debugging. It should be removed to clean up the code.
| # Parse scheme from string if needed | ||
| from auto_round import schemes as ar_schemes | ||
|
|
||
| scheme_map = { | ||
| "FP8_STATIC": ar_schemes.FP8_STATIC, | ||
| "MXFP8": ar_schemes.MXFP8, | ||
| "MXFP4": ar_schemes.MXFP4, | ||
| } | ||
| # scheme = scheme_map.get(args.scheme, args.scheme) |
There was a problem hiding this comment.
This block for parsing the scheme seems to be unused as the line that would use scheme_map is commented out. This makes the import and the map itself dead code. Please remove this block if it's not needed.
| # # Check if running with torchrun | ||
| # if "RANK" in os.environ and "WORLD_SIZE" in os.environ: | ||
| # logger.info("Detected torchrun environment") | ||
| # main_torchrun(model_name, scheme, args.iters, args.nsamples) |
There was a problem hiding this comment.
This commented-out block for handling torchrun is unused. It should be removed to improve code clarity.
| import pdb | ||
| import sys | ||
|
|
||
|
|
||
| class ForkedPdb(pdb.Pdb): | ||
| """A Pdb subclass that may be used | ||
| from a forked multiprocessing child | ||
|
|
||
| """ | ||
|
|
||
| def interaction(self, *args, **kwargs): | ||
| _stdin = sys.stdin | ||
| try: | ||
| sys.stdin = open("/dev/stdin") | ||
| pdb.Pdb.interaction(self, *args, **kwargs) | ||
| finally: | ||
| sys.stdin = _stdin | ||
|
|
There was a problem hiding this comment.
The ForkedPdb class and its imports (pdb, sys) are defined but not used within this file, making it dead code. This appears to be a debugging utility and should be removed from the production codebase.
| kwargs["device_map"] = ( | ||
| f"cuda:{rank}" if torch.cuda.is_available() else "cpu" | ||
| ) | ||
| print(f"AutoRoundModifier: moving wrapped_model to {kwargs['device_map']}", flush=True) |
There was a problem hiding this comment.
A print statement is used for logging. It's better to use the existing logging framework for consistency and better control over log levels and output. Please replace this with a call to rank_log.
| print(f"AutoRoundModifier: moving wrapped_model to {kwargs['device_map']}", flush=True) | |
| rank_log(f"AutoRoundModifier: moving wrapped_model to {kwargs['device_map']}") |
Signed-off-by: yiliu30 <yi4.liu@intel.com>
Signed-off-by: yiliu30 <yi4.liu@intel.com>
1 participant
SUMMARY:
"please provide a brief summary"
TEST PLAN:
"please outline how the changes were tested"