Skip to content

[maxtext] improve profiling in decode #1863

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

Merged
merged 1 commit into from
Jun 25, 2025
Merged

Conversation

copybara-service[bot]
Copy link

@copybara-service copybara-service bot commented Jun 23, 2025

[maxtext] improve profiling in decode

Limit the profiling of decode to config.profiler_steps steps. User may request generate many tokens, but since each generation is identical, it's not necessary to profile all of them. Profiling only the first few should be sufficient.

Also added a method in Profiler to do post-pocessing on the collected trace.

Copy link

google-cla bot commented Jun 23, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@@ -33,6 +33,7 @@

# Number of text sequences to process in a single batch.
_NUM_STREAMS = 1
_MAX_STEPS_TO_PROFILE = 10
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use the general MaxText profiling config?

@@ -92,3 +92,6 @@ def should_activate_periodic_profile(self, step):

def should_deactivate_periodic_profile(self, step):
return self.profile_period > 0 and (step - self.finished_initial_profile_step) % self.profile_period == 0

def post_process(self):
pass
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plan is to fill this in later, right?

Copy link
Collaborator

@SurbhiJainUSC SurbhiJainUSC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is profiling even added to decode.py? Looks like the PR got merged without any review: #1814.

@bvandermoon
Copy link
Collaborator

bvandermoon commented Jun 24, 2025

Why is profiling even added to decode.py? Looks like the PR got merged without any review: #1814.

@SurbhiJainUSC As FYI - the internal cl number is in the branch name at the top of the PR: 769698398. Hard to find if you haven't seen it before

@copybara-service copybara-service bot force-pushed the test_773929904 branch 2 times, most recently from cb6d96c to a7213ce Compare June 24, 2025 18:01
@SurbhiJainUSC
Copy link
Collaborator

Why is profiling even added to decode.py? Looks like the PR got merged without any review: #1814.

@SurbhiJainUSC As FYI - the internal cl number is in the branch name at the top of the PR: 769698398. Hard to find if you haven't seen it before

ohh, I got it. Thank you for pointing that out.

Limit the profiling of decode to `config.profiler_steps` steps. User may request generate many tokens, but since each generation is identical, it's not necessary to profile all of them. Profiling only the first few should be sufficient.

Also added a method in Profiler to do post-pocessing on the collected trace.

PiperOrigin-RevId: 775799587
@copybara-service copybara-service bot merged commit 3a78db6 into main Jun 25, 2025
@copybara-service copybara-service bot deleted the test_773929904 branch June 25, 2025 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants