[export] add translations for SymInt/Bool deserialization; FloorDiv #136802

pianpwk · 2024-09-26T23:06:55Z

Summary:
fix for #136797

We have an issue with deserialization for symbolic expressions, where the original expression has a structure mismatch with the deserialized expression. This happens because we serialize the expressions into strings, and deserialize using sympy.sympify().

This seems to cause issues with custom torch sympy.Functions, in this case FloorDiv. In this case, the expression FloorDiv(a, b) turns into sympy.floor(a, PowByNatural(b, -1)), or sympy.floor(sympy.Mul(*a), PowByNatural(b, -1)) if a is also a sympy.Mul. This seems to break downstream when we call torch.empty_strided() on this expression, and the PowByNatural has an invalid ValueRange (e.g. [0, -1]).

This fixes that by adding translations after deserialization, to pattern match and transform expressions into the original format. Other potential strategies are a) we could also serialize SymInts as sympy.Expr structures, but that seems a lot more involved and BC breaking, or b) we could modify sympy.sympify(?), but I don't know if this is possible.

Test Plan: test_export/test_serialize

Differential Revision: D63493615

pytorch-bot · 2024-09-26T23:06:59Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

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

❌ 8 New Failures

As of commit b53cc2d with merge base 46f158b ():

NEW FAILURES - The following jobs have failed:

pull / linux-focal-cuda12.1-py3.10-gcc9 / test (default, 5, 5, lf.linux.4xlarge.nvidia.gpu) (gh)
export/test_serdes.py::SerDesExportTestExport::test_dim_dynamic_serdes
pull / linux-focal-cuda12.1-py3.10-gcc9-sm86 / test (default, 5, 5, lf.linux.g5.4xlarge.nvidia.gpu) (gh)
export/test_serdes.py::SerDesExportTestExport::test_dim_dynamic_serdes
pull / linux-focal-py3.11-clang10 / test (default, 4, 4, lf.linux.2xlarge) (gh)
export/test_serdes.py::SerDesExportTestExport::test_dim_dynamic_serdes
pull / linux-focal-py3.12-clang10 / test (default, 4, 4, lf.linux.2xlarge) (gh)
export/test_serdes.py::SerDesExportTestExport::test_dim_dynamic_serdes
pull / linux-focal-py3.12-clang10-experimental-split-build / test (default, 2, 3, linux.2xlarge) (gh)
export/test_serdes.py::SerDesExportTestExport::test_dim_dynamic_serdes
pull / linux-focal-py3.9-clang10 / test (default, 4, 4, lf.linux.2xlarge) (gh)
export/test_serdes.py::SerDesExportTestExport::test_dim_dynamic_serdes
pull / linux-jammy-py3.10-clang15-asan / test (default, 5, 6, lf.linux.4xlarge) (gh)
export/test_serdes.py::SerDesExportTestExport::test_dim_dynamic_serdes
pull / linux-jammy-py3.9-gcc11 / test (default, 4, 4, lf.linux.2xlarge) (gh)
export/test_serdes.py::SerDesExportTestExport::test_dim_dynamic_serdes

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

facebook-github-bot · 2024-09-26T23:07:05Z

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

…136802) Summary: fix for #136797 We have an issue with deserialization for symbolic expressions, where the original expression has a structure mismatch with the deserialized expression. This happens because we serialize the expressions into strings, and deserialize using sympy.sympify(). This seems to cause issues with custom torch sympy.Functions, in this case FloorDiv. In this case, this alters the structure from `FloorDiv(a, b)` into `sympy.floor(a, PowByNatural(b, -1))`, or `sympy.floor(sympy.Mul(*a), PowByNatural(b, -1))` if a is also a sympy.Mul. This seems to break downstream when we call torch.empty_strided() on this expression, and the PowByNatural has an invalid ValueRange (e.g. [0, -1]). This fixes that by adding translations after deserialization, to pattern match and transform expressions into the original format. Other potential strategies are a) we could also serialize SymInts as sympy.Expr structures, but that seems a lot more involved and BC breaking, or b) we could modify sympy.sympify(?), but I don't know if this is possible. Test Plan: test_export/test_serialize Differential Revision: D63493615

facebook-github-bot · 2024-09-26T23:48:48Z

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

facebook-github-bot · 2024-09-26T23:49:49Z

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

pianpwk · 2024-09-27T16:36:17Z

@ezyang would you know of a better way to do this? It seems like even if we let the invalid ValueRange pass or flip min/max, PowByNatural + ValueRanges is the thing to avoid, because it still means we lose precision when truncating the range to ints (e.g. [0, -1]).

angelayi · 2024-09-27T19:55:35Z

torch/_export/serde/serialize.py

                                compiler_max=vr.upper,  # type: ignore[arg-type]
                            )

+            sym = _translate_deserialized_sym_expr(sym)


should this line go directly after line 1526?

torch/_export/serde/serialize.py

ezyang · 2024-09-30T02:45:57Z

So, the proper way to fix this is to stop serializing the Sympy string expressions as strings, for which we have NO stability or even round-trippability guarantee. Sympy expressions are supposed to be roundtrippable to FX IR, and back, via the Python reference / sympy interpreter stuff (there are some bugs / incompleteness, but we should fix those anyway.)

But I'm guessing you didn't want to hear that? Lol.

pianpwk · 2024-09-30T17:05:37Z

So, the proper way to fix this is to stop serializing the Sympy string expressions as strings, for which we have NO stability or even round-trippability guarantee. Sympy expressions are supposed to be roundtrippable to FX IR, and back, via the Python reference / sympy interpreter stuff (there are some bugs / incompleteness, but we should fix those anyway.)

But I'm guessing you didn't want to hear that? Lol.

Hahaha no but this makes total sense

ezyang · 2024-09-30T18:21:14Z

If you are dead set on string format, I'd probably still want to have a customized printer and parser. In particular the parser needs to override the mapping of FloorDiv to which class. As a stopgap, we could potentially rename functions in torch.utils._sympy.functions so they don't conflict with Sympy

bhack · 2024-10-01T00:00:21Z

I think we had the same problem with inductor cache right?

site-packages/torch/utils/_sympy/interp.py:159] [0/1] failed while executing pow_by_natural([VR[0, int_oo], VR[-1, -1]])
site-packages/torch/_inductor/codecache.py:609] [0/1] Can't pickle
site-packages/torch/_inductor/codecache.py:609] [0/1] Traceback (most recent call last):
site-packages/torch/_inductor/codecache.py:609] [0/1]   File "/opt/conda/lib/python3.11/site-packages/torch/_inductor/codecache.py", line 605, in dumps
site-packages/torch/_inductor/codecache.py:609] [0/1]     pickler.dump(obj)
site-packages/torch/_inductor/codecache.py:609] [0/1] AttributeError: Can't pickle local object 'make_opaque_unary_fn.<locals>.OpaqueUnaryFn'
site-packages/torch/utils/_sympy/interp.py:159] [0/1] failed while executing pow_by_natural([VR[0, int_oo], VR[-1, -1]])

…136802) Summary: fix for #136797 We have an issue with deserialization for symbolic expressions, where the original expression has a structure mismatch with the deserialized expression. This happens because we serialize the expressions into strings, and deserialize using sympy.sympify(). This seems to cause issues with custom torch sympy.Functions, in this case FloorDiv. In this case, this alters the structure from `FloorDiv(a, b)` into `sympy.floor(a, PowByNatural(b, -1))`, or `sympy.floor(sympy.Mul(*a), PowByNatural(b, -1))` if a is also a sympy.Mul. This seems to break downstream when we call torch.empty_strided() on this expression, and the PowByNatural has an invalid ValueRange (e.g. [0, -1]). This fixes that by adding translations after deserialization, to pattern match and transform expressions into the original format. Other potential strategies are a) we could also serialize SymInts as sympy.Expr structures, but that seems a lot more involved and BC breaking, or b) we could modify sympy.sympify(?), but I don't know if this is possible. Test Plan: test_export/test_serialize Differential Revision: D63493615

facebook-github-bot · 2024-10-03T20:51:54Z

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

pianpwk · 2024-10-03T20:53:54Z

Yeah, I think we should just move to not string format, and add to the export serialization schema

bhack · 2024-10-04T23:45:36Z

#136802 (comment)

Is this related right?

bhack · 2024-10-10T15:56:57Z

Do I need to open a new ticket for #136802 (comment) or we are on the same page here?

bhack · 2024-10-23T15:20:30Z

Is there a roadmap to review this?

bhack · 2024-11-05T15:34:15Z

Is this stalled?

bhack · 2024-11-06T13:02:36Z

I've tried to hotpatch the latest nightly with this PR and the deserializzation issue is still here.

Summary: Latest attempt after [136802](#136802) and [140084](#140084) got shelved. This keeps the string format for `expr_str`, but calls `sympy.printing.repr.srepr(s)` instead of `str(s)`, which prints expressions more explicitly, e.g. ``` ((2*x)//(3*y + 4)) -> "FloorDiv(Mul(Integer(2), Symbol('x')), Add(Mul(Integer(3), Symbol('y')), Integer(4)))" ``` This is nice because: - we have better roundtrippability for deserialization, robust to pretty printing changes like [this](https://github.com/pytorch/pytorch/blob/6c9bfd52b6a76ddff053bcff4d23ea7f4c280e9a/torch/utils/_sympy/functions.py#L208) that caused the issue in the first place. - this preserves the BC surface for both 1) sigmoid thrift serialization, by keeping the string format, and 2) deserialization for old IRs, since `sympy.sympify(...)` still handles the old `str(s)` format. - more memory efficient than storing ASTs; the [AST attempt](#140084) increased artifact size by 20% on some toy programs. - doesn't even require a schema version bump. Additionally to push some test cases over the line, this redoes expression processing (handling ranges, symbol caching) by doing bottom-up processing instead of the current hacky-ish workflow. Test Plan: test_serdes, test_serialize, internal tests broken by AST PR Reviewed By: zhxchen17 Differential Revision: D66283208

Summary: Latest attempt after [136802](#136802) and [140084](#140084) got shelved. This keeps the string format for `expr_str`, but calls `sympy.printing.repr.srepr(s)` instead of `str(s)`, which prints expressions more explicitly, e.g. ``` ((2*x)//(3*y + 4)) -> "FloorDiv(Mul(Integer(2), Symbol('x')), Add(Mul(Integer(3), Symbol('y')), Integer(4)))" ``` This is nice because: - we have better roundtrippability for deserialization, robust to pretty printing changes like [this](https://github.com/pytorch/pytorch/blob/6c9bfd52b6a76ddff053bcff4d23ea7f4c280e9a/torch/utils/_sympy/functions.py#L208) that caused the issue in the first place. - this preserves the BC surface for both 1) sigmoid thrift serialization, by keeping the string format, and 2) deserialization for old IRs, since `sympy.sympify(...)` still handles the old `str(s)` format. - more memory efficient than storing ASTs; the [AST attempt](#140084) increased artifact size by 20% on some toy programs. - doesn't even require a schema version bump. Additionally to push some test cases over the line, this redoes expression processing (handling ranges, symbol caching) by doing bottom-up processing instead of the current hacky-ish workflow. Test Plan: test_serdes, test_serialize, internal tests broken by AST PR Differential Revision: D66283208 Pull Request resolved: #141284 Approved by: https://github.com/zhxchen17

…141284) Summary: Latest attempt after [136802](pytorch#136802) and [140084](pytorch#140084) got shelved. This keeps the string format for `expr_str`, but calls `sympy.printing.repr.srepr(s)` instead of `str(s)`, which prints expressions more explicitly, e.g. ``` ((2*x)//(3*y + 4)) -> "FloorDiv(Mul(Integer(2), Symbol('x')), Add(Mul(Integer(3), Symbol('y')), Integer(4)))" ``` This is nice because: - we have better roundtrippability for deserialization, robust to pretty printing changes like [this](https://github.com/pytorch/pytorch/blob/6c9bfd52b6a76ddff053bcff4d23ea7f4c280e9a/torch/utils/_sympy/functions.py#L208) that caused the issue in the first place. - this preserves the BC surface for both 1) sigmoid thrift serialization, by keeping the string format, and 2) deserialization for old IRs, since `sympy.sympify(...)` still handles the old `str(s)` format. - more memory efficient than storing ASTs; the [AST attempt](pytorch#140084) increased artifact size by 20% on some toy programs. - doesn't even require a schema version bump. Additionally to push some test cases over the line, this redoes expression processing (handling ranges, symbol caching) by doing bottom-up processing instead of the current hacky-ish workflow. Test Plan: test_serdes, test_serialize, internal tests broken by AST PR Differential Revision: D66283208 Pull Request resolved: pytorch#141284 Approved by: https://github.com/zhxchen17

github-actions · 2025-01-05T15:33:47Z

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

bhack · 2025-01-05T15:38:36Z

if this is going to be close as stale do we still have a ticket to track this?

avikchaudhuri · 2025-01-06T17:24:20Z

Was this fixed by #141284?

bhack · 2025-01-06T17:40:47Z

I've tested on Nov, 2 and it was merge on Nov, 22. So probably it is ok

pianpwk requested review from angelayi, avikchaudhuri, tugsbayasgalan, ydwu4 and zhxchen17 as code owners September 26, 2024 23:06

pytorch-bot bot added the ciflow/inductor label Sep 26, 2024

facebook-github-bot added the fb-exported label Sep 26, 2024

facebook-github-bot force-pushed the export-D63493615 branch from e48c4b9 to ff4fe54 Compare September 26, 2024 23:48

pianpwk changed the title ~~[export] translation for FloorDiv deserialization~~ [export] add translations for SymInt/Bool deserialization; FloorDiv Sep 26, 2024

pianpwk added the release notes: export label Sep 26, 2024

pianpwk force-pushed the export-D63493615 branch from ff4fe54 to f359edf Compare September 26, 2024 23:49

angelayi reviewed Sep 27, 2024

View reviewed changes

facebook-github-bot force-pushed the export-D63493615 branch from f359edf to b53cc2d Compare October 3, 2024 20:51

bhack mentioned this pull request Oct 12, 2024

ValueRange division breaks with pow_by_natural #136797

Closed

pianpwk mentioned this pull request Nov 21, 2024

[export] fix sympy.expr roundtrippability for serialization #141284

Closed

bhack mentioned this pull request Dec 9, 2024

[torch.export.load] failed while executing pow_by_natural #136628

Open

ezyang mentioned this pull request Dec 11, 2024

isin prevents dynamic shapes in modules #142507

Closed

github-actions bot added the Stale label Jan 5, 2025

github-actions bot closed this Feb 5, 2025

github-actions bot deleted the export-D63493615 branch March 8, 2025 01:52

[export] add translations for SymInt/Bool deserialization; FloorDiv #136802

[export] add translations for SymInt/Bool deserialization; FloorDiv #136802

Uh oh!

Conversation

pianpwk commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

❌ 8 New Failures

Uh oh!

facebook-github-bot commented Sep 26, 2024

Uh oh!

facebook-github-bot commented Sep 26, 2024

Uh oh!

facebook-github-bot commented Sep 26, 2024

Uh oh!

pianpwk commented Sep 27, 2024

Uh oh!

angelayi Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

pianpwk Oct 3, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ezyang commented Sep 30, 2024

Uh oh!

pianpwk commented Sep 30, 2024

Uh oh!

ezyang commented Sep 30, 2024

Uh oh!

bhack commented Oct 1, 2024

Uh oh!

facebook-github-bot commented Oct 3, 2024

Uh oh!

pianpwk commented Oct 3, 2024

Uh oh!

bhack commented Oct 4, 2024

Uh oh!

bhack commented Oct 10, 2024

Uh oh!

bhack commented Oct 23, 2024

Uh oh!

bhack commented Nov 5, 2024

Uh oh!

bhack commented Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 5, 2025

Uh oh!

bhack commented Jan 5, 2025

Uh oh!

avikchaudhuri commented Jan 6, 2025

Uh oh!

bhack commented Jan 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pianpwk commented Sep 26, 2024 •

edited

Loading

pytorch-bot bot commented Sep 26, 2024 •

edited

Loading

bhack commented Nov 6, 2024 •

edited

Loading