Skip to content

Conversation

@shiyang-weng
Copy link
Contributor

int8 scaled_embedding_bag reverted by #2974
On #2972, they shared error reason related to the unused qtype.

We re-enable int8 scaled_embedding_bag and fix the unused variables issue on this PR

@pytorch-bot
Copy link

pytorch-bot bot commented Sep 25, 2025

🔗 Helpful Links

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

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

✅ No Failures

As of commit 02a1bc4 with merge base 0d3217d (image):
💚 Looks good so far! There are no failures yet. 💚

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

@meta-cla meta-cla 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 25, 2025
@shiyang-weng shiyang-weng marked this pull request as draft September 25, 2025 02:41
@Xia-Weiwen Xia-Weiwen changed the title [CPU] Support int8 scaled embedding bag [Reland][CPU] Support int8 scaled embedding bag Sep 25, 2025
@pytorch-bot pytorch-bot bot added the ci-no-td label Sep 25, 2025
@Xia-Weiwen Xia-Weiwen added the topic: not user facing Use this tag if you don't want this PR to show up in release notes label Sep 25, 2025
@Xia-Weiwen Xia-Weiwen self-requested a review September 25, 2025 03:10
@shiyang-weng shiyang-weng force-pushed the wengshiy/int8_scaled_embedding_bag branch from ac75d21 to e846df7 Compare September 25, 2025 05:18
}

} // namespace torchao
} // namespace torchao
Copy link
Collaborator

Choose a reason for hiding this comment

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

unnecessary change?

@Xia-Weiwen
Copy link
Collaborator

CC @mingfeima for review. Thanks.

Comment on lines +1126 to +1127
# Next setp: support more out_dtype
out_dtype = torch.float32
Copy link
Contributor

Choose a reason for hiding this comment

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

should this arg be exposed to the op as well in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should this arg be exposed to the op as well in the future?

Yes

Copy link
Contributor

@jerryzh168 jerryzh168 left a comment

Choose a reason for hiding this comment

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

stamping, please make sure CI passes

@Xia-Weiwen
Copy link
Collaborator

Hi @mingfeima Could you please review this PR? Thanks.

#endif

template <typename index_t>
template <typename index_t, typename data_t>

Choose a reason for hiding this comment

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

this is not mandatory, but you can use block_dim as an template argument and make this function simpler.

template <typename index_t, typename scalar_t, int block_dim>

@mingfeima
Copy link

one more thing, it would be better to refactor the fp8 conversition simd code with https://github.com/pytorch/ao/blob/main/torchao/csrc/cpu/aten_kernels/float8_linear.cpp, maybe put them in to vec util.h.

@shiyang-weng shiyang-weng marked this pull request as ready for review September 29, 2025 07:06
@Xia-Weiwen Xia-Weiwen merged commit 4a03494 into pytorch:main Sep 29, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-no-td CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. topic: not user facing Use this tag if you don't want this PR to show up in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants