Skip to content
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.

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

add adaptive_avg_pool2d #363

Merged
merged 15 commits into from May 21, 2024
Merged

add adaptive_avg_pool2d #363

merged 15 commits into from May 21, 2024

Conversation

kiya00
Copy link
Collaborator

@kiya00 kiya00 commented May 3, 2024

Before submitting
  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

Fixes #346 .

@IvanYashchuk IvanYashchuk removed their request for review May 6, 2024 11:28
@kiya00
Copy link
Collaborator Author

kiya00 commented May 7, 2024

Hi @nikitaved , I fixed the test case, could you help to take a look again

Copy link
Contributor

@nikitaved nikitaved left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, now I am back! LGTM! Thank you :)

@kiya00
Copy link
Collaborator Author

kiya00 commented May 16, 2024

Hi @t-vi @mruberry , could you also take a look if it can be merged

thunder/core/prims.py Outdated Show resolved Hide resolved
thunder/core/prims.py Outdated Show resolved Hide resolved
thunder/core/prims.py Outdated Show resolved Hide resolved
thunder/core/prims.py Outdated Show resolved Hide resolved
thunder/core/prims.py Outdated Show resolved Hide resolved
thunder/torch/__init__.py Outdated Show resolved Hide resolved
thunder/torch/__init__.py Outdated Show resolved Hide resolved
thunder/torch/__init__.py Outdated Show resolved Hide resolved
thunder/torch/__init__.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@mruberry mruberry left a comment

Choose a reason for hiding this comment

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

Awesome, @kiya00! I made some small comments for your review.

@IvanYashchuk would you like to take a look at this, too?

Copy link
Collaborator

@IvanYashchuk IvanYashchuk left a comment

Choose a reason for hiding this comment

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

Let's move the meta functions from prims.py to the torch file and remove the prims.* functions. They don't add any value since it's a rather direct path from thunder.torch.adaptive_avg_pool2d -> thunder.prims.adaptive_avg_pool2d.

Once it's done let's merge it.

thunder/core/prims.py Outdated Show resolved Hide resolved
thunder/executors/torchex.py Outdated Show resolved Hide resolved
@kiya00
Copy link
Collaborator Author

kiya00 commented May 21, 2024

Hi @t-vi @carmocca , I think it's ready to merge, the failures come from the known distributed case issue

@t-vi t-vi enabled auto-merge (squash) May 21, 2024 15:08
@t-vi
Copy link
Collaborator

t-vi commented May 21, 2024

Thank you @kiya00 @nikitaved @IvanYashchuk @mruberry

@t-vi t-vi merged commit 50b2567 into main May 21, 2024
37 checks passed
@t-vi t-vi deleted the adp_avg_pool2d branch May 21, 2024 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support torch.nn.functional.adaptive_avg_pool2d
5 participants