[export] serialize sympy.Exprs as ASTs instead of strings #140084

pianpwk · 2024-11-08T01:26:19Z

Summary: The way we've been de/serializing sympy.Exprs is not roundtrippable in all cases (serialize by calling str(expr), and deserialize by calling sympy.sympify(expr_str)). This has led to expressions being mathematically equivalent but structurally different, causing issues in ValueRanges. Example issue: #136797

This starts to deprecate the use of expr_str and stores expressions in AST format instead. For BC purposes, expr_str deserialization is still supported, but we will always serialize to expr_ast. We'll kill this once the serialization upgrader design is finalized and implemented.

Test Plan: test_export

Differential Revision: D65638757

pytorch-bot · 2024-11-08T01:26:23Z

🔗 Helpful Links

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

📄 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.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

[DomainsOnly] Jobs fail with GLIBC version not found

❌ 3 New Failures

As of commit 5087a21 with merge base 0443398 ():

NEW FAILURES - The following jobs have failed:

Lint / lintrunner-noclang / linux-job (gh)
>>> Lint for test/test_nestedtensor.py:
linux-binary-manywheel / manywheel-py3_9-cuda12_6-test / test (gh)
RuntimeError: cuDNN version incompatibility: PyTorch was compiled against (9, 5, 1) but found runtime version (9, 1, 0). PyTorch already comes bundled with cuDNN. One option to resolving this error is to ensure PyTorch can find the bundled cuDNN. one possibility is that there is a conflicting cuDNN in LD_LIBRARY_PATH.
pull / linux-jammy-py3.9-gcc11 / test (numpy_2_x, 1, 1, lf.linux.2xlarge) (gh)
'Test'

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

facebook-github-bot · 2024-11-08T01:26:32Z

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

angelayi · 2024-11-08T14:26:30Z

torch/_export/serde/schema.py

 # This is for storing the symbolic expressions behind symints/symfloats/symbools
 # For example, we can get something like
 # SymExpr(expr_str="s0 + s1", hint=SymExprHint(as_int=4)
 # if we also have the hint that s0 and s1 are both 2.


update the comment?

angelayi · 2024-11-08T14:29:15Z

torch/_export/serde/serialize.py

-
-            if val.expr_str in self.symbol_name_to_symbol:
-                sym = self.symbol_name_to_symbol[val.expr_str]
+            if val.expr_str != "":


Would it be better to check like, if hasattr(val, "expr_ast")?

Ah the field isn't optional, and it's BC breaking if we change that. I could switch to None instead.

angelayi · 2024-11-08T14:30:50Z

torch/_export/serde/serialize.py

+                )
+            return expr
+
+    def __deprecated_do_not_use_deserialize_symint_expr_str(self, s: str, hint: Optional[int] = None) -> sympy.Expr:


do you think theres a way to implement this like an upgrader? If we see something using expr_str can we somehow change it to become an expr_ast and then use the expr_ast implementation?

We eventually want to implement an upgrader/downgrader so if we just do that here then it could save us the work later.

Yeahh this is a temporary fix while we figure out the upgrader design

Summary: for roundtrippability Test Plan: test_export Reviewed By: angelayi Differential Revision: D65638757

facebook-github-bot · 2024-11-08T18:06:03Z

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

pianpwk · 2024-11-08T18:07:08Z

torch/_export/serde/serialize.py

+        return SymExprNode(target=None, base=base, args=[])
+    else:
+        return SymExprNode(
+            target=type(expr).__module__ + "." + type(expr).__qualname__,


@zhxchen17 @avikchaudhuri

pianpwk · 2024-11-08T18:10:27Z

torch/_export/serde/serialize.py

+        else:
+            assert s.target is not None
+            *_module, _qualname = s.target.split(".")
+            node_cls = getattr(sys.modules[".".join(_module)], _qualname)


@zhxchen17 @avikchaudhuri any thoughts on this? storing the node target as the python class name (similar to nn_module_stack).

An alternative is to keep a mapping of known functions <-> names, something like

{ "mul": sympy.Mul, "add": sympy.Add, ... }

But that requires constant updating if we run into new functions. It potentially is safer though, and easier to catch if some day sympy changes their function names/locations in library (we'd break running this file & upgrading sympy pin, instead of when we try to deserialize the particular program).

Hmm, I doubt the locations will change much otherwise any user of sympy will also break? Do the class names here resolve to public paths or private paths (since sympy has many aliases and deep directory structure)?

Summary: The way we've been de/serializing sympy.Exprs is not roundtrippable in all cases (serialize by calling `str(expr)`, and deserialize by calling `sympy.sympify(expr_str)`). This has led to expressions being mathematically equivalent but structurally different, causing issues in ValueRanges. Example issue: #136797 This starts to deprecate the use of `expr_str` and stores expressions in AST format instead. For BC purposes, `expr_str` deserialization is still supported, but we will always serialize to `expr_ast`. We'll kill this once the serialization upgrader design is finalized and implemented. Test Plan: test_export Reviewed By: angelayi Differential Revision: D65638757

facebook-github-bot · 2024-11-18T21:15:37Z

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

Summary: The way we've been de/serializing sympy.Exprs is not roundtrippable in all cases (serialize by calling `str(expr)`, and deserialize by calling `sympy.sympify(expr_str)`). This has led to expressions being mathematically equivalent but structurally different, causing issues in ValueRanges. Example issue: #136797 This starts to deprecate the use of `expr_str` and stores expressions in AST format instead. For BC purposes, `expr_str` deserialization is still supported, but we will always serialize to `expr_ast`. We'll kill this once the serialization upgrader design is finalized and implemented. Test Plan: test_export Reviewed By: angelayi Differential Revision: D65638757

facebook-github-bot · 2024-11-19T03:08:55Z

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

Summary: The way we've been de/serializing sympy.Exprs is not roundtrippable in all cases (serialize by calling `str(expr)`, and deserialize by calling `sympy.sympify(expr_str)`). This has led to expressions being mathematically equivalent but structurally different, causing issues in ValueRanges. Example issue: #136797 This starts to deprecate the use of `expr_str` and stores expressions in AST format instead. For BC purposes, `expr_str` deserialization is still supported, but we will always serialize to `expr_ast`. We'll kill this once the serialization upgrader design is finalized and implemented. Test Plan: test_export Reviewed By: angelayi Differential Revision: D65638757

facebook-github-bot · 2024-11-19T19:23:13Z

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

Summary: The way we've been de/serializing sympy.Exprs is not roundtrippable in all cases (serialize by calling `str(expr)`, and deserialize by calling `sympy.sympify(expr_str)`). This has led to expressions being mathematically equivalent but structurally different, causing issues in ValueRanges. Example issue: #136797 This starts to deprecate the use of `expr_str` and stores expressions in AST format instead. For BC purposes, `expr_str` deserialization is still supported, but we will always serialize to `expr_ast`. We'll kill this once the serialization upgrader design is finalized and implemented. Test Plan: test_export Reviewed By: angelayi Differential Revision: D65638757

facebook-github-bot · 2024-11-20T00:24:22Z

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

pytorchmergebot · 2024-11-20T07:38:44Z

Merge started

Your change will be merged while ignoring the following 1 checks: Lint / lintrunner-noclang / linux-job

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

izaitsevfb · 2024-11-21T20:08:16Z

@pytorchbot revert -m "reverted internally in D66253238" -c ghfirst

pytorchmergebot · 2024-11-21T20:09:46Z

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

…40084)" This reverts commit d869344. Reverted #140084 on behalf of https://github.com/izaitsevfb due to reverted internally in D66253238 ([comment](#140084 (comment)))

pytorchmergebot · 2024-11-21T20:09:58Z

@pianpwk your PR has been successfully reverted.

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

…0084) Summary: The way we've been de/serializing sympy.Exprs is not roundtrippable in all cases (serialize by calling `str(expr)`, and deserialize by calling `sympy.sympify(expr_str)`). This has led to expressions being mathematically equivalent but structurally different, causing issues in ValueRanges. Example issue: pytorch#136797 This starts to deprecate the use of `expr_str` and stores expressions in AST format instead. For BC purposes, `expr_str` deserialization is still supported, but we will always serialize to `expr_ast`. We'll kill this once the serialization upgrader design is finalized and implemented. Test Plan: test_export Differential Revision: D65638757 Pull Request resolved: pytorch#140084 Approved by: https://github.com/angelayi

…torch#140084)" This reverts commit d869344. Reverted pytorch#140084 on behalf of https://github.com/izaitsevfb due to reverted internally in D66253238 ([comment](pytorch#140084 (comment)))

…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

ezyang · 2024-12-11T18:58:19Z

What's the status on this?

github-actions · 2025-02-09T19:34:01Z

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.

pianpwk requested review from angelayi, avikchaudhuri, tugsbayasgalan, ydwu4 and zhxchen17 as code owners November 8, 2024 01:26

pytorch-bot bot added ciflow/inductor release notes: export labels Nov 8, 2024

facebook-github-bot added the fb-exported label Nov 8, 2024

angelayi approved these changes Nov 8, 2024

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Nov 8, 2024

facebook-github-bot pushed a commit that referenced this pull request Nov 8, 2024

[export] serialize sympy.Exprs as ASTs instead of strings (#140084)

6ea6823

Summary: for roundtrippability Test Plan: test_export Reviewed By: angelayi Differential Revision: D65638757

facebook-github-bot force-pushed the export-D65638757 branch from 00ef9a2 to 6ea6823 Compare November 8, 2024 18:05

pianpwk commented Nov 8, 2024

View reviewed changes

facebook-github-bot force-pushed the export-D65638757 branch from 6ea6823 to 9242544 Compare November 18, 2024 21:15

facebook-github-bot force-pushed the export-D65638757 branch from 9242544 to d9bf2dc Compare November 19, 2024 03:08

pianpwk mentioned this pull request Nov 19, 2024

Add truediv support in export serializer #136364

Closed

5 tasks

facebook-github-bot force-pushed the export-D65638757 branch from d9bf2dc to 18dab5c Compare November 19, 2024 19:22

facebook-github-bot force-pushed the export-D65638757 branch from 18dab5c to 5087a21 Compare November 20, 2024 00:23

pytorchmergebot added the Merged label Nov 20, 2024

pytorchmergebot closed this in d869344 Nov 20, 2024

pytorchmergebot removed the merging label Nov 20, 2024

pytorchmergebot added Reverted ci-no-td Do not run TD on this PR labels Nov 21, 2024

pytorchmergebot reopened this Nov 21, 2024

This was referenced Nov 21, 2024

Back out "[export] serialize sympy.Exprs as ASTs instead of strings" #141252

Closed

Back out "[export] serialize sympy.Exprs as ASTs instead of strings" #141253

Closed

pianpwk mentioned this pull request Nov 21, 2024

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

Closed

ezyang mentioned this pull request Dec 11, 2024

isin prevents dynamic shapes in modules #142507

Closed

github-actions bot added the Stale label Feb 9, 2025

github-actions bot closed this Mar 11, 2025

github-actions bot deleted the export-D65638757 branch April 11, 2025 02:29

[export] serialize sympy.Exprs as ASTs instead of strings #140084

[export] serialize sympy.Exprs as ASTs instead of strings #140084

Uh oh!

Conversation

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

Uh oh!

Uh oh!

pytorch-bot bot commented Nov 8, 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/140084

❗ 1 Active SEVs

❌ 3 New Failures

Uh oh!

facebook-github-bot commented Nov 8, 2024

Uh oh!

angelayi Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

angelayi Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

pianpwk Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

angelayi Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

pianpwk Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Nov 8, 2024

Uh oh!

pianpwk Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avikchaudhuri Nov 11, 2024

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Nov 18, 2024

Uh oh!

facebook-github-bot commented Nov 19, 2024

Uh oh!

facebook-github-bot commented Nov 19, 2024

Uh oh!

facebook-github-bot commented Nov 20, 2024

Uh oh!

pytorchmergebot commented Nov 20, 2024

Merge started

Uh oh!

izaitsevfb commented Nov 21, 2024

Uh oh!

pytorchmergebot commented Nov 21, 2024

Uh oh!

pytorchmergebot commented Nov 21, 2024

Uh oh!

ezyang commented Dec 11, 2024

Uh oh!

github-actions bot commented Feb 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

pianpwk commented Nov 8, 2024 •

edited

Loading

pytorch-bot bot commented Nov 8, 2024 •

edited

Loading

pianpwk Nov 8, 2024 •

edited

Loading