Skip to content

check invalid string type for dest_nodes in extract_sub_graph - #13057

Merged
drpngx merged 7 commits into
tensorflow:masterfrom
facaiy:ENH/check_string_input
Sep 24, 2017
Merged

drpngx merged 7 commits into
tensorflow:masterfrom
facaiy:ENH/check_string_input

Conversation

@facaiy

@facaiy facaiy commented Sep 15, 2017

Copy link
Copy Markdown
Member

Fix #13047.

How to test

  • add an unit test.
  • pass all tests.

@drpngx

drpngx commented Sep 16, 2017

Copy link
Copy Markdown
Contributor

Jenkins, test this please.

@drpngx
drpngx requested a review from reedwm September 17, 2017 00:00
@drpngx

drpngx commented Sep 17, 2017

Copy link
Copy Markdown
Contributor

@reedwm mind taking a look?

@drpngx drpngx added the awaiting review Pull request awaiting review label Sep 17, 2017
# Github 13047, cast single string to list
if isinstance(dest_nodes, str):
dest_nodes = [dest_nodes]
logging.warning("dest_nodes should be a list.")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Raise a TypeError instead. We shouldn't have a special case for passing in a string.

raise TypeError("graph_def must be a graph_pb2.GraphDef proto.")

# Github 13047, cast single string to list
if isinstance(dest_nodes, str):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use six.string_types instead of str. See saver.py for an example.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hi, @reedwm . If string is invalid, how about raising TypeError if only dest_nodes is not list? Please take a look at the latest commit, thanks.

if not isinstance(graph_def, graph_pb2.GraphDef):
raise TypeError("graph_def must be a graph_pb2.GraphDef proto.")

# Github 13047, cast single string to list

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this comment is necessary.

@facaiy
facaiy force-pushed the ENH/check_string_input branch 3 times, most recently from c12e52c to 0676b05 Compare September 19, 2017 04:55
@facaiy
facaiy force-pushed the ENH/check_string_input branch from 0676b05 to 8151f3b Compare September 19, 2017 05:24
if not isinstance(graph_def, graph_pb2.GraphDef):
raise TypeError("graph_def must be a graph_pb2.GraphDef proto.")

if not isinstance(dest_nodes, list):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This will break existing code where dest_nodes is a non-list iterable, such as a tuple or set. Instead, you should raise an error if its not an instance of a collections.Iterable, or is an instance of six.string_types as I described above.

self.assertEqual("n5", sub_graph.node[3].name)

def testExtractSubGraphWithInvalidDestNodes(self):
# Github issue, 13047

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove this comment.

@caisq caisq added stat:awaiting response Status - Awaiting response from author and removed awaiting review Pull request awaiting review labels Sep 19, 2017
@facaiy

facaiy commented Sep 22, 2017

Copy link
Copy Markdown
Member Author

How about taking a test? Thanks.

@reedwm

reedwm commented Sep 22, 2017

Copy link
Copy Markdown
Contributor

Jenkins, test this please.

@facaiy

facaiy commented Sep 22, 2017 •

Copy link
Copy Markdown
Member Author

This test failure seems unrelated.

rm: cannot remove '/workspace/tensorflow/contrib/makefile/gen/dep/tensorflow/contrib/makefile/downloads/fft2d/fftsg.d': Permission denied
make: *** [clean] Error 1
Build step 'Execute shell' marked build as failure
[Set GitHub commit status (universal)] ERROR on repos [] (sha:7702103) with context:tensorflow-pull-requests-makefile
Unable to get pull request builder trigger!!

@reedwm

reedwm commented Sep 22, 2017

Copy link
Copy Markdown
Contributor

Jenkins, test this please.

@facaiy facaiy changed the title ENH: cast single string to list for extract_sub_graph check invalid string type for dest_nodes in extract_sub_graph Sep 24, 2017
@drpngx
drpngx merged commit fe3a2e6 into tensorflow:master Sep 24, 2017
@drpngx

drpngx commented Sep 24, 2017

Copy link
Copy Markdown
Contributor

Yay! All tests passed.

@drpngx drpngx removed the stat:awaiting response Status - Awaiting response from author label Sep 24, 2017
@facaiy
facaiy deleted the ENH/check_string_input branch September 24, 2017 23:57
caisq added a commit to caisq/tensorflow that referenced this pull request Sep 26, 2017
END_PUBLIC

---
Commit 1e1b3d9 authored by Pete Warden<[email protected]>
Committed by gunan<[email protected]>:
Changed output directory for Pi CI build to fix permissions problem with nightlies (tensorflow#13257)

* Fix for RTLD_GLOBAL breakage of Pi builds, and removed Eigen version change for Pi that's no longer needed

* Fixed Pi Zero OpenBLAS build problems and tidied up directories used

* More robust checks in Pi build script

* Changed output directory for Pi CI build to fix permissions problem

---
Commit fe3a2e6 authored by Yan Facai (???)<[email protected]>
Committed by drpngx<[email protected]>:
check invalid string type for dest_nodes in extract_sub_graph (tensorflow#13057)

* BUG: check str type

* TST: add unit test

* CLN: remove list check

* CLN: use warning

* CLN: 2 indent

* CLN: raise TypeError if not list

* CLN: check string only

---
Commit 225ab76 authored by Jean Wanka<[email protected]>
Committed by Jean Wanka<[email protected]>:
Fix polynomial decay with cycle for global step=0

For polynomial decay with cycle=True the learning rate at
step 0 becomes NaN, because in the process of calculating it we
devide by 0. This change should fix it, by setting the multiplier
for the decay steps to one for global_step=0.

---
Commit 286f570 authored by Bjarke Hammersholt Roune<[email protected]>
Committed by TensorFlower Gardener<[email protected]>:
Make Service::TransferToClient not attempt to manipulate the literal when the transfer failed, preventing a crash and allowing the caller to see the reason for the failed transfer.

PiperOrigin-RevId: 169770126

---
Commit e0501bc authored by Yong Tang<[email protected]>
Committed by Shanqing Cai<[email protected]>:
Fix GRUBlockCell parameter naming inconsistency (tensorflow#13153)

* Fix GRUBlockCell parameter naming inconsistency

This fix tries to fix the issue in 13137 where
parameter `cell_size` is used instead of `num_units`.
This is inconsistent with other RNN cells.

This fix adds support of `num_units` while at the same
time maintains backward compatiblility for `cell_size`.

This fix fixes 13137.

Signed-off-by: Yong Tang <[email protected]>

* Add `@deprecated_args` for 'cell_size' in `GRUBlockCell`

This commit adds `@deprecated_args` for 'cell_size' in `GRUBlockCell`

Signed-off-by: Yong Tang <[email protected]>

* Address review comment

Signed-off-by: Yong Tang <[email protected]>

---
Commit 02a2eba authored by Pete Warden<[email protected]>
Committed by gunan<[email protected]>:
Fix for RTLD_GLOBAL breakage of Pi builds, and removed Eigen version change that's no longer needed (tensorflow#13251)

* Fix for RTLD_GLOBAL breakage of Pi builds, and removed Eigen version change for Pi that's no longer needed

* Fixed Pi Zero OpenBLAS build problems and tidied up directories used

* More robust checks in Pi build script

---
Commit 8ef7222 authored by Sanjoy Das<[email protected]>
Committed by TensorFlower Gardener<[email protected]>:
Remove a redundant setName.

The EmitComputation should have emitted a function with the right name, so use a
CHECK instead.

PiperOrigin-RevId: 169764856

---
Commit 1b94147 authored by Neal Wu<[email protected]>
Committed by TensorFlower Gardener<[email protected]>:
Fix broken GitHub links in tensorflow and tensorflow_models resulting from The Great Models Move (a.k.a. the research subfolder)

PiperOrigin-RevId: 169763373

---
Commit b1ada5f authored by Justine Tunney<[email protected]>
Committed by TensorFlower Gardener<[email protected]>:
Fix TensorBoard python -m invoke in docs

PiperOrigin-RevId: 169758752

---
Commit 2957cd8 authored by Mustafa Ispir<[email protected]>
Committed by TensorFlower Gardener<[email protected]>:
Local run option of estimator training.

PiperOrigin-RevId: 169756384

---
Commit 1dc2fe7 authored by Gunhan Gulsoy<[email protected]>
Committed by TensorFlower Gardener<[email protected]>:
BEGIN_PUBLIC
Automated g4 rollback of changelist 166264198

PiperOrigin-RevId: 169998124
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants