Skip to content

Delete redefinitions of methods in Variable already present on Tensor. #29667

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

Delete redefinitions of methods in Variable already present on Tensor. #29667

ezyang wants to merge 6 commits into from

Conversation

@ezyang
Copy link
Contributor

@ezyang ezyang commented Nov 12, 2019 •
edited
Loading

Stack from ghstack:

Some previous implementations are defined in native_functions.yaml.
In this case, I don't define them explicitly in Tensor; instead
they are placed in VariableTypeManual.cpp. When I did this, I would
have deleted documentation; instead, this documentation was moved
to native_functions.yaml

This also replaces current_version with just _version.

This is a carved out portion of #28287, rebased past Tensor-Variable
merge.

Signed-off-by: Edward Z. Yang [email protected]

Differential Revision: D18504934

@ezyang
Delete redefinitions of methods in Variable already present on Tensor.
8417cc6 
Some previous implementations are defined in native_functions.yaml.
In this case, I don't define them explicitly in Tensor; instead
they are placed in VariableTypeManual.cpp. When I did this, I would
have deleted documentation; instead, this documentation was moved
to native_functions.yaml

This is a carved out portion of #28287, rebased past Tensor-Variable
merge.

Signed-off-by: Edward Z. Yang <[email protected]>

[ghstack-poisoned]
@ezyang ezyang requested a review from apaszke as a code owner November 12, 2019 21:30
This was referenced Nov 12, 2019
Closed
Closed
Closed
Closed
Closed
@ezyang ezyang requested review from albanD, gchanan and yf225 November 12, 2019 21:31
@ezyang
Update on "Delete redefinitions of methods in Variable already presen…
5e8aa33 
…t on Tensor."

Some previous implementations are defined in native_functions.yaml.
In this case, I don't define them explicitly in Tensor; instead
they are placed in VariableTypeManual.cpp. When I did this, I would
have deleted documentation; instead, this documentation was moved
to native_functions.yaml

This is a carved out portion of #28287, rebased past Tensor-Variable
merge.

Signed-off-by: Edward Z. Yang <[email protected]>

[ghstack-poisoned]
@ezyang
Update on "Delete redefinitions of methods in Variable already presen…
f522571 
…t on Tensor."


Some previous implementations are defined in native_functions.yaml.
In this case, I don't define them explicitly in Tensor; instead
they are placed in VariableTypeManual.cpp. When I did this, I would
have deleted documentation; instead, this documentation was moved
to native_functions.yaml

This also replaces `current_version` with just `_version`.

This is a carved out portion of #28287, rebased past Tensor-Variable
merge.

Signed-off-by: Edward Z. Yang <[email protected]>

[ghstack-poisoned]
ezyang added a commit that referenced this pull request Nov 12, 2019
@ezyang
Delete redefinitions of methods in Variable already present on Tensor.
54004f3 
Some previous implementations are defined in native_functions.yaml.
In this case, I don't define them explicitly in Tensor; instead
they are placed in VariableTypeManual.cpp. When I did this, I would
have deleted documentation; instead, this documentation was moved
to native_functions.yaml

This is a carved out portion of #28287, rebased past Tensor-Variable
merge.

Signed-off-by: Edward Z. Yang <[email protected]>

ghstack-source-id: 230faf9
Pull Request resolved: #29667
@ezyang
Update on "Delete redefinitions of methods in Variable already presen…
21ad803 
…t on Tensor."


Some previous implementations are defined in native_functions.yaml.
In this case, I don't define them explicitly in Tensor; instead
they are placed in VariableTypeManual.cpp. When I did this, I would
have deleted documentation; instead, this documentation was moved
to native_functions.yaml

This also replaces `current_version` with just `_version`.

This is a carved out portion of #28287, rebased past Tensor-Variable
merge.

Signed-off-by: Edward Z. Yang <[email protected]>

[ghstack-poisoned]
ezyang added a commit that referenced this pull request Nov 13, 2019
@ezyang
Delete redefinitions of methods in Variable already present on Tensor.
49ecdea 
Some previous implementations are defined in native_functions.yaml.
In this case, I don't define them explicitly in Tensor; instead
they are placed in VariableTypeManual.cpp. When I did this, I would
have deleted documentation; instead, this documentation was moved
to native_functions.yaml

This is a carved out portion of #28287, rebased past Tensor-Variable
merge.

Signed-off-by: Edward Z. Yang <[email protected]>

ghstack-source-id: dd2af2f
Pull Request resolved: #29667
@kostmo
Copy link
Member

kostmo commented Nov 13, 2019

CircleCI build failures summary

As of commit 21ad803:

  • 1/1 broken upstream at merge base 6d54c5d (see grid view)
    • You may want to rebase on the viable/strict branch (see age history): 
      git fetch viable/strict
git rebase viable/strict
  • 0/1 failures introduced in this PR
  • 0/1 recognized as flaky

Here are the reasons each build failed:

Job Step Log excerpt
pytorch_xla_linux_xenial_py3_6_clang7_build Build Failed to run '['/var/lib/jenkins/workspace/xla/scripts/generate_code.sh']'

This comment was automatically generated by Dr. CI.
Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker.

gchanan
gchanan requested changes Nov 13, 2019
View reviewed changes
aten/src/ATen/native/native_functions.yaml Outdated
- func: backward(Tensor self, Tensor? gradient=None, bool keep_graph=False, bool create_graph=False) -> ()
variants: method

# Sets the tensor data held by this `Variable` to be the same as `new_data`.
Copy link
Contributor

@gchanan gchanan Nov 13, 2019

Choose a reason for hiding this comment

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

I think we should deprecate this in C++ (i.e., only in documentation).

You can say use something like NoGrad and in-place functions instead.

Copy link
Contributor Author

@ezyang ezyang Nov 14, 2019

Choose a reason for hiding this comment

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

Well, in Python I can say x.data = ... and to the best of my knowledge we haven't deprecated that yet. Are you suggesting we deprecate this too?

Copy link
Collaborator

@albanD albanD Nov 14, 2019

Choose a reason for hiding this comment

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

YES ! Please !
(note that might require update to some of our optimizers)

aten/src/ATen/native/native_functions.yaml Outdated
supports_named_tensor: True

# Returns a copy of this `Variable` that is detached from its autograd graph
# and has a blank version. This method is OK to call if the `Variable` is a
Copy link
Contributor

@gchanan gchanan Nov 13, 2019

Choose a reason for hiding this comment

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

this isn't correct, I think you are confusing detach and .data (in python).

E.g.:

>>> torch.randn(2,3).add_(1).add_(2).detach()._version
2
>>> torch.randn(2,3).add_(1).add_(2).data._version
0

Copy link
Contributor Author

@ezyang ezyang Nov 14, 2019

Choose a reason for hiding this comment

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

BTW, I plead "pre-existing" code ;) (I'll fix)

@ezyang
Update on "Delete redefinitions of methods in Variable already presen…
b1c7e31 
…t on Tensor."


Some previous implementations are defined in native_functions.yaml.
In this case, I don't define them explicitly in Tensor; instead
they are placed in VariableTypeManual.cpp. When I did this, I would
have deleted documentation; instead, this documentation was moved
to native_functions.yaml

This also replaces `current_version` with just `_version`.

This is a carved out portion of #28287, rebased past Tensor-Variable
merge.

Signed-off-by: Edward Z. Yang <[email protected]>

[ghstack-poisoned]
ezyang added a commit that referenced this pull request Nov 14, 2019
@ezyang
Delete redefinitions of methods in Variable already present on Tensor.
5d577c1 
Some previous implementations are defined in native_functions.yaml.
In this case, I don't define them explicitly in Tensor; instead
they are placed in VariableTypeManual.cpp. When I did this, I would
have deleted documentation; instead, this documentation was moved
to native_functions.yaml

This is a carved out portion of #28287, rebased past Tensor-Variable
merge.

Signed-off-by: Edward Z. Yang <[email protected]>

ghstack-source-id: 74d7575
Pull Request resolved: #29667
@ezyang ezyang mentioned this pull request Nov 14, 2019
Closed
albanD
albanD reviewed Nov 15, 2019
View reviewed changes
aten/src/ATen/native/native_functions.yaml Outdated

# Returns the input index of the gradient `Node` to which this
# `Variable` is connected. Note: input indexes of the gradient `Node`
# correspond to output indexes of the corresponding forward `Node`.
Copy link
Collaborator

@albanD albanD Nov 15, 2019

Choose a reason for hiding this comment

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

"forward None" is not really a thing :/
Maybe just "forward function" here?

Copy link
Contributor Author

@ezyang ezyang Nov 15, 2019

Choose a reason for hiding this comment

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

I plead preexisting comments XD (ok will fix)

aten/src/ATen/native/native_functions.yaml Outdated
supports_named_tensor: True
variants: function, method

# Like `detach()`, but removes this `Variable` in-place. This method may
Copy link
Collaborator

@albanD albanD Nov 15, 2019

Choose a reason for hiding this comment

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

"removes" -> "modifies" ?

@ezyang ezyang requested a review from gchanan November 15, 2019 16:47
@ezyang
Update on "Delete redefinitions of methods in Variable already presen…
6aca9de 
…t on Tensor."


Some previous implementations are defined in native_functions.yaml.
In this case, I don't define them explicitly in Tensor; instead
they are placed in VariableTypeManual.cpp. When I did this, I would
have deleted documentation; instead, this documentation was moved
to native_functions.yaml

This also replaces `current_version` with just `_version`.

This is a carved out portion of #28287, rebased past Tensor-Variable
merge.

Signed-off-by: Edward Z. Yang <[email protected]>

Differential Revision: [D18504934](https://our.internmc.facebook.com/intern/diff/D18504934)

[ghstack-poisoned]
@ezyang ezyang mentioned this pull request Nov 15, 2019
Closed
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Nov 18, 2019

@ezyang merged this pull request in f6cadad.

zdevito pushed a commit to zdevito/ATen that referenced this pull request Nov 18, 2019
@ezyang @facebook-github-bot
Delete redefinitions of methods in Variable already present on Tensor…
a5596a9 
…. (#29667)

Summary:
Pull Request resolved: pytorch/pytorch#29667

Some previous implementations are defined in native_functions.yaml.
In this case, I don't define them explicitly in Tensor; instead
they are placed in VariableTypeManual.cpp. When I did this, I would
have deleted documentation; instead, this documentation was moved
to native_functions.yaml

This also replaces `current_version` with just `_version`.

This is a carved out portion of #28287, rebased past Tensor-Variable
merge.

Signed-off-by: Edward Z. Yang <[email protected]>

Test Plan: Imported from OSS

Differential Revision: D18504934

Pulled By: ezyang

fbshipit-source-id: be7adf45b637daffe2b0b1631eb31d967525fc31
ezyang added a commit to ezyang/pytorch that referenced this pull request Nov 19, 2019
@ezyang
Delete redefinitions of methods in Variable already present on Tensor.
7bf950a 
Some previous implementations are defined in native_functions.yaml.
In this case, I don't define them explicitly in Tensor; instead
they are placed in VariableTypeManual.cpp. When I did this, I would
have deleted documentation; instead, this documentation was moved
to native_functions.yaml

This is a carved out portion of pytorch#28287, rebased past Tensor-Variable
merge.

Signed-off-by: Edward Z. Yang <[email protected]>

ghstack-source-id: 0d2141e
Pull Request resolved: pytorch#29667
@facebook-github-bot facebook-github-bot deleted the gh/ezyang/532/head branch November 22, 2019 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@albanD albanD albanD left review comments

@yf225 yf225 yf225 approved these changes

@apaszke apaszke Awaiting requested review from apaszke

@gchanan gchanan Awaiting requested review from gchanan

Assignees

No one assigned

Labels

Merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@ezyang @kostmo @facebook-github-bot @gchanan @yf225 @albanD @mruberry