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

feat: E4M3fnuz FP8 format added #281

Closed
wants to merge 5 commits into from

Conversation

maktukmak
Copy link
Contributor

Currently, the E4M3 fP8 format implemented is ARM-Intel-Nvidia style. However, there is another style, IEEE 754 (torch name is float8_e4m3fnuz), which has different bit configuration and min-max values. This pull request aims to incorporate this style. The unit tests currently pass on CPU because it supports both styles. However, they will fail when tested on other devices. I need guidance on how to design the tests so that they only run a specific style based on the device. Once I have this information, I can complete this PR.

@maktukmak maktukmak requested a review from dacorvo as a code owner August 15, 2024 21:44
@dacorvo
Copy link
Collaborator

dacorvo commented Aug 20, 2024

@maktukmak thank you for this pull-request.
As a first comment, please amend your commit to make it conventional (feat: e4m3fnuz added). This is a bit tedious but I want people to use meaningful commit messages and the simplest way was to use a predefined CI workflow that enforces conventional commit messages (I may improve this in the future to make it more flexible).
To exclude e4m3fnuz tests for a specific device, you have two options:

  • duplicate the tests for these types and use the pytest.mark.skip_device decorator,
  • inside each test, start by testing the device and qtype, and skip the test explicitly with pytest.skip.
    You will find examples of both solutions in the existing test files (the first solution is used to skip float8 tests for MPS, and the second for tinygemm if CUDA version is less than 2.1).

@maktukmak
Copy link
Contributor Author

maktukmak commented Aug 20, 2024

@dacorvo, I excluded CUDA for e4m3fnuz in tests using the second option, and changed the commit names.

@maktukmak maktukmak changed the title [Draft] E4M3fnuz FP8 format added E4M3fnuz FP8 format added Aug 22, 2024
@maktukmak maktukmak changed the title E4M3fnuz FP8 format added feat: E4M3fnuz FP8 format added Aug 22, 2024
Copy link

This PR is stale because it has been open 15 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Sep 13, 2024
@maktukmak
Copy link
Contributor Author

@dacorvo , I fixed the style so it may pass the checks now.

@github-actions github-actions bot removed the Stale label Sep 14, 2024
Copy link
Collaborator

@dacorvo dacorvo left a comment

Choose a reason for hiding this comment

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

Thanks for this pull-request !

@dacorvo dacorvo mentioned this pull request Sep 17, 2024
@dacorvo
Copy link
Collaborator

dacorvo commented Sep 17, 2024

Rebased and merged as #310

@dacorvo dacorvo closed this Sep 17, 2024
Sign up for free to 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.

2 participants