Skip to content

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

Closed
ezyang wants to merge 6 commits intogh/ezyang/532/basefrom
gh/ezyang/532/head
Closed

Delete redefinitions of methods in Variable already present on Tensor.#29667
ezyang wants to merge 6 commits intogh/ezyang/532/basefrom
gh/ezyang/532/head

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 ezyang@fb.com

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 <ezyang@fb.com>

[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 <ezyang@fb.com>

[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 <ezyang@fb.com>

[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 <ezyang@fb.com>

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 <ezyang@fb.com>

[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 <ezyang@fb.com>

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 <ezyang@fb.com>

[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 <ezyang@fb.com>

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 <ezyang@fb.com>

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 <ezyang@fb.com>

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 <ezyang@fb.com>

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.

7 participants

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