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

fix issue #27985 #27986

Merged
merged 7 commits into from
Apr 22, 2019
Merged

fix issue #27985 #27986

merged 7 commits into from
Apr 22, 2019

Conversation

pminervini
Copy link
Contributor

@pminervini pminervini commented Apr 19, 2019

The dresult variable may be an IndexedSlices object, which does not have a _copy method.

Link to the issue: #27985

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@pminervini
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@rthadur rthadur added this to Assigned Reviewer in PR Queue via automation Apr 19, 2019
@rthadur rthadur self-assigned this Apr 19, 2019
@rthadur rthadur requested a review from alextp April 19, 2019 16:02
@rthadur rthadur added comp:ops OPs related issues size:XS CL Change Size: Extra Small labels Apr 19, 2019
PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Apr 19, 2019
Copy link
Contributor

@alextp alextp left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

I'd like a test which fails before this change and passes after, so we don't accidentally break this again.

@pminervini
Copy link
Contributor Author

@alextp yeah let me briefly look into that

@pminervini
Copy link
Contributor Author

pminervini commented Apr 19, 2019

I managed to reproduce the problem with a small self-contained script:

#!/usr/bin/env python3
# -*- coding: utf-8 -*-

import tensorflow as tf
import tensorflow.contrib.eager as tfe

tf.enable_eager_execution()

shape = (5, 5)
initializer = tf.random_uniform_initializer(-1.0, 1.0)
var = tfe.Variable(initializer(shape), name='tensor')

with tf.GradientTape() as tape:
        a = tf.gather(tf.gather(var, [0, 1]), [0, 1])
        b = tf.gather(tf.gather(var, [2, 3]), [0, 1])
        r = tf.einsum('ij,ij->i', a, b)
g = tape.gradient(r, [var])

Here's the output - I'll commit a unit test shortly.

$ ./fun.py 
2019-04-19 20:30:43.159014: I tensorflow/core/platform/cpu_feature_guard.cc:141] Your CPU supports instructions that this TensorFlow binary was not compiled to use: AVX2 FMA
2019-04-19 20:30:43.240709: I tensorflow/stream_executor/cuda/cuda_gpu_executor.cc:964] successful NUMA node read from SysFS had negative value (-1), but there must be at least one NUMA node, so returning NUMA node zero
2019-04-19 20:30:43.241170: I tensorflow/core/common_runtime/gpu/gpu_device.cc:1432] Found device 0 with properties: 
name: TITAN X (Pascal) major: 6 minor: 1 memoryClockRate(GHz): 1.531
pciBusID: 0000:02:00.0
totalMemory: 11.91GiB freeMemory: 5.75GiB
2019-04-19 20:30:43.241190: I tensorflow/core/common_runtime/gpu/gpu_device.cc:1511] Adding visible gpu devices: 0
2019-04-19 20:30:43.517288: I tensorflow/core/common_runtime/gpu/gpu_device.cc:982] Device interconnect StreamExecutor with strength 1 edge matrix:
2019-04-19 20:30:43.517315: I tensorflow/core/common_runtime/gpu/gpu_device.cc:988]      0 
2019-04-19 20:30:43.517324: I tensorflow/core/common_runtime/gpu/gpu_device.cc:1001] 0:   N 
2019-04-19 20:30:43.517479: I tensorflow/core/common_runtime/gpu/gpu_device.cc:1115] Created TensorFlow device (/job:localhost/replica:0/task:0/device:GPU:0 with 5526 MB memory) -> physical GPU (device: 0, name: TITAN X (Pascal), pci bus id: 0000:02:00.0, compute capability: 6.1)
Traceback (most recent call last):
  File "./fun.py", line 17, in <module>
    g = tape.gradient(r, [var])
  File "/home/anaconda/anaconda3/lib/python3.6/site-packages/tensorflow/python/eager/backprop.py", line 901, in gradient
    output_gradients=output_gradients)
  File "/home/anaconda/anaconda3/lib/python3.6/site-packages/tensorflow/python/eager/imperative_grad.py", line 64, in imperative_grad
    output_gradients)
  File "/home/anaconda/anaconda3/lib/python3.6/site-packages/tensorflow/python/framework/ops.py", line 858, in grad_fun
    return [dresult._copy(device_name=self_device)]
AttributeError: 'IndexedSlices' object has no attribute '_copy'

@pminervini
Copy link
Contributor Author

@alextp this should do the trick - what do you think?

@rthadur rthadur requested a review from alextp April 19, 2019 20:09
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Apr 19, 2019
alextp
alextp previously approved these changes Apr 19, 2019
@alextp alextp added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Apr 19, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 19, 2019
PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Apr 19, 2019
@pminervini
Copy link
Contributor Author

@alextp link to the test: https://github.com/pminervini/tensorflow/blob/master/tensorflow/python/framework/ops_test.py#L161

Is there a way to enforce this to run on GPU?

@alextp
Copy link
Contributor

alextp commented Apr 22, 2019

Use the @test_util.run_gpu_only decorator

@@ -880,7 +880,7 @@ def _copy(self, ctx=None, device_name=None):
self_device = self.device

def grad_fun(dresult):
return [dresult._copy(device_name=self_device)]
return [dresult._copy(device_name=self_device)] if hasattr(dresult, '_copy') else [dresult]
Copy link
Contributor

Choose a reason for hiding this comment

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

The linter will fail on this line of code.

Also, you should probably put the whole thing inside the [], like return [dresult._copy() if hasattr(dresult, '_copy') else dresult] which will make it easier to split into lines to fit in 80 columns.

@alextp alextp added the kokoro:force-run Tests on submitted change label Apr 22, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 22, 2019
@pminervini
Copy link
Contributor Author

@alextp done!

PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Apr 22, 2019
alextp
alextp previously approved these changes Apr 22, 2019
@alextp alextp added the kokoro:force-run Tests on submitted change label Apr 22, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 22, 2019
PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Apr 22, 2019
@pminervini
Copy link
Contributor Author

@alextp there was one line exceeding 80 chars in ops_test.py, it should be fine now

@rthadur rthadur requested a review from alextp April 22, 2019 17:11
@rthadur rthadur added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Apr 22, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 22, 2019
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Apr 22, 2019
@alextp alextp added the kokoro:force-run Tests on submitted change label Apr 22, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 22, 2019
@pminervini
Copy link
Contributor Author

@alextp mmm where is the problem?

@alextp
Copy link
Contributor

alextp commented Apr 22, 2019

Nothing wrong, the build is just currently broken.

@tensorflow-copybara tensorflow-copybara merged commit 8459d85 into tensorflow:master Apr 22, 2019
PR Queue automation moved this from Approved by Reviewer to Merged Apr 22, 2019
tensorflow-copybara pushed a commit that referenced this pull request Apr 22, 2019
PiperOrigin-RevId: 244719214
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes comp:ops OPs related issues ready to pull PR ready for merge process size:XS CL Change Size: Extra Small
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

6 participants