Skip to content
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

[Float8] Make Inference and Training code independent #808

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

drisspg
Copy link
Contributor

@drisspg drisspg commented Sep 4, 2024

Stacked PRs:


Summary

Remove Old FP8 inference flow and and switch over to Float8MMConfig
in favor of the newly added support to AQT + quantize_ apis. It also completely separates and utilization on training code by creating a new Float8MMConfig and a addmm wrapper in the inference.py file

Copy link

pytorch-bot bot commented Sep 4, 2024

🔗 Helpful Links

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

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

✅ No Failures

As of commit c5c333e with merge base f5703b0 (image):
💚 Looks good so far! There are no failures yet. 💚

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

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 4, 2024
@drisspg drisspg requested a review from vkuzo September 4, 2024 19:21
@drisspg drisspg changed the title [Float8Configs] Make named tuples have better docs + public [Float8Configs] Remove Old FP8 inference flow and and switch over to Float8MMCOnfig Sep 4, 2024
@drisspg drisspg changed the title [Float8Configs] Remove Old FP8 inference flow and and switch over to Float8MMCOnfig [Float8Configs] Remove Old FP8 inference flow and and switch over to Float8MMConfig Sep 4, 2024
@drisspg drisspg changed the title [Float8Configs] Remove Old FP8 inference flow and and switch over to Float8MMConfig [Float8Configs] Make named tuples have better docs + public Sep 4, 2024
@drisspg drisspg requested a review from vkuzo September 4, 2024 20:46
@drisspg drisspg changed the title [Float8Configs] Make named tuples have better docs + public [Float8] Make Inference and Training code independent Sep 4, 2024

Note:
If the input tensor is already in Float8 format, it is returned as is without re-casting.
class Float8MMConfig(NamedTuple):
Copy link
Contributor

Choose a reason for hiding this comment

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

can we make the names ScaledMMConfig and Float8MMConfig not be confusing? Ideally it should be clear why there are two objects and in which context the user should use which object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I was going to do ScaledMMConfigInference but that is even too verbose for me, any suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

for training I went with a user facing Float8GemmConfig which is a dataclass and easy to explain (IMO), the way the data gets passed around is not user facing so details of ScaledMMConfig don't matter as much

tbh making ScaledMMConfig public and reusable between training + inference sounds right to me, if you really want this to be public without a dataclass wrapper

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can not be a dataclass today because that wont work with compile, Lazos has a PR to make frozen dataclasses proxyable but until then it has to be a named tuple.

I love dataclasses but I dont understand why they are more understandable then a namedtuple, Is the problem that this name is similar to other names?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's less about dataclass versus named tuple, and more about the field readability / understandability / future proofness, etc. For example, if we made ScaledMMConfig have an output dtype enum instead of a boolean and ensured all the other args are consistent with other public APIs, I think that would be fine.

https://stackoverflow.com/a/18348004/1058521 is one minor reason, default values look nicer with dataclasses

stack-info: PR: #808, branch: drisspg/stack/9
@drisspg drisspg merged commit 848e123 into main Sep 4, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants