Make unspecified ints to range over negative and positive. #104658

ysiraichi · 2023-07-05T20:05:48Z

Stack from ghstack (oldest at bottom):

-> Make unspecified ints to range over negative and positive. #104658

Currently, negative unspecified ints get specialized. This PR creates symbolic values for
unspecified ints (including negative ones).

For example, with this PR, the following code only compiles once, instead of 3 times:

def foo(x, y):
    return torch.fill(torch.zeros(x.shape), y)

foo(10)
foo(-5)
foo(-3)

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @Xia-Weiwen @wenzhe-nrv @jiayisunx @peterbell10 @ipiszy @ngimel @yf225 @chenyang78 @kadeng @muchulee8 @anijain2305

Currently, negative unspecified ints get specialized. This PR creates symbolic values for unspecified ints (including negative ones). For example, with this PR, the following code only compiles once, instead of 3 times: ```python def foo(x, y): return torch.fill(torch.zeros(x.shape), y) foo(10) foo(-5) foo(-3) ``` [ghstack-poisoned]

pytorch-bot · 2023-07-05T20:05:51Z

🔗 Helpful Links

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

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

✅ No Failures

As of commit 7d58430:
💚 Looks good so far! There are no failures yet. 💚

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

Currently, negative unspecified ints get specialized. This PR creates symbolic values for unspecified ints (including negative ones). For example, with this PR, the following code only compiles once, instead of 3 times: ```python def foo(x, y): return torch.fill(torch.zeros(x.shape), y) foo(10) foo(-5) foo(-3) ``` ghstack-source-id: 1d5229b Pull Request resolved: #104658

torch/_dynamo/config.py

…o range over negative and positive." Currently, negative unspecified ints get specialized. This PR creates symbolic values for unspecified ints (including negative ones). For example, with this PR, the following code only compiles once, instead of 3 times: ```python def foo(x, y): return torch.fill(torch.zeros(x.shape), y) foo(10) foo(-5) foo(-3) ``` cc voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx ipiszy chenyang78 [ghstack-poisoned]

Currently, negative unspecified ints get specialized. This PR creates symbolic values for unspecified ints (including negative ones). For example, with this PR, the following code only compiles once, instead of 3 times: ```python def foo(x, y): return torch.fill(torch.zeros(x.shape), y) foo(10) foo(-5) foo(-3) ``` ghstack-source-id: 683ad02 Pull Request resolved: #104658

torch/_dynamo/variables/builder.py

ezyang · 2023-07-08T20:13:18Z

torch/fx/experimental/symbolic_shapes.py

        source: Source,
        dynamic_dim: DimDynamic = DimDynamic.DUCK,
        constraint_dim: DimConstraint = None,  # NB: includes None
+        positive: Optional[bool] = True,


Is this really Optional[bool]? What does None mean?

It seems you can set this to False to mean "never positive", but we don't actually have any concrete use case for this, AFAIK

That's true. I did this so that I wouldn't need to positive = positive if positive else None when I'm actually creating the symbol. Maybe I could just leave a comment when I call it with positive=None. What do you think?

I think a comment is OK too, just make it hard for people to accidentally say positive=False when they meant positive=None

Hmmm. Maybe we could just throw in an assert there like:

def create_symbol( ... positive: Optional[bool] = True ): assert positive != False, ( "Unspecified integers range over negative and positive numbers. " "Setting positive=False means strictly negative. " "Use positive=None instead." )

Let me know what you think.

Currently, negative unspecified ints get specialized. This PR creates symbolic values for unspecified ints (including negative ones). For example, with this PR, the following code only compiles once, instead of 3 times: ```python def foo(x, y): return torch.fill(torch.zeros(x.shape), y) foo(10) foo(-5) foo(-3) ``` cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 anijain2305 [ghstack-poisoned]

Currently, negative unspecified ints get specialized. This PR creates symbolic values for unspecified ints (including negative ones). For example, with this PR, the following code only compiles once, instead of 3 times: ```python def foo(x, y): return torch.fill(torch.zeros(x.shape), y) foo(10) foo(-5) foo(-3) ``` ghstack-source-id: 533f70d Pull Request resolved: #104658

Currently, negative unspecified ints get specialized. This PR creates symbolic values for unspecified ints (including negative ones). For example, with this PR, the following code only compiles once, instead of 3 times: ```python def foo(x, y): return torch.fill(torch.zeros(x.shape), y) foo(10) foo(-5) foo(-3) ``` cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 anijain2305 [ghstack-poisoned]

Currently, negative unspecified ints get specialized. This PR creates symbolic values for unspecified ints (including negative ones). For example, with this PR, the following code only compiles once, instead of 3 times: ```python def foo(x, y): return torch.fill(torch.zeros(x.shape), y) foo(10) foo(-5) foo(-3) ``` ghstack-source-id: 8965e07 Pull Request resolved: #104658

ezyang · 2023-07-10T20:13:51Z

By the way, if we notice that this is making codegen worse (because we are unable to assume that something is positive), we could proactively make the ints positive (guarding appropriately) pending seeing a negative value. It didn't seem like it's necessary here but this is something we can put in our back pocket.

ysiraichi · 2023-07-10T20:24:44Z

Right. Do you mean: create symbols for positive and negative "on demand"?

ezyang · 2023-07-10T23:25:31Z

Essentially, we could proactively assume that a symbol is positive even if we have no evidence for it, and then make it range over negative/positive if we saw a counterexample. Anyway, not for this PR.

ysiraichi · 2023-07-11T11:37:35Z

@ezyang @jansel I renamed create_symint_and_symbol --> create_unspecified_symint_and_symbol, so as to avoid incorrect use. I'm pretty sure I have changed the its only call site. Is it ok to suppress the BC-lint step?

ezyang · 2023-07-11T12:30:18Z

Yep do it

ysiraichi · 2023-07-11T13:41:58Z

@pytorchbot merge

pytorchmergebot · 2023-07-11T13:43:47Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

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

pytorchmergebot · 2023-07-11T14:04:08Z

Merge failed

Reason: 1 jobs have failed, first few of them are: trunk / win-vs2019-cpu-py3 / test (default, 2, 3, windows.4xlarge.nonephemeral)

Details for Dev Infra team

Raised by workflow job

ysiraichi · 2023-07-11T19:10:37Z

@pytorchbot merge

pytorchmergebot · 2023-07-11T19:13:11Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

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

pytorch-bot bot added the release notes: fx release notes category label Jul 5, 2023

github-actions bot added module: dynamo ciflow/inductor labels Jul 5, 2023

ysiraichi commented Jul 5, 2023

View reviewed changes

torch/_dynamo/config.py Outdated Show resolved Hide resolved

github-actions bot added the module: inductor label Jul 7, 2023

pytorchbot added the open source label Jul 7, 2023

ysiraichi requested review from ezyang and jansel July 8, 2023 13:19

ezyang reviewed Jul 8, 2023

View reviewed changes

torch/_dynamo/variables/builder.py Outdated Show resolved Hide resolved

ezyang reviewed Jul 8, 2023

View reviewed changes

torch/_dynamo/variables/builder.py Outdated Show resolved Hide resolved

ezyang reviewed Jul 8, 2023

View reviewed changes

torch/_dynamo/variables/builder.py Outdated Show resolved Hide resolved

ezyang reviewed Jul 8, 2023

View reviewed changes

ezyang approved these changes Jul 8, 2023

View reviewed changes

ysiraichi added the suppress-bc-linter Suppresses the failures of API backward-compatibility linter (Lint/bc_linter) label Jul 11, 2023

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Jul 11, 2023

pytorchmergebot added the merging label Jul 11, 2023

pytorchmergebot removed the merging label Jul 11, 2023

pytorchmergebot added the merging label Jul 11, 2023

pytorchmergebot added the Merged label Jul 11, 2023

pytorchmergebot removed the merging label Jul 11, 2023

pytorchmergebot closed this in ad37dd5 Jul 11, 2023

facebook-github-bot deleted the gh/ysiraichi/60/head branch July 15, 2023 14:17

Make unspecified ints to range over negative and positive. #104658

Make unspecified ints to range over negative and positive. #104658

Uh oh!

Conversation

ysiraichi commented Jul 5, 2023 • edited by pytorch-bot bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Jul 5, 2023 • 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/104658

✅ No Failures

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ezyang Jul 8, 2023

Choose a reason for hiding this comment

Uh oh!

ezyang Jul 8, 2023

Choose a reason for hiding this comment

Uh oh!

ysiraichi Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

ezyang Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

ysiraichi Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

ezyang commented Jul 10, 2023

Uh oh!

ysiraichi commented Jul 10, 2023

Uh oh!

ezyang commented Jul 10, 2023

Uh oh!

ysiraichi commented Jul 11, 2023

Uh oh!

ezyang commented Jul 11, 2023

Uh oh!

ysiraichi commented Jul 11, 2023

Uh oh!

pytorchmergebot commented Jul 11, 2023

Merge started

Uh oh!

pytorchmergebot commented Jul 11, 2023

Merge failed

Uh oh!

ysiraichi commented Jul 11, 2023

Uh oh!

pytorchmergebot commented Jul 11, 2023

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ysiraichi commented Jul 5, 2023 •

edited by pytorch-bot bot

Loading

pytorch-bot bot commented Jul 5, 2023 •

edited

Loading