Skip to content
This repository was archived by the owner on Aug 1, 2025. It is now read-only.

Conversation

fdrocha
Copy link
Contributor

@fdrocha fdrocha commented Sep 6, 2022

Stack from ghstack (oldest at bottom):

There is now a decomposition in pytorch that seems to have
better performance, see benchmarks at
pytorch/pytorch#84350

There is now a decomposition in pytorch that seems to have
better performance, see benchmarks at
pytorch/pytorch#84350

[ghstack-poisoned]
fdrocha pushed a commit that referenced this pull request Sep 6, 2022
There is now a decomposition in pytorch that seems to have
better performance, see benchmarks at
pytorch/pytorch#84350

ghstack-source-id: d8f7d54
Pull Request resolved: #1134
There is now a decomposition in pytorch that seems to have
better performance, see benchmarks at
pytorch/pytorch#84350

[ghstack-poisoned]
fdrocha pushed a commit that referenced this pull request Sep 6, 2022
There is now a decomposition in pytorch that seems to have
better performance, see benchmarks at
pytorch/pytorch#84350

ghstack-source-id: 7ff6b8a
Pull Request resolved: #1134
make_fallback(aten._embedding_bag)
make_fallback(aten._embedding_bag_forward_only)
make_fallback(aten._fused_moving_avg_obs_fq_helper)
make_fallback(aten.grid_sampler_2d_backward)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this PR resolves that, no? Since we're currently applying decompositions after autograd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right! Thanks, fixed now.

There is now a decomposition in pytorch that seems to have
better performance, see benchmarks at
pytorch/pytorch#84350

[ghstack-poisoned]
fdrocha pushed a commit that referenced this pull request Sep 6, 2022
There is now a decomposition in pytorch that seems to have
better performance, see benchmarks at
pytorch/pytorch#84350

ghstack-source-id: 3e6559d
Pull Request resolved: #1134
@fdrocha
Copy link
Contributor Author

fdrocha commented Sep 7, 2022

The test failures are in test/test_aot_cudagraphs.py. They seem unrelated to this PR, I get them also on main.
@Chillee any thoughts?

@Chillee
Copy link
Contributor

Chillee commented Sep 7, 2022

@fdrocha yeah they're not caused by this PR, but by updating the pin.

@fdrocha
Copy link
Contributor Author

fdrocha commented Sep 7, 2022

@fdrocha yeah they're not caused by this PR, but by updating the pin.

Thanks @Chillee . I was able to get the tests to go green by replacing "fake_result" with "val" in torchdynamo/torchdynamo/optimizations/training.py and in torch/fx/passes/backends/cudagraphs.py.
I think you did something similar in pytorch/pytorch#84331

Should I make those changes? Don't understand those parts of the code base, but it seems like the right thing...

There also a bunch of other instances of "fake_result" in torch repo that should probably be changed to "val" FWIW

@Chillee
Copy link
Contributor

Chillee commented Sep 7, 2022

@fdrocha Yeah we just need to get a PR in updating all the uses (also see pytorch/pytorch#84432).

@fdrocha
Copy link
Contributor Author

fdrocha commented Sep 7, 2022

@fdrocha Yeah we just need to get a PR in updating all the uses (also see pytorch/pytorch#84432).

I see. I guess I will wait for that PR to be merged before trying to merge this one.

@jansel
Copy link
Contributor

jansel commented Sep 7, 2022

We could also just skip the tests until the upstream breaking is fixed.

@Chillee
Copy link
Contributor

Chillee commented Sep 7, 2022

Yeah let's do that and land this.

There is now a decomposition in pytorch that seems to have
better performance, see benchmarks at
pytorch/pytorch#84350

[ghstack-poisoned]
fdrocha pushed a commit that referenced this pull request Sep 8, 2022
There is now a decomposition in pytorch that seems to have
better performance, see benchmarks at
pytorch/pytorch#84350

ghstack-source-id: c91a2c5
Pull Request resolved: #1134
There is now a decomposition in pytorch that seems to have
better performance, see benchmarks at
pytorch/pytorch#84350

[ghstack-poisoned]
fdrocha pushed a commit that referenced this pull request Sep 8, 2022
There is now a decomposition in pytorch that seems to have
better performance, see benchmarks at
pytorch/pytorch#84350

ghstack-source-id: 986b35a
Pull Request resolved: #1134
There is now a decomposition in pytorch that seems to have
better performance, see benchmarks at
pytorch/pytorch#84350

[ghstack-poisoned]
fdrocha pushed a commit that referenced this pull request Sep 9, 2022
There is now a decomposition in pytorch that seems to have
better performance, see benchmarks at
pytorch/pytorch#84350

ghstack-source-id: adef10f
Pull Request resolved: #1134
There is now a decomposition in pytorch that seems to have
better performance, see benchmarks at
pytorch/pytorch#84350

[ghstack-poisoned]
fdrocha pushed a commit that referenced this pull request Sep 12, 2022
There is now a decomposition in pytorch that seems to have
better performance, see benchmarks at
pytorch/pytorch#84350

ghstack-source-id: 4f2951f
Pull Request resolved: #1134
There is now a decomposition in pytorch that seems to have
better performance, see benchmarks at
pytorch/pytorch#84350

[ghstack-poisoned]
fdrocha pushed a commit that referenced this pull request Sep 12, 2022
There is now a decomposition in pytorch that seems to have
better performance, see benchmarks at
pytorch/pytorch#84350

ghstack-source-id: 872e1f1
Pull Request resolved: #1134
There is now a decomposition in pytorch that seems to have
better performance, see benchmarks at
pytorch/pytorch#84350

[ghstack-poisoned]
fdrocha pushed a commit that referenced this pull request Sep 14, 2022
There is now a decomposition in pytorch that seems to have
better performance, see benchmarks at
pytorch/pytorch#84350

ghstack-source-id: 93522ab
Pull Request resolved: #1134
@fdrocha fdrocha merged commit b799472 into gh/fdrocha/1/base Sep 14, 2022
@jansel
Copy link
Contributor

jansel commented Sep 15, 2022

This looks like it was not properly merged @fdrocha -- the merge button doesn't work with ghstack

@fdrocha
Copy link
Contributor Author

fdrocha commented Sep 15, 2022

This looks like it was not properly merged @fdrocha -- the merge button doesn't work with ghstack

Drat. I did use ghstack land to merge this but I got a bunch of permission related errors. I assumed it was okay because GH says "successfully merged" but guess not.

Any idea on how to fix this?

@lezcano
Copy link
Contributor

lezcano commented Sep 15, 2022

@ezyang what permissions do we need to land ghstack PRs?

@ezyang
Copy link
Contributor

ezyang commented Sep 15, 2022

This is landed now. Apparently you need to be repo admin lol

@ezyang
Copy link
Contributor

ezyang commented Sep 15, 2022

oops actualy not yet

ezyang pushed a commit that referenced this pull request Sep 15, 2022
There is now a decomposition in pytorch that seems to have
better performance, see benchmarks at
pytorch/pytorch#84350

ghstack-source-id: 93522ab
Pull Request resolved: #1134
@ezyang ezyang deleted the gh/fdrocha/1/head branch September 15, 2022 23:59
@lezcano
Copy link
Contributor

lezcano commented Sep 16, 2022

@fdrocha we should start sending PRs without ghstack then :(

@fdrocha
Copy link
Contributor Author

fdrocha commented Sep 19, 2022

@ezyang this is not merged yet, right?
Do you think I should just make a new PR and merge that?

@lezcano
Copy link
Contributor

lezcano commented Sep 19, 2022

I think this was merged a couple days ago:
image

@fdrocha
Copy link
Contributor Author

fdrocha commented Sep 19, 2022 via email

@Chillee
Copy link
Contributor

Chillee commented Sep 19, 2022

I was talking to @ezyang and his opinion was that "we should just ask him to merge ghstack PRs if we want them landed" (for now).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants