projects/guix·blob python-team · refs

312 lines · 13.3 KiB

1From eb61daae91432be0b07bb2f6854887bedfa6fc95 Mon Sep 17 00:00:00 2001
2From: Asim Shankar <[email protected]>
3Date: Tue, 26 Jun 2018 00:57:33 -0700
4Subject: [PATCH] [C API]: Bugfix for TF_AddGradients.
5
6TF_AddGradients could create nodes in the graph with names that conflicted with
7other nodes in the graph. This would most clearly happen if TF_AddGradients()
8was called twice on the same graph, and could also happen if there were other
9nodes in the graph that happened to have "gradients" as a prefix of their name.
10
11Fix that.
12
13The added test in c_api_test.cc would fail in the call to TF_SessionRun() with
14 Node 'gradients/OnesLike' is not unique
15without the changes to c_api.cc and c_api_internal.h
16
17While at it, also fixed a possible name collision bug when using the C++ API
18to constructor graphs (using Scope).
19
20Thanks @karllessard for pointing this out.
21
22PiperOrigin-RevId: 202087996
23---
24 tensorflow/c/c_api.cc | 13 ++++-
25 tensorflow/c/c_api_test.cc | 65 ++++++++++++++++++++++--
26 tensorflow/c/c_test_util.cc | 7 +++
27 tensorflow/c/c_test_util.h | 3 ++
28 tensorflow/cc/framework/scope.cc | 30 ++++++++---
29 tensorflow/cc/framework/scope_internal.h | 3 +-
30 tensorflow/cc/framework/scope_test.cc | 10 ++++
31 7 files changed, 116 insertions(+), 15 deletions(-)
32
33diff --git a/tensorflow/c/c_api.cc b/tensorflow/c/c_api.cc
34index 09a03639d6fa3..37c8302e08bc3 100644
35--- a/tensorflow/c/c_api.cc
36+++ b/tensorflow/c/c_api.cc
37@@ -2414,7 +2414,18 @@ void TF_AddGradients(TF_Graph* g, TF_Output* y, int ny, TF_Output* x, int nx,
38 for (int i = first_new_node_id; i < g->graph.num_node_ids(); ++i) {
39 Node* n = g->graph.FindNodeId(i);
40 if (n == nullptr) continue;
41- g->name_map[n->name()] = n;
42+ // We have a convoluted scheme here: Using the C++ graph construction API
43+ // to add potentially many nodes to the graph without running the checks
44+ // (such as uniqueness of the names of nodes) we run with other functions
45+ // that add a node to the graph (like TF_FinishOperation).
46+ if (!g->name_map.insert(std::make_pair(n->name(), n)).second) {
47+ status->status = tensorflow::errors::Internal(
48+ "BUG: The API allowed construction of a graph with duplicate node "
49+ "names (",
50+ n->name(),
51+ "). This is a bug. Please file an issue at "
52+ "https://github.com/tensorflow/tensorflow/issues.");
53+ }
54 }
55 }
56
57diff --git a/tensorflow/c/c_api_test.cc b/tensorflow/c/c_api_test.cc
58index 577f10c5e69ea..bc04b53fbb7fa 100644
59--- a/tensorflow/c/c_api_test.cc
60+++ b/tensorflow/c/c_api_test.cc
61@@ -1160,7 +1160,7 @@ TEST(CAPI, GetOpDef) {
62 }
63
64 void StringVectorToArrays(const std::vector<string>& v,
65- std::unique_ptr<const void* []>* ptrs,
66+ std::unique_ptr<const void*[]>* ptrs,
67 std::unique_ptr<size_t[]>* lens) {
68 ptrs->reset(new const void*[v.size()]);
69 lens->reset(new size_t[v.size()]);
70@@ -1196,7 +1196,7 @@ class CApiColocationTest : public ::testing::Test {
71
72 void SetViaStringList(TF_OperationDescription* desc,
73 const std::vector<string>& list) {
74- std::unique_ptr<const void* []> list_ptrs;
75+ std::unique_ptr<const void*[]> list_ptrs;
76 std::unique_ptr<size_t[]> list_lens;
77 StringVectorToArrays(list, &list_ptrs, &list_lens);
78 TF_SetAttrStringList(desc, tensorflow::kColocationAttrName, list_ptrs.get(),
79@@ -1700,6 +1700,61 @@ TEST_F(CApiGradientsTest, OpWithNoGradientRegistered_NoGradInputs) {
80 TestGradientsError(false);
81 }
82
83+void ScalarFloatFromTensor(const TF_Tensor* t, float* f) {
84+ ASSERT_TRUE(t != nullptr);
85+ ASSERT_EQ(TF_FLOAT, TF_TensorType(t));
86+ ASSERT_EQ(0, TF_NumDims(t));
87+ ASSERT_EQ(4, TF_TensorByteSize(t));
88+ float* p = static_cast<float*>(TF_TensorData(t));
89+ *f = *p;
90+}
91+
92+TEST_F(CApiGradientsTest, MultipleCallsToAddGradients) {
93+ const float X = 3.0f, Y = 7.0f;
94+ TF_Operation* x = Placeholder(graph_, s_, "x", TF_FLOAT);
95+ TF_Operation* y = Placeholder(graph_, s_, "y", TF_FLOAT);
96+ TF_Operation* xy = Mul(x, y, graph_, s_, "xy");
97+ TF_Output dxy_dx, dxy_dy;
98+
99+ TF_Output outputs[1] = {{xy, 0}};
100+ TF_Output inputs[1] = {{x, 0}};
101+ TF_AddGradients(graph_, outputs, 1, inputs, 1, nullptr, s_, &dxy_dx);
102+ ASSERT_EQ(TF_OK, TF_GetCode(s_)) << TF_Message(s_);
103+
104+ inputs[0] = {y, 0};
105+ TF_AddGradients(graph_, outputs, 1, inputs, 1, nullptr, s_, &dxy_dy);
106+ ASSERT_EQ(TF_OK, TF_GetCode(s_)) << TF_Message(s_);
107+
108+ TF_SessionOptions* opts = TF_NewSessionOptions();
109+ TF_Session* sess = TF_NewSession(graph_, opts, s_);
110+ TF_DeleteSessionOptions(opts);
111+ ASSERT_EQ(TF_OK, TF_GetCode(s_)) << TF_Message(s_);
112+
113+ TF_Output feeds[] = {{x, 0}, {y, 0}};
114+ TF_Tensor* feedValues[] = {FloatTensor(X), FloatTensor(Y)};
115+ TF_Output fetches[] = {dxy_dx, dxy_dy};
116+ TF_Tensor* fetchValues[] = {nullptr, nullptr};
117+
118+ TF_SessionRun(sess, nullptr /* run_options */, feeds, feedValues, 2, fetches,
119+ fetchValues, 2, nullptr /* target_opers */, 0,
120+ nullptr /* run_metadata */, s_);
121+ TF_DeleteTensor(feedValues[0]);
122+ TF_DeleteTensor(feedValues[1]);
123+ ASSERT_EQ(TF_OK, TF_GetCode(s_)) << TF_Message(s_);
124+ TF_DeleteSession(sess, s_);
125+ ASSERT_EQ(TF_OK, TF_GetCode(s_)) << TF_Message(s_);
126+
127+ float dxy_dxValue = 0.0f, dxy_dyValue = 0.0f;
128+ ScalarFloatFromTensor(fetchValues[0], &dxy_dxValue);
129+ EXPECT_EQ(Y, dxy_dxValue);
130+
131+ ScalarFloatFromTensor(fetchValues[1], &dxy_dyValue);
132+ EXPECT_EQ(X, dxy_dyValue);
133+
134+ TF_DeleteTensor(fetchValues[0]);
135+ TF_DeleteTensor(fetchValues[1]);
136+}
137+
138 // REGISTER_OP for CApiAttributesTest test cases.
139 // Registers two ops, each with a single attribute called 'v'.
140 // The attribute in one op will have a type 'type', the other
141@@ -1784,7 +1839,7 @@ TEST_F(CApiAttributesTest, String) {
142
143 TEST_F(CApiAttributesTest, StringList) {
144 std::vector<string> list = {"bugs", "bunny", "duck"};
145- std::unique_ptr<const void* []> list_ptrs;
146+ std::unique_ptr<const void*[]> list_ptrs;
147 std::unique_ptr<size_t[]> list_lens;
148 StringVectorToArrays(list, &list_ptrs, &list_lens);
149 int list_total_size = 0;
150@@ -1800,7 +1855,7 @@ TEST_F(CApiAttributesTest, StringList) {
151 ASSERT_EQ(TF_OK, TF_GetCode(s_)) << TF_Message(s_);
152
153 EXPECT_TF_META("v", list.size(), TF_ATTR_STRING, list_total_size);
154- std::unique_ptr<void* []> values(new void*[list.size()]);
155+ std::unique_ptr<void*[]> values(new void*[list.size()]);
156 std::unique_ptr<size_t[]> lens(new size_t[list.size()]);
157 std::unique_ptr<char[]> storage(new char[list_total_size]);
158 TF_OperationGetAttrStringList(oper, "v", values.get(), lens.get(),
159@@ -2025,7 +2080,7 @@ TEST_F(CApiAttributesTest, TensorShapeProtoList) {
160 tensorflow::PartialTensorShape(pts2).AsProto(&proto);
161 proto.SerializeToString(&bytes2);
162
163- std::unique_ptr<const void* []> list_ptrs;
164+ std::unique_ptr<const void*[]> list_ptrs;
165 std::unique_ptr<size_t[]> list_lens;
166 const std::vector<string> list = {bytes1, bytes2};
167 StringVectorToArrays(list, &list_ptrs, &list_lens);
168diff --git a/tensorflow/c/c_test_util.cc b/tensorflow/c/c_test_util.cc
169index f3b28c1708129..24eb6c069b213 100644
170--- a/tensorflow/c/c_test_util.cc
171+++ b/tensorflow/c/c_test_util.cc
172@@ -216,6 +216,13 @@ TF_Operation* Min(TF_Operation* l, TF_Operation* r, TF_Graph* graph,
173 return MinWithDevice(l, r, graph, /*op_device=*/"", s, name);
174 }
175
176+TF_Operation* Mul(TF_Operation* l, TF_Operation* r, TF_Graph* graph,
177+ TF_Status* s, const char* name) {
178+ TF_Operation* op;
179+ BinaryOpHelper("Mul", l, r, graph, s, name, &op, "", true);
180+ return op;
181+}
182+
183 TF_Operation* Add(TF_Output l, TF_Output r, TF_Graph* graph, TF_Status* s,
184 const char* name) {
185 TF_OperationDescription* desc = TF_NewOperation(graph, "AddN", name);
186diff --git a/tensorflow/c/c_test_util.h b/tensorflow/c/c_test_util.h
187index c16aba666ee69..38313d647ca93 100644
188--- a/tensorflow/c/c_test_util.h
189+++ b/tensorflow/c/c_test_util.h
190@@ -80,6 +80,9 @@ TF_Operation* Add(TF_Output l, TF_Output r, TF_Graph* graph, TF_Status* s,
191 TF_Operation* Min(TF_Operation* l, TF_Operation* r, TF_Graph* graph,
192 TF_Status* s, const char* name = "min");
193
194+TF_Operation* Mul(TF_Operation* l, TF_Operation* r, TF_Graph* graph,
195+ TF_Status* s, const char* name = "mul");
196+
197 // If `op_device` is non-empty, set the created op on that device.
198 TF_Operation* MinWithDevice(TF_Operation* l, TF_Operation* r, TF_Graph* graph,
199 const string& op_device, TF_Status* s,
200diff --git a/tensorflow/cc/framework/scope.cc b/tensorflow/cc/framework/scope.cc
201index 62a889181e787..8c886f31711eb 100644
202--- a/tensorflow/cc/framework/scope.cc
203+++ b/tensorflow/cc/framework/scope.cc
204@@ -37,6 +37,11 @@ Scope& Scope::operator=(const Scope& other) {
205 return *this;
206 }
207
208+namespace {
209+const char kScopeSeparator[] = "/";
210+const char kSuffixSeparator[] = "_";
211+} // namespace
212+
213 Scope::Impl::Impl(Graph* graph, Status* status, NameMap* name_map,
214 ShapeRefiner* refiner, bool disable_shape_inference)
215 : graph_(graph),
216@@ -308,19 +313,23 @@ string Scope::Impl::GetUniqueName(const string& prefix,
217 return prefix;
218 }
219 auto entry = name_map_->find(prefix);
220- string unique_name = prefix;
221 if (entry == name_map_->end()) {
222 name_map_->insert({prefix, 0});
223- } else {
224- unique_name = strings::StrCat(unique_name, "_", ++entry->second);
225+ return prefix;
226 }
227+ string unique_name;
228+ do {
229+ unique_name = strings::StrCat(prefix, kSuffixSeparator, ++entry->second);
230+ } while (name_map_->find(unique_name) != name_map_->end());
231+ name_map_->insert({unique_name, 0});
232 return unique_name;
233 }
234
235 string Scope::Impl::GetNameForOp(const string& default_name) const {
236 const string unique_name =
237 GetUniqueName(default_name, true /* check_single_use */);
238- const string sep = name_.empty() || unique_name.empty() ? "" : "/";
239+ const string sep =
240+ name_.empty() || unique_name.empty() ? "" : kScopeSeparator;
241 return strings::StrCat(name_, sep, unique_name);
242 }
243
244@@ -345,7 +354,8 @@ Scope Scope::NewSubScope(const string& child_scope_name) const {
245 }
246 const string unique_name =
247 impl()->GetUniqueName(child_scope_name, false /* check_single_use */);
248- const string sep = impl()->name_.empty() || unique_name.empty() ? "" : "/";
249+ const string sep =
250+ impl()->name_.empty() || unique_name.empty() ? "" : kScopeSeparator;
251 return Scope(new Impl(*this, Impl::Tags::ScopeName(),
252 strings::StrCat(impl()->name_, sep, unique_name),
253 false /* copy_names */));
254@@ -412,7 +422,7 @@ CompositeOpScopes Scope::GetCompositeOpScopes(
255 if (!impl()->single_use_scope()) {
256 Scope child = NewSubScope(impl()->op_name_.empty() ? composite_op_name
257 : impl()->op_name_);
258- const string child_op_sep = impl()->name_.empty() ? "" : "_";
259+ const string child_op_sep = impl()->name_.empty() ? "" : kSuffixSeparator;
260 const string child_name =
261 strings::StrCat(impl()->name_, child_op_sep, child.impl()->name_);
262 return {child,
263@@ -435,7 +445,13 @@ class InternalScope {
264 static Scope NewScope(Graph* graph, Status* status, ShapeRefiner* refiner) {
265 Scope::Impl::NameMap* name_map = new Scope::Impl::NameMap;
266 for (const Node* node : graph->nodes()) {
267- (*name_map)[node->name()] = 0;
268+ const string& name = node->name();
269+ (*name_map)[name] = 0;
270+ // Add all name prefixes ('/' separated).
271+ size_t idx = -1;
272+ while ((idx = name.find(kScopeSeparator, idx + 1)) != string::npos) {
273+ (*name_map)[name.substr(0, idx)] = 0;
274+ }
275 }
276 // We provide null destructors for these shared ptrs (except for name_map)
277 // since the caller owns them and doesn't want the scope to destroy them.
278diff --git a/tensorflow/cc/framework/scope_internal.h b/tensorflow/cc/framework/scope_internal.h
279index 8efcfed20d0b8..58adaef2e942a 100644
280--- a/tensorflow/cc/framework/scope_internal.h
281+++ b/tensorflow/cc/framework/scope_internal.h
282@@ -34,8 +34,7 @@ class Scope::Impl {
283 // name that has not been used so far in a scope will get no suffix. Later
284 // uses of the same name will get suffixes _1, _2, _3, etc. Multiple scopes
285 // can share the same NameMap. For instance, a new scope created using
286- // WithControlDependencies() should would share the same NameMap with the
287- // parent.
288+ // WithControlDependencies() would share the same NameMap with the parent.
289 typedef std::unordered_map<string, int> NameMap;
290
291 Impl(const std::shared_ptr<Graph>& graph,
292diff --git a/tensorflow/cc/framework/scope_test.cc b/tensorflow/cc/framework/scope_test.cc
293index 9eca9d3face34..b40b345eb8423 100644
294--- a/tensorflow/cc/framework/scope_test.cc
295+++ b/tensorflow/cc/framework/scope_test.cc
296@@ -26,6 +26,16 @@ TEST(ScopeTest, BasicNames) {
297 EXPECT_EQ(root.GetUniqueNameForOp("mul"), "mul");
298 }
299
300+TEST(ScopeTest, OpAndScopeNameCollision) {
301+ Scope root = Scope::NewRootScope();
302+ EXPECT_EQ(root.GetUniqueNameForOp("foo"), "foo");
303+ EXPECT_EQ(root.GetUniqueNameForOp("foo"), "foo_1");
304+ EXPECT_EQ(root.GetUniqueNameForOp("foo_1"), "foo_1_1");
305+ EXPECT_EQ(root.GetUniqueNameForOp("foo_2"), "foo_2");
306+ EXPECT_EQ(root.GetUniqueNameForOp("foo"), "foo_3");
307+ EXPECT_EQ(root.GetUniqueNameForOp("foo_2"), "foo_2_1");
308+}
309+
310 TEST(ScopeTest, HierarchicalNames) {
311 Scope root = Scope::NewRootScope();
312 Scope child = root.NewSubScope("child");