This repository has been archived by the owner on Aug 7, 2024. It is now read-only.
make all 3 gemms in Float8Linear support configurability, not user facing #315
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: not ready for review yet Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
vkuzo
added a commit
that referenced
this pull request
Jul 12, 2024
Summary: not ready for review yet Test Plan: Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 302e7c443595b5bb65acb27f3fcca82ccf664a6d Pull Request resolved: #315
facebook-github-bot
added
the
CLA Signed
Summary: not ready for review yet Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
vkuzo
added a commit
that referenced
this pull request
Jul 15, 2024
Summary: not ready for review yet Test Plan: Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 1ef7943cbe21517d40975c69d0be4a719c7bf20d Pull Request resolved: #315
drisspg
reviewed
Jul 19, 2024
| inpt_tensor: torch.Tensor, | ||
| linear_mm_config: LinearMMConfig, | ||
| reduce_amax: bool = False, | ||
| gemm_input_role: GemmInputRole = GemmInputRole.X, |
There was a problem hiding this comment.
maybe no default value for gemm role?
There was a problem hiding this comment.
we can clean up in a separate PR, there is extra complexity because we'd need to change the argument order
drisspg
reviewed
Jul 19, 2024
| self.backward_config = ScaledMMConfig( | ||
| emulate, False, False, config.pad_inner_dim | ||
| # TODO(future): user level configuration of gemms | ||
| self.linear_mm_config = LinearMMConfig( |
There was a problem hiding this comment.
[I think this might be another stylistic thing so no need to change]:
I think I would actually make this a func and then super document it. Its not very clear reading this what everything does so I would clearly explain in that func the exact recipe that we choose by default
There was a problem hiding this comment.
this isn't user facing, so we can clean up at any time
drisspg
reviewed
Jul 19, 2024
| @@ -76,6 +84,7 @@ def float8_cat(aten_op, args, kwargs=None): | |||
| scale = chunked_tensors[0]._scale | |||
There was a problem hiding this comment.
Future PR:
We should code share between unflatten/flatten and here to just splat out the extra metadata that lives on the tensor
drisspg
reviewed
Jul 19, 2024
drisspg
reviewed
Jul 19, 2024
Summary: not ready for review yet Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
vkuzo
added a commit
that referenced
this pull request
Jul 19, 2024
Summary: not ready for review yet Test Plan: Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 74ca77e254806922b7dd4d078cd1a8b9f9c74f6d Pull Request resolved: #315
Summary: not ready for review yet Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
vkuzo
added a commit
that referenced
this pull request
Jul 19, 2024
Summary: not ready for review yet Test Plan: Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: ebffcec426b76fba656a881d9a43397499179773 Pull Request resolved: #315
vkuzo
changed the title
drisspg
approved these changes
Jul 19, 2024
…not user facing" Summary: This PR adds some plumbing for how to eventually make all 3 gemms in a linear fwd/bwd configurable: 1. add `LinearMMConfig` to `Float8Tensor` to tie together the three `ScaledMMConfig` objects, one per gemm 2. add `GemmInputRole` to `Float8Tensor` to specify how to pick the right config 3. plumb all of these throughout the codebase Note that none of this is user facing, and there is no logic change. Planned follow-ups: * a future PR will make the per-gemm behavior configurable in a user facing way, which will hook up to the objects introduced in this PR * a future PR will update the naming from x/w/dL_dY to input/weight/grad_output throughout the codebase Test Plan: ``` ./test/test_everything.sh ``` Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
vkuzo
added a commit
that referenced
this pull request
Jul 19, 2024
Summary: not ready for review yet Test Plan: Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 39cb928167b6e6be7e754569363f634f28f5472a Pull Request resolved: #315
|
@vkuzo has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
This pull request has been merged in c58fb5d. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stack from ghstack (oldest at bottom):
Summary:
This PR adds some plumbing for how to eventually make all 3 gemms in a linear fwd/bwd configurable:
LinearMMConfigtoFloat8Tensorto tie together the threeScaledMMConfigobjects, one per gemmGemmInputRoletoFloat8Tensorto specify how to pick the right configNote that none of this is user facing, and there is no logic change. Planned follow-ups:
Test Plan:
Reviewers:
Subscribers:
Tasks:
Tags:
Differential Revision: D59973551