-
Notifications
You must be signed in to change notification settings - Fork 88
mindone adapted ms2.6 && ms2.7 #1168
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: master
Are you sure you want to change the base?
Conversation
bc038b6
to
d839b94
Compare
Update here too? Line 33 in 5a76add
|
/gemini review |
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.
Code Review
This pull request adapts the code for MindSpore versions 2.6 and 2.7. The changes mainly involve updating the JIT optimization level and replacing deprecated functions like chunk_ext
with mint.chunk
.
I've found a critical issue in examples/opensora_hpcai/scripts/inference.py
where the performance metric for sampling speed is calculated incorrectly. This leads to misleading logs. I've provided a suggestion to fix the formula and use the correct variable for batch size. The other changes look good and are consistent with the goal of adapting to a new MindSpore version.
batch_time = time.time() - start_time | ||
logger.info( | ||
f"Batch time cost: {batch_time:.3f}s, sampling speed: {args.sampling_steps * ns / batch_time:.4f} step/s" | ||
f"Batch time cost: {batch_time:.3f}s, sampling speed: {batch_time / args.sampling_steps * ns:.4f} s/step" |
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.
The formula for calculating sampling speed in s/step
is incorrect. This will result in misleading performance metrics being logged.
There are two issues with the current calculation:
-
Incorrect formula: To calculate seconds per step (
s/step
), you should divide the total time by the total number of steps. The current formulabatch_time / args.sampling_steps * ns
is evaluated as(batch_time / args.sampling_steps) * ns
due to operator precedence, which is not mathematically correct fors/step
. It should bebatch_time / (args.sampling_steps * num_samples)
. -
Incorrect number of samples: The calculation uses
ns
, which is the total number of prompts for the entire run. For a per-batch performance metric, it should use the number of samples in the current batch. You can get this fromsamples.shape[0]
, assamples
is available in this scope.
I've suggested a fix below that addresses both points.
f"Batch time cost: {batch_time:.3f}s, sampling speed: {batch_time / args.sampling_steps * ns:.4f} s/step" | |
f"Batch time cost: {batch_time:.3f}s, sampling speed: {batch_time / (args.sampling_steps * samples.shape[0]):.4f} s/step" |
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.
@QYuFong Pls fix
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.
Done
|
d839b94
to
2142611
Compare
I mean update typing and docs: |
What does this PR do?
Fixes # (issue)
Adds # (feature)
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.
@xxx