(feat)Example MmaDA: update performance in readme #1377
Conversation
wtomin
requested review from
CaitinZhao,
SamitHuang,
vigo999 and
zhanghuiyao
as code owners
Summary of ChangesHello @wtomin, 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 primarily focuses on enhancing the MmaDA example by integrating performance metrics and updating documentation. It introduces throughput logging in inference scripts and comprehensive performance tables in the README, offering users clearer benchmarks. Additionally, it addresses a critical bug in the 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 adds performance metrics to the MMaDA example, including throughput calculations in inference scripts and performance tables in the README. It also includes a fix for Zero2 training in PyNative mode.
My review focuses on the correctness of the new performance calculations. I've found a few issues where the duration measurement for throughput is inaccurate because it includes post-processing time, and in one case, the formula for calculating tokens per second seems incorrect. I've provided suggestions to fix these issues to ensure the reported performance metrics are accurate. The documentation updates and the fix in zero.py look good.
| print(f"Inference time: {time() - infer_start:.3f}s") | ||
| print(f"Throughput: {out.shape[1] / (time() - infer_start):.3f} token/s") |
There was a problem hiding this comment.
The duration time() - infer_start is calculated twice, which can lead to slight inaccuracies in both the reported inference time and throughput. The second call to time() will result in a slightly longer duration, making the throughput appear lower than it is. It's better to calculate the duration once and reuse it for both print statements.
| print(f"Inference time: {time() - infer_start:.3f}s") | |
| print(f"Throughput: {out.shape[1] / (time() - infer_start):.3f} token/s") | |
| duration = time() - infer_start | |
| print(f"Inference time: {duration:.3f}s") | |
| print(f"Throughput: {out.shape[1] / duration:.3f} token/s") |
| output_ids = model.mmu_generate(input_ids, max_new_tokens=1024, steps=512, block_length=1024) | ||
| text = uni_prompting.text_tokenizer.batch_decode( | ||
| output_ids[:, input_ids.shape[1] :], skip_special_tokens=True | ||
| ) | ||
| throughputs.append(output_ids.shape[1] / (time() - infer_start)) |
There was a problem hiding this comment.
The duration for throughput calculation is measured after the batch_decode operation. This includes post-processing time and will lead to an inaccurate (lower) throughput value for the model generation. The duration should be measured immediately after the model.mmu_generate call.
| output_ids = model.mmu_generate(input_ids, max_new_tokens=1024, steps=512, block_length=1024) | |
| text = uni_prompting.text_tokenizer.batch_decode( | |
| output_ids[:, input_ids.shape[1] :], skip_special_tokens=True | |
| ) | |
| throughputs.append(output_ids.shape[1] / (time() - infer_start)) | |
| output_ids = model.mmu_generate(input_ids, max_new_tokens=1024, steps=512, block_length=1024) | |
| infer_duration = time() - infer_start | |
| text = uni_prompting.text_tokenizer.batch_decode( | |
| output_ids[:, input_ids.shape[1] :], skip_special_tokens=True | |
| ) | |
| throughputs.append(output_ids.shape[1] / infer_duration) |
| if isinstance(getattr(self.optimizer, attr), ms.ParameterTuple): | ||
| if attr in ["_parameters", "parameters"]: | ||
| if ms.get_context("mode") == ms.PYNATIVE_MODE: | ||
| for name in self.optimizer._params_list: |
There was a problem hiding this comment.
somehow in MS2.7, self.optimizer._params_list is a empty list in ZeRO3, better to test if zero3 works
4 participants
What does this PR do?
Adds # (feature)
mindone/trainers/zero.pyintroduced by [FEATURE] Support Qwen2.5VL training #1214 caused a parameter shape error in Zero2 training of MmaDA. Falled back to the previous code snippet.Before submitting
What's New. Here are thedocumentation guidelines
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@SamitHuang @zhtmike @vigo999