[inductor] added a config to always add tensor constants #110491

chenyang78 · 2023-10-04T00:31:23Z

Summary:
In some scenarios, we want to update constants at runtime.
In such cases, we have to keep the original constants in
the generated code without applying any constant-inlining
optimizations.

This PR adds a config to force us to add tensor constants.

Differential Revision: D49895154

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @Xia-Weiwen @wenzhe-nrv @jiayisunx @peterbell10 @ipiszy @yf225 @kadeng @muchulee8 @aakhundov @ColinPeppler

pytorch-bot · 2023-10-04T00:31:26Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

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

✅ You can merge normally! (2 Unrelated Failures)

As of commit 4007e1e with merge base e8f1f4e ():

UNSTABLE - The following jobs failed but were likely due to flakiness present on trunk and has been marked as unstable:

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

facebook-github-bot · 2023-10-04T00:31:30Z

This pull request was exported from Phabricator. Differential Revision: D49895154

jansel · 2023-10-04T03:52:58Z

torch/_inductor/graph.py

                name = f"constant_{name}"
+            # We may generate a var name for each constant in the codegen.
+            # Let's only keep sane characters.
+            name = re.sub(r"[^a-zA-Z0-9_]", "_", name)


Where are the non-indent chars coming from? Could this result in name collisions?

The special chars come from the model. For example, some model that we are dealing with has a bias named like this abc:XYZ, where : is not allowed in a var name in C/C++.

Very good point regarding the name collisions! Yes, I think we could have name collisions, although the chance might be low. Perhaps we could add a counter suffix to make a unique name? e.g.

name = re.sub(r"[^a-zA-Z0-9_]", "_", name) name = f"{name}_{next(self.constant_id)"

What do you think? Thanks.

This is generated code and doesn't bleed into how we should store weights for weight refresh (which is presumably the unmodified original name as a string). Do any users need to reference the these generated names (which which case adding a suffix should be a last resort in case of duplicates, and track previously seen names), if it's just for readability during debug then adding the suffix doesn't appear to have much if any downside.

Let's add a suffix and also check for collision.

I don't think the user needs to reference the generated names.

Let's add a suffix and also check for collision.

Done. Thanks.

Let's use while for adding suffix and checking.
Something like:

prefix = re.sub(r"[^a-zA-Z0-9_]", "_", name) name = prefix cnt = 0 while name in self.constants: name = f"{prefix}_{cnt}" cnt += 1

In this way the name is more tracible to the original model, since it's easy to see integers appended to some variable for un-named vars (is add_0 a constant or the result of some add operation?)
Also, I don't really need to assert if our artificial name collides, we could just create a new one instead.

Good point! Fixed. Thanks.

facebook-github-bot · 2023-10-04T17:49:25Z

This pull request was exported from Phabricator. Differential Revision: D49895154

) Summary: In some scenarios, we want to update constants at runtime. In such cases, we have to keep the original constants in the generated code without applying any constant-inlining optimizations. This PR adds a config to force us to add tensor constants. Reviewed By: muchulee8 Differential Revision: D49895154

facebook-github-bot · 2023-10-04T17:53:36Z

This pull request was exported from Phabricator. Differential Revision: D49895154

facebook-github-bot · 2023-10-04T20:20:06Z

This pull request was exported from Phabricator. Differential Revision: D49895154

) Summary: In some scenarios, we want to update constants at runtime. In such cases, we have to keep the original constants in the generated code without applying any constant-inlining optimizations. This PR adds a config to force us to add tensor constants. Reviewed By: muchulee8 Differential Revision: D49895154

facebook-github-bot · 2023-10-04T20:20:37Z

This pull request was exported from Phabricator. Differential Revision: D49895154

facebook-github-bot · 2023-10-05T19:07:23Z

This pull request was exported from Phabricator. Differential Revision: D49895154

facebook-github-bot · 2023-10-05T19:07:28Z

This pull request was exported from Phabricator. Differential Revision: D49895154

facebook-github-bot · 2023-10-05T22:33:50Z

This pull request was exported from Phabricator. Differential Revision: D49895154

) Summary: In some scenarios, we want to update constants at runtime. In such cases, we have to keep the original constants in the generated code without applying any constant-inlining optimizations. This PR adds a config to force us to add tensor constants. Reviewed By: muchulee8 Differential Revision: D49895154

facebook-github-bot · 2023-10-05T22:34:02Z

This pull request was exported from Phabricator. Differential Revision: D49895154

) Summary: In some scenarios, we want to update constants at runtime. In such cases, we have to keep the original constants in the generated code without applying any constant-inlining optimizations. This PR adds a config to force us to add tensor constants. Reviewed By: muchulee8 Differential Revision: D49895154

facebook-github-bot · 2023-10-06T16:41:46Z

This pull request was exported from Phabricator. Differential Revision: D49895154

facebook-github-bot · 2023-10-06T16:42:04Z

This pull request was exported from Phabricator. Differential Revision: D49895154

facebook-github-bot · 2023-10-07T07:49:46Z

@pytorchbot merge

(Initiating merge automatically since Phabricator Diff has merged)

pytorchmergebot · 2023-10-07T07:51:36Z

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

facebook-github-bot added the fb-exported label Oct 4, 2023

chenyang78 requested a review from jansel October 4, 2023 00:31

github-actions bot added module: inductor ciflow/inductor labels Oct 4, 2023

chenyang78 requested review from bertmaher, mikekgfb and muchulee8 October 4, 2023 00:31

jansel requested changes Oct 4, 2023

View reviewed changes

mikekgfb approved these changes Oct 4, 2023

View reviewed changes

chenyang78 added the topic: not user facing topic category label Oct 4, 2023

chenyang78 force-pushed the export-D49895154 branch from 7f4aeb1 to 3948190 Compare October 4, 2023 17:49

chenyang78 force-pushed the export-D49895154 branch from 3948190 to dc6bbb3 Compare October 4, 2023 17:53

chenyang78 requested a review from jansel October 4, 2023 18:22

chenyang78 force-pushed the export-D49895154 branch from dc6bbb3 to 3d88cc3 Compare October 4, 2023 20:20

chenyang78 force-pushed the export-D49895154 branch 2 times, most recently from dae0315 to cb396b5 Compare October 5, 2023 19:07

chenyang78 force-pushed the export-D49895154 branch from 7207a4c to dae0315 Compare October 5, 2023 19:07

chenyang78 force-pushed the export-D49895154 branch from cb396b5 to 857b323 Compare October 5, 2023 22:33

chenyang78 force-pushed the export-D49895154 branch from 857b323 to ab03192 Compare October 5, 2023 22:33

chenyang78 force-pushed the export-D49895154 branch from ab03192 to bcad8e3 Compare October 6, 2023 16:41

chenyang78 force-pushed the export-D49895154 branch from bcad8e3 to 4007e1e Compare October 6, 2023 16:41

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 7, 2023

pytorchmergebot added the merging label Oct 7, 2023

pytorchmergebot added Merged and removed merging labels Oct 7, 2023

pytorchmergebot closed this in 432df71 Oct 7, 2023

[inductor] added a config to always add tensor constants #110491

[inductor] added a config to always add tensor constants #110491

Uh oh!

Conversation

chenyang78 commented Oct 4, 2023 • edited by pytorch-bot bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

✅ You can merge normally! (2 Unrelated Failures)

Uh oh!

facebook-github-bot commented Oct 4, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 4, 2023

Uh oh!

facebook-github-bot commented Oct 4, 2023

Uh oh!

facebook-github-bot commented Oct 4, 2023

Uh oh!

facebook-github-bot commented Oct 4, 2023

Uh oh!

facebook-github-bot commented Oct 5, 2023

Uh oh!

facebook-github-bot commented Oct 5, 2023

Uh oh!

facebook-github-bot commented Oct 5, 2023

Uh oh!

facebook-github-bot commented Oct 5, 2023

Uh oh!

facebook-github-bot commented Oct 6, 2023

Uh oh!

facebook-github-bot commented Oct 6, 2023

Uh oh!

facebook-github-bot commented Oct 7, 2023

Uh oh!

pytorchmergebot commented Oct 7, 2023

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

chenyang78 commented Oct 4, 2023 •

edited by pytorch-bot bot

Loading

pytorch-bot bot commented Oct 4, 2023 •

edited

Loading