[Debugger Plugin] Exit TensorBoard on SIGINT even with debugger enabled #975

caisq · 2018-02-15T18:11:13Z

No description provided.

chihuahua

Thank you @caisq for fixing this. I just have some nits!

chihuahua · 2018-02-15T20:54:57Z

tensorboard/plugins/debugger/README.md

+2. Send `SIGINT` to the TensorBoard process running the Debugger Plugin, e.g.,
+   by using `Ctrl+C`.
+
+# Limitations and Unknown Issues


nit: These are known (not unknown) issues.

chihuahua · 2018-02-15T20:55:35Z

tensorboard/plugins/debugger/README.md

-* Hitting Ctrl+C (issuing a SIGINT signal) might fail to terminate execution for a model that is instrumented with
-  `TensorBoardDebugWrapperSession` or its corresponding hook. The same limitation may be present in the tensorboard
-  process as well. In those cases, the user must manually
-  [kill](https://www.linux.com/learn/intro-to-linux/2017/5/how-kill-process-command-line) the processes.
 * The debugger dashboard does not yet support multiple users debugging at once.


Maybe change this phrasing to "debugger plugin" to match other mentions.

chihuahua · 2018-02-15T20:55:55Z

tensorboard/plugins/debugger/README.md

+
+# Limitations and Unknown Issues
+
+The Debugger Plugin has the following limitations and known issues. We plan to


nit: Maybe lowercase debugger plugin to match other uses.

chihuahua · 2018-02-15T20:56:26Z

tensorboard/plugins/debugger/README.md

-# Limitations
+# Frequently Asked Questions (FAQ)
+
+## Q: How to exit the debugging?


Maybe: How to exit debugging?

chihuahua · 2018-02-15T20:57:16Z

tensorboard/plugins/debugger/interactive_debugger_plugin.py

+      # let the debugged tensorflow runtime proceed past the current Session.run
+      # in the C++ layer and return to the Python layer, so the SIGINT handler
+      # registered there may be triggered.
+      for _ in xrange(len(self._debugger_data_server.breakpoints) +  1):


nit: Extra space before 1.

chihuahua · 2018-02-15T20:57:37Z

tensorboard/plugins/debugger/interactive_debugger_plugin.py

+      # registered there may be triggered.
+      for _ in xrange(len(self._debugger_data_server.breakpoints) +  1):
+        self._debugger_data_server.put_incoming_message(True)
+      # while True:  # DEBUG


Can we get rid of these debug comments?

caisq

Thanks for the review!

caisq · 2018-02-15T21:56:52Z

tensorboard/plugins/debugger/README.md

-# Limitations
+# Frequently Asked Questions (FAQ)
+
+## Q: How to exit the debugging?


caisq · 2018-02-15T21:57:37Z

tensorboard/plugins/debugger/README.md

+
+# Limitations and Unknown Issues
+
+The Debugger Plugin has the following limitations and known issues. We plan to


caisq · 2018-02-15T21:58:11Z

tensorboard/plugins/debugger/README.md

-* Hitting Ctrl+C (issuing a SIGINT signal) might fail to terminate execution for a model that is instrumented with
-  `TensorBoardDebugWrapperSession` or its corresponding hook. The same limitation may be present in the tensorboard
-  process as well. In those cases, the user must manually
-  [kill](https://www.linux.com/learn/intro-to-linux/2017/5/how-kill-process-command-line) the processes.
 * The debugger dashboard does not yet support multiple users debugging at once.


caisq · 2018-02-15T21:58:26Z

tensorboard/plugins/debugger/interactive_debugger_plugin.py

+      # registered there may be triggered.
+      for _ in xrange(len(self._debugger_data_server.breakpoints) +  1):
+        self._debugger_data_server.put_incoming_message(True)
+      # while True:  # DEBUG


caisq · 2018-02-15T21:58:30Z

tensorboard/plugins/debugger/interactive_debugger_plugin.py

+      # let the debugged tensorflow runtime proceed past the current Session.run
+      # in the C++ layer and return to the Python layer, so the SIGINT handler
+      # registered there may be triggered.
+      for _ in xrange(len(self._debugger_data_server.breakpoints) +  1):


nfelt

FWIW it appears based on grpc/grpc#3820 (comment) that we can make Ctrl+C successfully kill TensorBoard without the two-stage shutdown process by adding a signal handler for SIGINT that instead does os.kill(os.getpid(), signal.SIGTERM) i.e. it basically tells the OS to resend the signal as SIGTERM instead of SIGINT. I tried this locally and it does successfully kill TensorBoard with a single Ctrl+C (albeit now with a Terminated message, from the shell I think).

It's maybe worth checking if we can that this works on Windows, but overall I think it would be good to preserve the ability of a single Ctrl+C to kill TensorBoard if it all possible. It's less graceful of a shutdown but it's much easier for users. If we aren't going to do that, we should probably add logic to the startup message so that if the debugger plugin is active, the message no longer says Ctrl+C alone will kill TensorBoard - otherwise I think users will rightfully consider it a bug.

nfelt · 2018-02-15T19:38:13Z

tensorboard/plugins/debugger/interactive_debugger_server_lib.py

@@ -524,6 +524,9 @@ def start_the_debugger_data_receiving_server(self):
    """
    self.run_server()

+  def stop_the_debugger_data_receiving_server(self):
+    self.stop_server()


Is this alias really necessary? The callsite could just call stop_server() directly, no?

Good point. Removed the stop_the_debugger_data_receiving_server and start_the_debugger_data_receiving_server methods.

nfelt · 2018-02-15T21:58:38Z

tensorboard/plugins/debugger/interactive_debugger_plugin.py

+
+    signal.signal(signal.SIGINT, self.signal_handler)
+
+  def signal_handler(self, unused_signal, unused_frame):


It might be good to note somewhere that this is required because of arguably a deficiency in grpc/python 2.7:
grpc/grpc#6999 (They marked it as wontfix somewhat unreasonably, I think, given that the debugger plugin is exactly that kind of "rare case" where this has an effect in the real world.) E.g. in python 3, Ctrl+C will successfully kill TensorBoard (actually, it requires two of them in a row, but still it's closer).

Added comments. Thanks.

nfelt · 2018-02-15T22:07:12Z

One other thing: should we also be adding this handler to the non-interactive debugger_plugin.py? Or is it not affected by this issue?

caisq · 2018-02-15T22:10:13Z

@nfelt, you wrote:

I tried this locally and it does successfully kill TensorBoard with a single Ctrl+C (albeit now with a Terminated message, from the shell I think).

Did you try it from a tensorboard binary (without bazel), or with bazel run? I think bazel run handles SIGINT differently, it'll run the binary as a subprocess and kill it no matter what.

nfelt · 2018-02-15T22:23:13Z

I was running bazel-bin/tensorboard/tensorboard directly, not under bazel run.

caisq · 2018-02-16T21:50:32Z

@nfelt

About the non-interactive debugger_plugin.py, those are used mainly during the debugging of long-running (possibly remotely running training) jobs. So interactive termination of those are not as important.

Thanks for the info about the os.kill(os.getpid(), signal.SIGTERM) approach. I prefer the current sys.exit() approach because it leads to a more orderly shutdown (without any scary-looking stack traces, for one thing).

caisq · 2018-02-16T22:14:55Z

Thank you both for the review, @chihuahua and @nfelt. I'm going to merge the PR now. If you have any remaining comments, please let me know and I'll be happy to address them in future PRs.

nfelt · 2018-02-16T23:18:39Z

@caisq In general, I think it's important that if starting TensorBoard advertises that you can kill it with Ctrl+C, we need to be able to kill TensorBoard with Ctrl+C in all expected use cases, without a special procedure that you need to read the FAQ (of a specific plugin, no less) in order to discover. (And for the same reason, I am inclined to think the non-interactive debugger_plugin.py matters too.)

Testing out this PR, however, I'm able to Ctrl+C from TensorBoard only and it seems to work without having to Ctrl+C the TF job as well (this is using your tdp_client binary in experimental). But it sounds like there are circumstances where the TF job will get stuck? If there's a way to avoid those cases so that Ctrl+C on the TF job isn't required in any case, that's still what I would prefer.

If we can't do that, I'm still kind of inclined to do os.kill(), maybe as a fallback (say, we wait 3 seconds after handling the SIGINT to see if we can shutdown gracefully, otherwise we do SIGTERM). At least in my testing I'm not seeing that producing any stacktraces, though if it did that also isn't a dealbreaker in my opinion (it's typical for Ctrl+C on a python process to produce a stacktrace from the KeyboardInterrupt bubbling up; this happens even with Ctrl+C on python -c 'while True: pass'). And I'm not sure I'm understanding if there are concrete benefits to being more "orderly" about the shutdown - unless an abrupt shutdown might actually disrupt the TF job, but then that seems like a more general problem, since TensorBoard might still crash for other reasons (OOM, etc.) and bringing down the TF job with it a result seems rather fragile.

[Debugger Plugin] Exit TensorBoard on SIGINT even with debugger enabled

bf6f927

caisq requested a review from chihuahua February 15, 2018 18:11

caisq added the plugin:debugger label Feb 15, 2018

caisq added 2 commits February 15, 2018 13:41

fix typo

91bb965

Fix doc string.

d54451d

chihuahua approved these changes Feb 15, 2018

View reviewed changes

caisq commented Feb 15, 2018

View reviewed changes

chaser

0ec2773

nfelt reviewed Feb 15, 2018

View reviewed changes

Address @nfelt's comments

af2656b

caisq merged commit 8b30876 into tensorflow:master Feb 16, 2018

nfelt mentioned this pull request Mar 5, 2018

CTRL+C not working with debugger_port #1016

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Debugger Plugin] Exit TensorBoard on SIGINT even with debugger enabled #975

[Debugger Plugin] Exit TensorBoard on SIGINT even with debugger enabled #975

caisq commented Feb 15, 2018

chihuahua left a comment

chihuahua Feb 15, 2018

chihuahua Feb 15, 2018

caisq Feb 15, 2018

chihuahua Feb 15, 2018

caisq Feb 15, 2018

chihuahua Feb 15, 2018

caisq Feb 15, 2018

chihuahua Feb 15, 2018

caisq Feb 15, 2018

chihuahua Feb 15, 2018

caisq Feb 15, 2018

caisq left a comment

caisq Feb 15, 2018

caisq Feb 15, 2018

caisq Feb 15, 2018

caisq Feb 15, 2018

caisq Feb 15, 2018

nfelt left a comment

nfelt Feb 15, 2018

caisq Feb 16, 2018

nfelt Feb 15, 2018

caisq Feb 16, 2018

nfelt commented Feb 15, 2018

caisq commented Feb 15, 2018

nfelt commented Feb 15, 2018

caisq commented Feb 16, 2018

caisq commented Feb 16, 2018

nfelt commented Feb 16, 2018


		# Limitations and Unknown Issues

		The Debugger Plugin has the following limitations and known issues. We plan to


		signal.signal(signal.SIGINT, self.signal_handler)

		def signal_handler(self, unused_signal, unused_frame):

[Debugger Plugin] Exit TensorBoard on SIGINT even with debugger enabled #975

[Debugger Plugin] Exit TensorBoard on SIGINT even with debugger enabled #975

Conversation

caisq commented Feb 15, 2018

chihuahua left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caisq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfelt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfelt commented Feb 15, 2018

caisq commented Feb 15, 2018

nfelt commented Feb 15, 2018

caisq commented Feb 16, 2018

caisq commented Feb 16, 2018

nfelt commented Feb 16, 2018