Skip to content

Lazy codegen change for xla #76717

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
wants to merge 2 commits into from
Closed

Lazy codegen change for xla #76717

wants to merge 2 commits into from

Conversation

@JackCaoG
Copy link
Collaborator

@JackCaoG JackCaoG commented May 3, 2022 •
edited
Loading

Codegen change to enable PyTorch/XLA to generate the first op in pytorch/xla#3544.

@bdhirsh @wconstab

@JackCaoG JackCaoG requested a review from wconstab May 3, 2022 03:44
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented May 3, 2022 •
edited
Loading

🔗 Helpful links

✅ No Failures (0 Pending)

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

Expand to see more

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

@JackCaoG JackCaoG requested a review from bdhirsh May 3, 2022 03:44
@mruberry mruberry added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label May 3, 2022
@JackCaoG JackCaoG force-pushed the lazy_code_gen_xla branch from 161e04b to 6425217 Compare May 6, 2022 00:07
@JackCaoG JackCaoG force-pushed the lazy_code_gen_xla branch from 6425217 to f92c758 Compare May 6, 2022 00:11
wconstab
wconstab reviewed May 6, 2022
View reviewed changes
torch/csrc/lazy/core/shape_inference.h Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

haha. ok, we may have a namespace war on our hands

Copy link
Contributor

Choose a reason for hiding this comment

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

(this is ok to change to lazy:: IMO. but.. i also worry a little bit that people will not realize it breaks something when they change it back as a cleanup)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think as long as that pr trigger a xla test they will see xla build fail.

@JackCaoG
Copy link
Collaborator Author

JackCaoG commented May 6, 2022

dumb question, where can I find the linter instruction for pytorch 😄 ?

@wconstab
Copy link
Contributor

wconstab commented May 6, 2022

dumb question, where can I find the linter instruction for pytorch 😄 ?

https://github.com/pytorch/pytorch/blob/master/CONTRIBUTING.md#local-linting
tl;dr you just need to make setup_lint once and then make quicklint usually

@JackCaoG JackCaoG force-pushed the lazy_code_gen_xla branch from f92c758 to f8ad4fc Compare May 6, 2022 00:41
@albanD albanD removed their request for review May 10, 2022 02:14
@JackCaoG JackCaoG force-pushed the lazy_code_gen_xla branch from 5aceca3 to 862506a Compare May 10, 2022 04:20
wconstab
wconstab reviewed May 10, 2022
View reviewed changes
Copy link
Contributor

@wconstab wconstab left a comment

Choose a reason for hiding this comment

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

LGTM, i'll let nick approve

@JackCaoG
Copy link
Collaborator Author

CI Failures are related to lu_solve which I think is irrelevant to this pr. @Krovatkin I think this pr is ready to be reviewed. This pr can also just go in without xla pr.

Krovatkin
Krovatkin approved these changes May 11, 2022
View reviewed changes
Copy link
Contributor

@Krovatkin Krovatkin left a comment

Choose a reason for hiding this comment

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

:shipit:

@JackCaoG
Copy link
Collaborator Author

Let me resolve the conflict and try to land this.

@JackCaoG JackCaoG force-pushed the lazy_code_gen_xla branch from 862506a to b7a82cd Compare May 11, 2022 20:53
@Krovatkin
Copy link
Contributor

@pytorchbot merge this

@github-actions
Copy link
Contributor

Hey @JackCaoG.
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.

facebook-github-bot pushed a commit that referenced this pull request May 13, 2022
@JackCaoG @facebook-github-bot
Lazy codegen change for xla (#76717) (#76717)
6f38b09 
Summary:
Codegen change to enable PyTorch/XLA to generate the first op in pytorch/xla#3544.

bdhirsh wconstab

Pull Request resolved: #76717
Approved by: https://github.com/Krovatkin

Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/e36a8c1f137af6de7fc5f8d2160f0bb355738824

Reviewed By: atalman

Differential Revision: D36354010

Pulled By: atalman

fbshipit-source-id: 318588850c01a88feeb764ab46b9265bd10df2cf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wconstab wconstab wconstab left review comments

@bdhirsh bdhirsh Awaiting requested review from bdhirsh

+1 more reviewer

@Krovatkin Krovatkin Krovatkin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla signed Merged open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@JackCaoG @facebook-github-bot @wconstab @Krovatkin @pytorchbot @mruberry @pytorchmergebot