Skip to content

Conversation

@jamesjwu
Copy link
Contributor

@jamesjwu jamesjwu commented Oct 10, 2024

Stack from ghstack (oldest at bottom):

My previous PR made these eager instead of lazy, making it regress some small pr time benchmarks that don't use structured trace. Making them lazy again by putting them under a function call fixes the issue. Production jobs should not be affected by the change either way, because structured logs are turned on and evaluated there.

Without this diff:

collecting compile time instruction count for aotdispatcher_inference_nosubclass_cpu
compile time instruction count for iteration 0 is 10241330219
compile time instruction count for iteration 1 is 1970239415
compile time instruction count for iteration 2 is 1955721527
compile time instruction count for iteration 3 is 1949317363
compile time instruction count for iteration 4 is 1947567621
collecting compile time instruction count for aotdispatcher_training_nosubclass_cpu
compile time instruction count for iteration 0 is 4203676200
compile time instruction count for iteration 1 is 3739626299
compile time instruction count for iteration 2 is 3736479815
compile time instruction count for iteration 3 is 3734131630
compile time instruction count for iteration 4 is 3733066773
collecting compile time instruction count for aotdispatcher_inference_subclass_cpu
compile time instruction count for iteration 0 is 5621749132
compile time instruction count for iteration 1 is 5621658470
compile time instruction count for iteration 2 is 5620004680
compile time instruction count for iteration 3 is 5617585817
compile time instruction count for iteration 4 is 5618850646
collecting compile time instruction count for aotdispatcher_training_subclass_cpu
compile time instruction count for iteration 0 is 10005741891
compile time instruction count for iteration 1 is 10003433898
compile time instruction count for iteration 2 is 10002792031
compile time instruction count for iteration 3 is 10000217081
compile time instruction count for iteration 4 is 10000992803

With this diff:

collecting compile time instruction count for aotdispatcher_inference_nosubclass_cpu
compile time instruction count for iteration 0 is 10128746827
compile time instruction count for iteration 1 is 1965846584
compile time instruction count for iteration 2 is 1952468717
compile time instruction count for iteration 3 is 1946109980
compile time instruction count for iteration 4 is 1944459203
collecting compile time instruction count for aotdispatcher_training_nosubclass_cpu
compile time instruction count for iteration 0 is 4052066041
compile time instruction count for iteration 1 is 3589558675
compile time instruction count for iteration 2 is 3585668104
compile time instruction count for iteration 3 is 3584020963
compile time instruction count for iteration 4 is 3583162039
collecting compile time instruction count for aotdispatcher_inference_subclass_cpu
compile time instruction count for iteration 0 is 5617038583
compile time instruction count for iteration 1 is 5617303182
compile time instruction count for iteration 2 is 5613289422
compile time instruction count for iteration 3 is 5612847744
compile time instruction count for iteration 4 is 5612093362
collecting compile time instruction count for aotdispatcher_training_subclass_cpu
compile time instruction count for iteration 0 is 9705752209
compile time instruction count for iteration 1 is 9704758787
compile time instruction count for iteration 2 is 9703760884
compile time instruction count for iteration 3 is 9704687557
compile time instruction count for iteration 4 is 9704824140

Specifically, aotdispatcher_training_nosubclass_cpu and aotdispatcher_training_subclass_cpu look better

@pytorch-bot
Copy link

pytorch-bot bot commented Oct 10, 2024

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/137700

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit 1915f6f with merge base 839d356 (image):

NEW FAILURE - The following job has failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

jamesjwu added a commit that referenced this pull request Oct 10, 2024
ghstack-source-id: 27ce8f6
Pull Request resolved: #137700
@jamesjwu jamesjwu added ciflow/trunk Trigger trunk jobs on your pull request topic: not user facing topic category labels Oct 10, 2024
),
)
joint_graph_str = fx_g.print_readable(
joint_graph_str_fn = lambda: fx_g.print_readable(
Copy link
Contributor

Choose a reason for hiding this comment

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

we have a LazyString class we use in a few other places to lazily materialize strings for logging - maybe we should use it here for consistency (to be fair it is a pretty tiny wrapper) https://github.com/pytorch/pytorch/blob/main/torch/_logging/_internal.py#L1083

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah in this case since we're passing a lambda anyway I figured making it lazy string was literally more overhead

Copy link
Collaborator

Choose a reason for hiding this comment

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

You could technically just pass a functools.partial :P

@jamesjwu
Copy link
Contributor Author

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: 1 mandatory check(s) failed. The first few are:

Dig deeper by viewing the failures on hud

Details for Dev Infra team Raised by workflow job

Failing merge rule: Core Maintainers

@laithsakka
Copy link
Contributor

if those effect
"basic_modules_ListOfLinears_inductor": 3.7966236673926,
"basic_modules_ListOfLinears_inductor_gpu_force_shape_pad": 2.1187519601234
you might have to rebase an update expected value if you are not already on top of:
#137541

@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2024

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

@github-actions github-actions bot added the Stale label Dec 9, 2024
@github-actions github-actions bot closed this Jan 8, 2025
@github-actions github-actions bot deleted the gh/jamesjwu/70/head branch February 9, 2025 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Stale topic: not user facing topic category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants