Added new features #23

beasteers · 2017-10-28T17:35:42Z

I expanded the functionality a bit.

Added line magic so that notifications can be triggered mid-cell
Added option to use the cell output as the notification message
Added autonotify magic, which lets users automatically send notifications if a cell execution takes longer than x seconds

mdagost · 2017-10-29T18:06:18Z

@bensteers Awesome! Thanks for this! Will take a look at it early this week.

mdagost · 2017-11-02T21:08:18Z

@bensteers These features look really, really awesome. Thank you so much! I'm going to try to get a detailed code review done today or tomorrow.

mdagost · 2017-11-02T21:09:08Z

jupyternotify/jupyternotify.py

@@ -2,16 +2,24 @@
 # for more details on the implementation here
 import json
 import uuid
+import time


We have a thing about alphabetizing imports. Could you put this import before import uuid please?

mdagost · 2017-11-02T21:09:49Z

jupyternotify/jupyternotify.py


 from IPython.core.getipython import get_ipython
-from IPython.core.magic import cell_magic, Magics, magics_class
+from IPython.core.magic import cell_magic, Magics, magics_class, line_cell_magic, line_magic


Same comment about alphabetizing: could you make it cell_magic, line_cell_magic, line_magic, Magics, magics_class?

beasteers · 2017-11-03T04:42:04Z

Glad you like! I alphabetized the imports. Let me know of any other changes.

mdagost · 2017-11-03T15:14:28Z

@bensteers @htorrence is going to do a full code review.

htorrence

@bensteers This looks great, just a couple of small questions/comments before we merge it in!

htorrence · 2017-11-03T16:48:03Z

jupyternotify/jupyternotify.py

+        "--output", action='store_true',
+        help="Use last output as message"
+    )
+    @line_magic


Why is autonotify line magic rather than cell magic? Since it depends on the run time of the cell it seems like it would make more sense as cell magic

It's line magic because you only specify it once, at the top of your notebook, in a similar vein to how you use %matplotlib inline to make plots show in your notebook. Then any cell that runs over a certain amount of time will issue a notification. If it were cell magic, it would imply that autonotify is only applied to that cell.

I basically wrote autonotify because I didn't want to have to write %%notify at the top of every one of my cells that I want notifications for.

ah okay, that makes sense

htorrence · 2017-11-03T16:53:19Z

jupyternotify/jupyternotify.py

+                pass # can't convert to string. Use default message
+
+        # allow notify to stop autonotify
+        if not self.__class__.notification_uuid: return


let's put return and pass on the next line for PEP8 compliance

lol yep. lazy late night coding :p

htorrence · 2017-11-03T17:17:40Z

README.md

+```
+
+
+## Automatically trigger notification after a certain cell execution time.


Let's lose the period to mirror the other sections

htorrence · 2017-11-03T17:17:54Z

README.md

+```
+`autonotify` also takes the arguments `--message` / `-m` and `--output` / `-o`.
+
+## Use cell output as message.


Again, let's lose the period in the title

htorrence · 2017-11-03T17:21:18Z

README.md

+```python
+import numpy as np
+import time
+# autonotify after 30 seconds


I initially thought this would notify at 30 seconds if execution was still ongoing - that seems like the preferred behavior to me, but I'm not sure how to implement that. If notification at the requested time is not possible I'd clarify the comment here that the notification only occurs when the cell finishes.

No, I definitely think the current implementation is the way to go. The idea is that, when I run a cell that takes 2 minutes to run (or 40 minutes which was the case last week), I want to know when the cell has finished executing so that I don't have to keep leaving Netflix to see if it's done yet.

If you wanted that functionality (notification after 30s), we could probably work that out. I think you would just have to create a separate thread in pre_run_cell that just waits 30 seconds to send the message, but I'm not really sure how useful that would be.

I was worried about the explanation not being clear, so I can try and further clarify the distinction.

I'm thinking that using --after might be a confusing option name and does kind of imply that it would trigger after 30 seconds regardless of whether the cell has finished running or not. I'm not sure what would be better to use though. Let me know if you have any ideas

htorrence · 2017-11-03T19:24:23Z

LGTM!

beasteers · 2017-11-03T19:31:35Z

yay! Thanks for the review!!

Darel13712 · 2017-11-08T14:28:23Z

jupyternotify/jupyternotify.py

+        default="Cell Execution Has Finished!!",
+        help="Custom notification message"
+    )
+    @line_magic


@bensteers Hey there. I know it's already merged, but do you think it is possible to make autonotification appear if total amount of time for all currently runinng cells exceeds threshold and not just one cell? It would be more useful for little thresholds if you have multiple cells running -- every quite fast but together they should be notified and it is more convenient for multiple big cells so you get only one notification when they all are finished?

Hm, yeah that's a good idea. I don't think I know enough about Jupyter development to know how to do this off-hand. Do you know if IPython has a cell queue that we can access? It currently hooks into events that run before and after any cell execution, but as it is, it's totally naive about the cell queue.

No, I'm afraid I don't know about IPython much too. May be @mdagost have some ideas?

Also do you know if it is possible to get cell-id so we could scroll to the executed cell on finish?

I'm not finding anything in the IPython docs about getting the current cell id or a list of the queued cells. I don't really have the time to find where that information is stored as I'm swamped with school work, but if you're able to locate it, I can most likely implement what you're requesting.

I'd imagine it's located somewhere in the IPython interactive shell object (ip = get_ipython()) though that's just a guess. ip docs.

Ben Steers added 10 commits October 28, 2017 02:08

added line magic

613435d

auto-notify after x seconds

b917be2

notify using cell output

e62f08f

added examples

703b21f

fixed my typo

a6f2770

small tweaks and fixes

d457faf

fixed syntax highlighting and updated description

bc00df1

small example change to improve clarity

b8e5dea

added check to make sure output could be converted to string

eacf4c1

Merge branch 'master' of https://github.com/bensteers/jupyter-notify

3a7ad37

mdagost reviewed Nov 2, 2017

View reviewed changes

alphabetized imports

aa24008

htorrence suggested changes Nov 3, 2017

View reviewed changes

Ben Steers added 2 commits November 3, 2017 15:08

fix pep8

effa9ff

readme fixes

e42a1d2

htorrence approved these changes Nov 3, 2017

View reviewed changes

mdagost merged commit 6dd1e3b into ShopRunner:master Nov 3, 2017

Darel13712 reviewed Nov 8, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added new features #23

Added new features #23

		```


		## Automatically trigger notification after a certain cell execution time.

Added new features #23

Added new features #23

Conversation

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

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