🔗 Helpful Links

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

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

❌ 3 New Failures, 3 Cancelled Jobs

As of commit 93ea684 with merge base b16f020 ():

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

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

Why? We already have fbgemm as a submodule and a dependency?
Why would we copy code from it as well?

Why? We already have fbgemm as a submodule and a dependency? Why would we copy code from it as well?

The code we want is not compiled by default from fbgemm under its experimental sources. Though we could change how we build fbgemm as a submodule, there is a cutlass implementation in pytorch today for rowwise f8 gemm but not a CK [ROCm] implementation, but the 'experimental' CK one in fbgemm was stable enough to port to pytorch so that we can have parity.

Hi, @drisspg @atalman : Any plan to land this PR? We have other work items depending on it.

We have to be more and more careful with dependency management to stop major issues we have around release and packaging.
While I completely understand that copy pasting the piece of code you want and manually modifying it is the most straighforward way to get this done, I don't think it is a good solution for PyTorch mid term.
We should either:

• Update the fbgemm repo to be able to pull in what we need properly as a submodule.
• Remove the fbgemm submodule altogether if it doesn't work and copy paste the few kernels we care about.

Having both at the same time sounds like a recipe for disaster down the road both for maintenance and binary conflict reasons.

We have to be more and more careful with dependency management to stop major issues we have around release and packaging. While I completely understand that copy pasting the piece of code you want and manually modifying it is the most straighforward way to get this done, I don't think it is a good solution for PyTorch mid term. We should either:

• Update the fbgemm repo to be able to pull in what we need properly as a submodule.
• Remove the fbgemm submodule altogether if it doesn't work and copy paste the few kernels we care about.

Having both at the same time sounds like a recipe for disaster down the road both for maintenance and binary conflict reasons.

Thanks for the comment. We will evaluate this further.

We should either:

• Update the fbgemm repo to be able to pull in what we need properly as a submodule.
• Remove the fbgemm submodule altogether if it doesn't work and copy paste the few kernels we care about.

Having both at the same time sounds like a recipe for disaster down the road both for maintenance and binary conflict reasons.

Third option. Why don't we instead consider this a migration of this CK implementation that is under the experimental portion of fbgemm into pytorch and deprecate the fbgemm experimental? Consider the feature graduated?

@albanD any opinion on my third option above?

Sure but then I would have quite a few questions from a quick look at the code from the point of view of it living in core:

• Is it actually used in this PR?
• Is this code tested?
• Is this really just about doing template instantiation for every single shape used in a specific llama version? If so what's the impact on binary size, what is the plan for future model releases, why llama and not others?

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.