Skip to content

[PyTorch] Adjust checkpointing of FP8 metadata for attention - #917

Merged
cyanguwa merged 15 commits into
NVIDIA:mainfrom
cyanguwa:basemodule_dpa
Jun 15, 2024
Merged

[PyTorch] Adjust checkpointing of FP8 metadata for attention#917
cyanguwa merged 15 commits into
NVIDIA:mainfrom
cyanguwa:basemodule_dpa

Conversation

@cyanguwa

@cyanguwacyanguwa commented Jun 12, 2024

Copy link
Copy Markdown
Collaborator

Description

This PR relocates the FP8 metadata for attention from FusedAttention to DotProductAttention. It makes DotProductAttention a TransformerEngineBaseModule and FusedAttention a torch.nn.module. In the future, core_attention._extra_state will be the centralized place for FP8 metadata for any attention backend, instead of core_attention.fused_attention._extra_state which was just for FusedAttention (originated from #768 ).

Type of change

  • Documentation change (change only to the documentation, either a fix or a new content)
  • Bug fix (non-breaking change which fixes an issue)
  • [ x] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Changes

Please list the changes introduced in this PR:

  • subclassed DotProductAttention to TransformerEngineBaseModule
  • reverted FusedAttention to torch.nn.module
  • passed fp8_meta from DotProductAttention to FusedAttention
  • added miscellaneous warnings and tests

Checklist:

  • [x ] I have read and followed the contributing guidelines
  • [x ] The functionality is complete
  • [x ] I have commented my code, particularly in hard-to-understand areas
  • [x ] I have made corresponding changes to the documentation
  • [x ] My changes generate no new warnings
  • [x ] I have added tests that prove my fix is effective or that my feature works
  • [x ] New and existing unit tests pass locally with my changes

Signed-off-by: Charlene Yang <8636796+cyanguwa@users.noreply.github.com>
Signed-off-by: Charlene Yang <8636796+cyanguwa@users.noreply.github.com>
Signed-off-by: Charlene Yang <8636796+cyanguwa@users.noreply.github.com>
Signed-off-by: Charlene Yang <8636796+cyanguwa@users.noreply.github.com>
Signed-off-by: Charlene Yang <8636796+cyanguwa@users.noreply.github.com>
Signed-off-by: Charlene Yang <8636796+cyanguwa@users.noreply.github.com>
Signed-off-by: Charlene Yang <8636796+cyanguwa@users.noreply.github.com>
@cyanguwacyanguwa changed the title [PyTorch] Store FusedAttention._extra_state to DotProductAttention._extra_state when checkpointing[PyTorch] Store FusedAttention's extra_state to DotProductAttention's extra_state when checkpointingJun 12, 2024
@cyanguwa

Copy link
Copy Markdown
CollaboratorAuthor

/te-ci pytorch

@cyanguwa
cyanguwa requested a review from ksivamanJune 12, 2024 01:56
…ction; test FP8 for sm90+
Signed-off-by: Charlene Yang <8636796+cyanguwa@users.noreply.github.com>
@cyanguwa

Copy link
Copy Markdown
CollaboratorAuthor

/te-ci pytorch

@cyanguwa
cyanguwa requested a review from timmoon10June 13, 2024 00:46

@timmoon10timmoon10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this approach is hacky. It adds an unnecessary fp8_meta to DotProductAttention and it intertwines the implementations of DotProductAttention and FusedAttention. I think the right solution is to refactor FusedAttention so it's not a subclass of TransformerEngineBaseModule. That way the fp8_meta lives in DotProductAttention, which passes it to FusedAttention.

I won't block since we want to get a bugfix before the release. It doesn't affect the checkpoint format, so we can fix the implementation in the future.

Comment threadtransformer_engine/pytorch/module/base.py Outdated
Comment threadtransformer_engine/pytorch/attention.py Outdated
Comment threadtransformer_engine/pytorch/attention.py Outdated
Comment threadtests/pytorch/test_sanity.py
Comment threadtransformer_engine/pytorch/module/base.py Outdated
Comment threadtests/pytorch/test_sanity.py
Comment threadtests/pytorch/test_sanity.py
Comment threadtests/pytorch/test_sanity.py Outdated
cyanguwaand others added 5 commits June 14, 2024 14:54
@cyanguwa

Copy link
Copy Markdown
CollaboratorAuthor

/te-ci pytorch

@timmoon10timmoon10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@cyanguwacyanguwa changed the title [PyTorch] Store FusedAttention's extra_state to DotProductAttention's extra_state when checkpointing[PyTorch] Adjust storage of FP8 metadata for attention in checkpointingJun 15, 2024
@cyanguwacyanguwa changed the title [PyTorch] Adjust storage of FP8 metadata for attention in checkpointing[PyTorch] Adjust checkpointing of FP8 metadata for attentionJun 15, 2024
@cyanguwa
cyanguwa merged commit fe5aa60 into NVIDIA:mainJun 15, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@cyanguwa@timmoon10@ksivaman