[Inductor] Update AttrsDescriptor instantiation for Triton changes #137458

alexbaden · 2024-10-08T00:03:31Z

The AttrsDescriptor class has been present in Triton for almost a year now (introduced here), so we should be able to rely on it existing. I am in the process of supporting the new AttrsDescriptor class and @jansel suggested I split changes to the existing class out separately to make sure nothing breaks removing the legacy attribute descriptor attributes.

Initially I attempted to remove the branching around detecting whether AttrsDescriptor exists but that breaks because PyTorch must build without Triton. So, I went back and updated for the naming introduced in the commit linked above, and also removed two unused attributes divisible_by_8 and ids_to_fold which were removed in Feb 2024 (triton-lang/triton#3122 and triton-lang/triton#3080 respectively).

With these changes only the internal workings of the AttrsDescriptor class will differ between supported Triton versions, but the data stored will remain consistent.

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @ipiszy @yf225 @chenyang78 @kadeng @muchulee8 @ColinPeppler @amjames @desertfire @chauhang

pytorch-bot · 2024-10-08T00:03:35Z

🔗 Helpful Links

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

📄 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.

✅ No Failures

As of commit 5d08a70 with merge base 39c5048 ():
💚 Looks good so far! There are no failures yet. 💚

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

alexbaden · 2024-10-08T00:05:24Z

@pytorchbot label "topic: not user facing"

jansel · 2024-10-10T04:04:06Z

Tests are failing, you can't import triton in global scope since PyTorch needs to run without it installed.

alexbaden · 2024-10-11T00:22:25Z

@jansel I updated this PR to leave the branch in case Triton is not installed but to remove deprecated attributes and naming.

alexbaden · 2024-10-11T11:37:10Z

Pushed a fix for the lint error, all other tests passed.

alexbaden · 2024-10-11T19:34:31Z

@jansel I do not think the failing test is related to this PR. On the previous commit (prior to the lint fix) the result was:

PASS: benchmark ('add_loop_inductor_gpu', 'compile_time_instruction_count') pass, actual result 22443112811 +1.23% is within expected 22171041650 ±1.50%

but on the lint fix commit we're getting

REGRESSION: benchmark ('add_loop_inductor_gpu', 'compile_time_instruction_count') failed, actual result 22510259893 is 1.53% higher than expected 22171041650 ±+1.50% if this is an expected regression, please update the expected results.

just slightly above expected. It looks like a few recent commits on main have similar results (right around 1.50% with some just barely exceeding the threshold). What do you think?

jansel · 2024-10-11T20:28:13Z

@pytorchbot rebase

jansel · 2024-10-11T20:28:26Z

Let's see if a rebase fixes that test failure.

pytorchmergebot · 2024-10-11T20:29:49Z

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

…scriptor

pytorchmergebot · 2024-10-11T20:29:53Z

Successfully rebased alex/remove_legacy_attr_descriptor onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout alex/remove_legacy_attr_descriptor && git pull --rebase)

jansel · 2024-10-11T21:54:09Z

It looks like the regression did not go away. Is there any reason why this would make compile time worse?

We could also update the expected time for the test.

alexbaden · 2024-10-12T14:39:00Z

I looked last night and the failure was still there - but today it looks like it went away? Or am I misreading the latest run?

jansel · 2024-10-14T20:12:43Z

Not sure what happened there...

jansel · 2024-10-14T20:12:52Z

@pytorchbot merge

pytorchmergebot · 2024-10-14T20:15:10Z

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 · 2024-10-14T20:21:34Z

This PR (#137458) was merged in 39d21ed but it is still open, likely due to a Github bug, so mergebot is closing it manually. If you think this is a mistake, please feel free to reopen and contact Dev Infra.

@jansel

…ytorch#137458) The `AttrsDescriptor` class has been present in Triton for almost a year now (introduced [here](triton-lang/triton@72c9833)), so we should be able to rely on it existing. I am in the process of supporting the new `AttrsDescriptor` class and @jansel suggested I split changes to the existing class out separately to make sure nothing breaks removing the legacy attribute descriptor attributes. Initially I attempted to remove the branching around detecting whether `AttrsDescriptor` exists but that breaks because PyTorch must build without Triton. So, I went back and updated for the naming introduced in the commit linked above, and also removed two unused attributes `divisible_by_8` and `ids_to_fold` which were removed in Feb 2024 (triton-lang/triton#3122 and triton-lang/triton#3080 respectively). With these changes only the internal workings of the `AttrsDescriptor` class will differ between supported Triton versions, but the data stored will remain consistent. Pull Request resolved: pytorch#137458 Approved by: https://github.com/jansel

The Triton `AttrsDescriptor` object was refactored in triton-lang/triton#4734. These changes add support for the new `AttrsDescriptor` while maintaining backwards compatibility with the existing version. The main changes are different names for the initialized of the descriptor parameters, and a creation via a static method instead of the class constructor. Depends on #137458 which removes some unused logic around the old descriptor. Those changes make this PR cleaner, but if for some reason that old logic is still used I can make adjustments. Use of the new `AttrsDescriptor` depends on triton-lang/triton#4888 Pull Request resolved: #137757 Approved by: https://github.com/jansel

@jansel

…137458) The `AttrsDescriptor` class has been present in Triton for almost a year now (introduced [here](triton-lang/triton@72c9833)), so we should be able to rely on it existing. I am in the process of supporting the new `AttrsDescriptor` class and @jansel suggested I split changes to the existing class out separately to make sure nothing breaks removing the legacy attribute descriptor attributes. Initially I attempted to remove the branching around detecting whether `AttrsDescriptor` exists but that breaks because PyTorch must build without Triton. So, I went back and updated for the naming introduced in the commit linked above, and also removed two unused attributes `divisible_by_8` and `ids_to_fold` which were removed in Feb 2024 (triton-lang/triton#3122 and triton-lang/triton#3080 respectively). With these changes only the internal workings of the `AttrsDescriptor` class will differ between supported Triton versions, but the data stored will remain consistent. Pull Request resolved: #137458 Approved by: https://github.com/jansel

The Triton `AttrsDescriptor` object was refactored in triton-lang/triton#4734. These changes add support for the new `AttrsDescriptor` while maintaining backwards compatibility with the existing version. The main changes are different names for the initialized of the descriptor parameters, and a creation via a static method instead of the class constructor. Depends on #137458 which removes some unused logic around the old descriptor. Those changes make this PR cleaner, but if for some reason that old logic is still used I can make adjustments. Use of the new `AttrsDescriptor` depends on triton-lang/triton#4888 Pull Request resolved: #137757 Approved by: https://github.com/jansel

pytorch-bot bot added the module: inductor label Oct 8, 2024

pytorch-bot bot added the topic: not user facing topic category label Oct 8, 2024

pytorchbot added the open source label Oct 8, 2024

colesbury requested a review from eellison October 8, 2024 03:26

colesbury added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Oct 8, 2024

alexbaden force-pushed the alex/remove_legacy_attr_descriptor branch from 74a8e7b to 27e91e2 Compare October 11, 2024 00:16

alexbaden changed the title ~~[Inductor] Expect Triton-provided AttrDescriptor~~ [Inductor] Update AttrDescriptor instantiation for Triton changes Oct 11, 2024

alexbaden changed the title ~~[Inductor] Update AttrDescriptor instantiation for Triton changes~~ [Inductor] Update AttrsDescriptor instantiation for Triton changes Oct 11, 2024

alexbaden mentioned this pull request Oct 11, 2024

[Inductor]: Support updated Triton AttrsDescriptor #137757

Closed

jansel approved these changes Oct 11, 2024

View reviewed changes

alexbaden added 3 commits October 11, 2024 20:29

[Inductor]: Remove unused Triton AttrsDescriptor attributes

fbc1df1

[Inductor] Replace deprecated instance_descriptor name with AttrsDe…

cbb09ad

…scriptor

Fix lint

5d08a70

pytorchmergebot force-pushed the alex/remove_legacy_attr_descriptor branch from bb0d8ad to 5d08a70 Compare October 11, 2024 20:29

jansel added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 11, 2024

pytorchmergebot added the merging label Oct 14, 2024

pytorchmergebot added the Merged label Oct 14, 2024

pytorchmergebot closed this Oct 14, 2024

pytorchmergebot removed the merging label Oct 14, 2024

[Inductor] Update AttrsDescriptor instantiation for Triton changes #137458

[Inductor] Update AttrsDescriptor instantiation for Triton changes #137458

Uh oh!

Conversation

alexbaden commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Oct 8, 2024 • 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/137458

✅ No Failures

Uh oh!

alexbaden commented Oct 8, 2024

Uh oh!

jansel commented Oct 10, 2024

Uh oh!

alexbaden commented Oct 11, 2024

Uh oh!

alexbaden commented Oct 11, 2024

Uh oh!

alexbaden commented Oct 11, 2024

Uh oh!

jansel commented Oct 11, 2024

Uh oh!

jansel commented Oct 11, 2024

Uh oh!

pytorchmergebot commented Oct 11, 2024

Uh oh!

pytorchmergebot commented Oct 11, 2024

Uh oh!

jansel commented Oct 11, 2024

Uh oh!

alexbaden commented Oct 12, 2024

Uh oh!

jansel commented Oct 14, 2024

Uh oh!

jansel commented Oct 14, 2024

Uh oh!

pytorchmergebot commented Oct 14, 2024

Merge started

Uh oh!

pytorchmergebot commented Oct 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alexbaden commented Oct 8, 2024 •

edited

Loading

pytorch-bot bot commented Oct 8, 2024 •

edited

Loading