Header bug fix #1079

metascroy · 2024-10-15T01:47:19Z

Summary: A last minute change created a compile error on the header. This fixes the issue.

Differential Revision: D64370707

pytorch-bot · 2024-10-15T01:47:22Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR

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

✅ No Failures

As of commit 610f80d with merge base 48bc81c ():
💚 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 · 2024-10-15T01:47:26Z

This pull request was exported from Phabricator. Differential Revision: D64370707

Summary: A last minute change created a compile error on the header. This fixes the issue. I also make the header 64 bytes and add a magic number at the start to make it safer in future. Differential Revision: D64370707

facebook-github-bot · 2024-10-15T02:52:33Z

This pull request was exported from Phabricator. Differential Revision: D64370707

malfet · 2024-10-15T03:09:15Z

I'm unsure what it fixes to be frank, perhaps better PR description would help clarify what it is trying to fix...

malfet · 2024-10-15T03:09:42Z

torchao/experimental/ops/linear_8bit_act_xbit_weight/packed_weights_header.h

-  TORCHAO_CHECK(
-      version >= 0 && version < 256, "version must be between 0 and 255");


What is wrong with this check?

Before it existed to make sure version could be packed into a char (since both version/weight_nbit were packed into an unsigned short). But the header now has more space, and version is now an int, so this bound check is no longer needed.

malfet · 2024-10-15T03:11:11Z

torchao/experimental/ops/packed_weights_header.h

@@ -10,44 +10,47 @@
 #include <cassert>
 namespace torchao::ops {

-enum PackedWeightsFormat : unsigned short {
+enum PackedWeightsFormat : int {


Why this must be a signed type? unsigned int sounds like a better idea. Also enum inside the namespace is oxymoron, please use enum class

Suggested change

enum PackedWeightsFormat : int {

enum class PackedWeightsFormat : uint32_t {

malfet · 2024-10-15T03:12:16Z

torchao/experimental/ops/packed_weights_header.h

-    static_assert(sizeof(format) + sizeof(params) == 16);
-    return 16;
+    static_assert(sizeof(magic) + sizeof(format) + sizeof(params) == 64);
+    return 64;


Why not use sizeof(XYZ)?

Suggested change

return 64;

return sizeof(PackedWeightsHeader);

I thought this made it clearer that the header is 64 bytes at a glance. I also checked and sizeof(PackedWeightsHeader) = 60, but the serialized size is still 64 bytes (I think because the static magic number doesn't count toward the size).

malfet · 2024-10-15T03:12:51Z

torchao/experimental/ops/packed_weights_header.h

  }

  inline void write(void* packed_weights) const {
-    auto header = (unsigned short*)(packed_weights);
-    header[0] = (unsigned short)format;
+    auto header = (int*)(packed_weights);


In C++ better used C++ cast-style ops

Suggested change

auto header = (int*)(packed_weights);

auto header = reinterpret_cast<int*>(packed_weights);

Summary: A last minute change created a compile error on the header. This fixes the issue. I also make the header 64 bytes and add a magic number at the start to make it safer in future. Differential Revision: D64370707

facebook-github-bot · 2024-10-15T16:07:39Z

This pull request was exported from Phabricator. Differential Revision: D64370707

Summary: A last minute change created a compile error on the header. This fixes the issue. I also make the header 64 bytes and add a magic number at the start to make it safer in future. Differential Revision: D64370707

facebook-github-bot · 2024-10-15T16:14:14Z

This pull request was exported from Phabricator. Differential Revision: D64370707

Summary: A last minute change created a compile error on the header. This fixes the issue. I also make the header 64 bytes and add a magic number at the start to make it safer in future. Differential Revision: D64370707

facebook-github-bot · 2024-10-15T16:18:47Z

This pull request was exported from Phabricator. Differential Revision: D64370707

Summary: Pull Request resolved: pytorch#1079 A last minute change created a compile error on the header. This fixes the issue. I also make the header 64 bytes and add a magic number at the start to make it safer in future. Differential Revision: D64370707

digantdesai · 2024-10-15T22:07:50Z

Address Nikita's comments before landing, stamping to unblock if you have already done so.

…at/ folder (pytorch#1076) * [Hackability Refactor] Move known_model_params under torchchat (pytorch#1073) * [Hackability Refactor] Migrate CLI call sites to explicitly go through torchchat.py (pytorch#1075) * [Hackability Refactor] Move model.py underneath torchchat/ (pytorch#1077) * Move model.py * Clear out init to avoid package circular import * [Hackability Refactor] Move select top level docs into folders within torchchat (pytorch#1080) * [Hackability Refactor] Move the top level util folder into torchchat/utils (pytorch#1079) * [Hackability Refactor] Move the top level util file into torchchat/utils/ * Cleared out init to avoid packing * [Hackability Refactor] Collapse gguf_util into gguf_loader (pytorch#1078) * [Hackability Refactor] Collapse gguf_util into gguf_loader * Update bad import * [Hackability Refactor] Move model_config into torchchat/model_config (pytorch#1082) * [Hackability Refactor] Move cli related files under torchchat/cli (pytorch#1083) * [Hackability Refactor] Move build/util into torchchat/utils (pytorch#1084) * [Hackability Refactor] Easy Moves: eval, gguf_loader, quantize, model_dist (pytorch#1085) * [Hackability Refactor] Easy Cheap Moves: eval, gguf_loader, quantize, model_dist * Update eval.py call sites that slipped through the initial pass * [Hackability Refactor] Update missed direct file calls to use torchchat.py (pytorch#1088) * [Hackability Refactor] Move export and generate under torchchat/ (pytorch#1089) * [Hackability Refactor] Move scripts under torchchat/utils (pytorch#1090) * [Hackability Refactor] Move scripts under torchchat/utils * Fix install script for AOTI * Update referenced path in build_android * Adding missing utils path * Add another layer for torchchat * Move the source command depending on if TC root is defined * [Hackability Refactor] Move installation related files into install/ (pytorch#1081) * [Hackability Refactor] Move installation related files into install/ * Fix install req path * Test fix with install path for bash * Debug messages * Remove changes to install in et_python_libs * Remove debug echo * Fix pin path for et * [Hackability Refactor] Restricted Lint (pytorch#1091) * [Hackability Refactor] Removing __main__ from export/generate/eval (pytorch#1092)

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 Oct 15, 2024

facebook-github-bot added the fb-exported label Oct 15, 2024

metascroy force-pushed the export-D64370707 branch from c8a2749 to abb7810 Compare October 15, 2024 02:52

malfet reviewed Oct 15, 2024

View reviewed changes

metascroy force-pushed the export-D64370707 branch from abb7810 to 04fa11d Compare October 15, 2024 16:07

metascroy force-pushed the export-D64370707 branch from 04fa11d to 0a93668 Compare October 15, 2024 16:13

Header bug fix (pytorch#1079)

610f80d

Summary: A last minute change created a compile error on the header. This fixes the issue. I also make the header 64 bytes and add a magic number at the start to make it safer in future. Differential Revision: D64370707

metascroy force-pushed the export-D64370707 branch from 0a93668 to 610f80d Compare October 15, 2024 16:18

digantdesai approved these changes Oct 15, 2024

View reviewed changes

facebook-github-bot merged commit 6ea36c5 into pytorch:main Oct 15, 2024
18 of 19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Header bug fix #1079

Header bug fix #1079

metascroy commented Oct 15, 2024

pytorch-bot bot commented Oct 15, 2024 •

edited

Loading

facebook-github-bot commented Oct 15, 2024

facebook-github-bot commented Oct 15, 2024

malfet commented Oct 15, 2024

malfet Oct 15, 2024

metascroy Oct 15, 2024

malfet Oct 15, 2024

malfet Oct 15, 2024

metascroy Oct 15, 2024 •

edited

Loading

malfet Oct 15, 2024

facebook-github-bot commented Oct 15, 2024

facebook-github-bot commented Oct 15, 2024

facebook-github-bot commented Oct 15, 2024

digantdesai commented Oct 15, 2024

		TORCHAO_CHECK(
		version >= 0 && version < 256, "version must be between 0 and 255");

	enum PackedWeightsFormat : int {
	enum class PackedWeightsFormat : uint32_t {

	auto header = (int*)(packed_weights);
	auto header = reinterpret_cast<int*>(packed_weights);

Header bug fix #1079

Header bug fix #1079

Conversation

metascroy commented Oct 15, 2024

pytorch-bot bot commented Oct 15, 2024 • edited Loading

🔗 Helpful Links

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

✅ No Failures

facebook-github-bot commented Oct 15, 2024

facebook-github-bot commented Oct 15, 2024

malfet commented Oct 15, 2024

malfet Oct 15, 2024

Choose a reason for hiding this comment

metascroy Oct 15, 2024

Choose a reason for hiding this comment

malfet Oct 15, 2024

Choose a reason for hiding this comment

malfet Oct 15, 2024

Choose a reason for hiding this comment

metascroy Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

malfet Oct 15, 2024

Choose a reason for hiding this comment

facebook-github-bot commented Oct 15, 2024

facebook-github-bot commented Oct 15, 2024

facebook-github-bot commented Oct 15, 2024

digantdesai commented Oct 15, 2024

pytorch-bot bot commented Oct 15, 2024 •

edited

Loading

metascroy Oct 15, 2024 •

edited

Loading