Add meta variants to float variants of normal#69632
Closed
nkaretnikov wants to merge 1 commit intogh/nkaretnikov/5/basefrom
Closed
Add meta variants to float variants of normal#69632nkaretnikov wants to merge 1 commit intogh/nkaretnikov/5/basefrom
nkaretnikov wants to merge 1 commit intogh/nkaretnikov/5/basefrom
Conversation
This mimics what normal_ does. Not sure whether TensorOptions need to explicitly set the device type to meta for these. Without this change, both of these variants already work with meta, but maybe explicit definitions are somehow better (e.g., result in less computations): >>> torch.normal(mean=4., std=1., size=(1, 2, 3), device='meta') tensor(..., device='meta', size=(1, 2, 3)) >>> torch.normal(mean=4., std=1., size=(4, 5), out=torch.rand(1, device='meta')) tensor(..., device='meta', size=(4, 5)) [ghstack-poisoned]
CI Flow Status⚛️ CI FlowRuleset - Version:
You can add a comment to the PR and tag @pytorchbot with the following commands: # ciflow rerun, "ciflow/default" will always be added automatically
@pytorchbot ciflow rerun
# ciflow rerun with additional labels "-l <ciflow/label_name>", which is equivalent to adding these labels manually and trigger the rerun
@pytorchbot ciflow rerun -l ciflow/scheduled -l ciflow/slowFor more information, please take a look at the CI Flow Wiki. |
This was referenced Dec 8, 2021
nkaretnikov
added a commit
that referenced
this pull request
Dec 8, 2021
This mimics what normal_ does. Not sure whether TensorOptions need to explicitly set the device type to meta for these. Without this change, both of these variants already work with meta, but maybe explicit definitions are somehow better (e.g., result in less computations): >>> torch.normal(mean=4., std=1., size=(1, 2, 3), device='meta') tensor(..., device='meta', size=(1, 2, 3)) >>> torch.normal(mean=4., std=1., size=(4, 5), out=torch.rand(1, device='meta')) tensor(..., device='meta', size=(4, 5)) ghstack-source-id: 3c27c38 Pull Request resolved: #69632
Contributor
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit 1977986 (more details on the Dr. CI page):
🕵️ 14 new failures recognized by patternsThe following CI failures do not appear to be due to upstream breakages:
|
Collaborator
Author
|
hey @pbelevich, i put you in reviewers because this touches/undoes some of the work you did related to RNG: #35167 (see the previous commits in this stack) |
Collaborator
Author
|
this is my first structured kernel port. so if something doesn't make sense, it's probably because i don't know what i'm doing. some notes related to this pr: #69386 in particular:
things i'm not sure about:
warning: you shouldn't rely on my tests/judgement since it's likely that i messed something up. but i did my best to uncover any potential issues related to BC/shapes. i made no effort to test RNG-related things since i didn't really touch them. |
lezcano
reviewed
Dec 9, 2021
Collaborator
lezcano
left a comment
There was a problem hiding this comment.
Any reason why we shouldn't port these to structured kernels?
Collaborator
Author
@lezcano well, i'm not sure you can do better than what i just did here (might be wrong tho). the signatures of these are completely different (differ more than just by the |
Collaborator
|
I see how these are different to the others. Even then, you are given the |
Collaborator
Author
i'm not sure what you mean exactly. specifically, i don't understand this part:
the problem i'm describing is this: to use structured, the signatures need to be: that is, they must only differ by the so the only way to make this work is either introducing a bunch of redundant optional memory-allocation parameters to the out variant, but that would be bad since allocation is already determined by the passed tensors OR you could add more functions to the api, but that would blow up the number of operators for no good reason, no? they would offer no new functionality and would just be there to satisfy the current parser/codegen machinery. lmk if i misunderstood your point |
Collaborator
Author
|
i'd also like to point out that these already work with |
ysiraichi
reviewed
Dec 11, 2021
Collaborator
ysiraichi
left a comment
There was a problem hiding this comment.
Hey @nkaretnikov.
Good job! I think we can avoid duplicating normal_out. Check out my comment, and let me know if it's unclear.
| return result; // similar to normal_meta_ | ||
| } | ||
|
|
||
| Tensor& normal_out( |
Collaborator
There was a problem hiding this comment.
It seems to me that both normal_out and normal_out_meta do the same thing. You could dispatch CPU, CUDA, and META to the same function.
Collaborator
Author
|
to avoid confusion, will open a new stack to address issues related to BC and broadcasting |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack from ghstack:
This mimics what normal_ does. Not sure whether TensorOptions need to
explicitly set the device type to meta for these.
Without this change, both of these variants already work with meta, but
maybe explicit definitions are somehow better (e.g., result in less
computations):