Skip to content

Introducing SymInt to Pytorch (for tracing size arithmetic) (master rebase) #74861

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.

Sign up for GitHub

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

Jump to bottom
Closed
Krovatkin wants to merge 8 commits into from
Closed

Introducing SymInt to Pytorch (for tracing size arithmetic) (master rebase) #74861

Krovatkin wants to merge 8 commits into from

Conversation

@Krovatkin
Copy link
Contributor

@Krovatkin Krovatkin commented Mar 28, 2022 •
edited
Loading

This PR introduces SymInt type to Pytorch which will be used by LTC and AOTAutograd for tracing size arithmetic and tests.
SymInt is a C++ union structure [int64_t, SymbolicIntNode*] that wraps around an int64_t field where the value of the field could be an index into a list of shared_ptr<SymbolicIntNode> or a real int.
This PR doesn't add any support for actually tracing symbolic ints. i.e. data_ for now can only contain real ints.

Goal 1: just to show we can add a type to PyTorch core. (wraps int) LANDEABLE
Finalize the naming - symint
Want the name to be short
Does invoke “size” - NO
SInt/SymInt/SymbolicInt
SInt could mean signed int
sym_int or symint or SymInt (originally it was “int”; capitalized implies object semantics, whereas lowercase implies value semantics)
JIT schema - symint
C++ - symint

See more details here: https://docs.google.com/document/d/1iiLNwR5ohAsw_ymfnOpDsyF6L9RTUaHMpD8YLw-jxEw

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Mar 28, 2022 •
edited
Loading

🔗 Helpful links

💊 CI failures summary and remediations

As of commit 840a382 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Mar 28, 2022
@Krovatkin Krovatkin force-pushed the krovatkin/size_int_master branch from 7d019e7 to 5731273 Compare March 28, 2022 23:28
@Krovatkin Krovatkin mentioned this pull request Mar 28, 2022
Closed
@Krovatkin Krovatkin changed the title SymInt rebased on master Introducing SymInt to Pytorch (for tracing size arithmetic) (master rebase) Mar 28, 2022
Krovatkin
Krovatkin commented Mar 28, 2022
View reviewed changes
torch/types.py Outdated
Copy link
Contributor Author

@Krovatkin Krovatkin Mar 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

this is a temporary workaround. Eventually SymInt would be Union[int, SymIntNode] and we will be hitting the exact same error:

error: Overloaded function signature 2 will never be matched: signature 1's parameter type(s) are the same or broader  [misc]

fyi: @ezyang

@Krovatkin Krovatkin force-pushed the krovatkin/size_int_master branch from 5731273 to bd9846f Compare March 29, 2022 04:17
@miladm miladm self-requested a review March 29, 2022 06:10
@Krovatkin Krovatkin force-pushed the krovatkin/size_int_master branch 2 times, most recently from 52b47dd to 0ebb322 Compare March 29, 2022 18:44
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Mar 29, 2022

@Krovatkin has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@Krovatkin Krovatkin force-pushed the krovatkin/size_int_master branch from 0ebb322 to 537fd43 Compare March 30, 2022 16:23
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Mar 30, 2022

@Krovatkin has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

1 similar comment
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Mar 30, 2022

@Krovatkin has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@Krovatkin
BoxedInt take 2
f493b9e 
rename SymbolicOrConcreteInt -> SymInt

address Ed's feedback

remove spurious changes

fix schema matching to allow int to be converted to symint

change to expect_int

test narrow_copy in jit

missed some feedback

fix headers

add a comment to SymInt

add annotation str to pass fx tests

fix a trailing space lint

address feedback and fix build failures

build fixes

fix dynamic types

more CI fixes

add pragma once

add SymInt

remove type not defined errors

resolve bc issues

add SymInt to templates
@Krovatkin
clean up int64_t overload
fc5be32
@Krovatkin
remove jit changes
9ec8247
@Krovatkin
divorce SymInt from int
69b5ba9
@Krovatkin
remove overload name
025d2b2
@Krovatkin
fix no-op build
7aa1c02
@Krovatkin
fix issues with lazy
901359f
@Krovatkin
clang-tidy
840a382
@Krovatkin Krovatkin force-pushed the krovatkin/size_int_master branch from 7365790 to 840a382 Compare March 31, 2022 17:02
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Mar 31, 2022

@Krovatkin has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot pushed a commit that referenced this pull request Mar 31, 2022
@Krovatkin @facebook-github-bot
Introducing SymInt to Pytorch (for tracing size arithmetic) (master r…
218643f 
…ebase) (#74861)

Summary:
This PR introduces `SymInt` type to Pytorch which will be used by LTC and AOTAutograd for tracing size arithmetic and tests.
`SymInt` is a C++ union structure [int64_t, SymbolicIntNode*] that wraps around an int64_t field where the value of the field could be an index into a list of `shared_ptr<SymbolicIntNode>` or a real int.
This PR doesn't add any support for actually tracing symbolic ints. i.e. data_ for now can only contain real ints.

```
Goal 1: just to show we can add a type to PyTorch core. (wraps int) LANDEABLE
Finalize the naming - symint
Want the name to be short
Does invoke “size” - NO
SInt/SymInt/SymbolicInt
SInt could mean signed int
sym_int or symint or SymInt (originally it was “int”; capitalized implies object semantics, whereas lowercase implies value semantics)
JIT schema - symint
C++ - symint
```

See more details here: https://docs.google.com/document/d/1iiLNwR5ohAsw_ymfnOpDsyF6L9RTUaHMpD8 (d843f63f2a49c339fe79c3016a082c60fbf3fed5)YLw-jxEw

Pull Request resolved: #74861

Reviewed By: qihqi, ngimel

Differential Revision: D35226230

Pulled By: Krovatkin

fbshipit-source-id: 34acf342bd50fcaa4d8d5dd49c2fd6a98823a5b3
@github-actions
Copy link
Contributor

github-actions bot commented Mar 31, 2022

Hey @Krovatkin.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

@WBobby WBobby mentioned this pull request Aug 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ezyang ezyang ezyang approved these changes

@bdhirsh bdhirsh Awaiting requested review from bdhirsh

@albanD albanD Awaiting requested review from albanD

@miladm miladm Awaiting requested review from miladm

Assignees

No one assigned

Labels

cla signed oncall: jit Add this issue/PR to JIT oncall triage queue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Krovatkin @facebook-github-bot @ezyang