Add option for hiding input #135

cloud-princess · 2018-05-10T15:56:29Z

Have made an attempt. Please tell me what to change/add. Thanks!

cloud-princess · 2018-05-10T15:58:38Z

rgbkrk · 2018-05-10T19:43:35Z

papermill/execute.py

@@ -147,7 +147,8 @@ def execute_notebook(notebook,
                     kernel_name=None,
                     progress_bar=True,
                     log_output=False,
-                     start_timeout=60):
+                     start_timeout=60,
+                     report-mode=None):


This should be an underscore, otherwise this is a subtraction.

rgbkrk · 2018-05-10T19:43:54Z

papermill/execute.py


    Returns:
         nb (NotebookNode): executed notebook object
    """
    print("Input Notebook:  %s" % get_pretty_path(notebook), file=sys.stderr)
    print("Output Notebook: %s" % get_pretty_path(output), file=sys.stderr)
    nb = load_notebook_node(notebook)
+
+    # Hide input if report-mode is set to True.
+    if report-mode:


report_mode here too.

cloud-princess · 2018-05-10T20:44:39Z

ah right! Will fix. Thanks!

cloud-princess · 2018-05-10T20:57:32Z

hopefully it is better now

codecov · 2018-05-10T20:58:01Z

Codecov Report

Merging #135 into master will increase coverage by 0.08%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #135      +/-   ##
==========================================
+ Coverage   73.58%   73.66%   +0.08%     
==========================================
  Files           8        8              
  Lines         935      938       +3     
==========================================
+ Hits          688      691       +3     
  Misses        247      247

cloud-princess · 2018-05-10T21:11:19Z

hm... what else is wrong?

rgbkrk · 2018-05-10T23:38:40Z

It's only reporting an error because coverage has gone down (no tests on this new command). I'll have to defer to one of the primary pythonistas for what they'd want next as well as style on the command. Thanks for submitting this!

cloud-princess · 2018-05-10T23:49:40Z

Alright! Looking forward to seeing what is done next!

MSeal · 2018-05-11T16:19:43Z

Two things to add now I think:

If we're adding an option to the execute command it should also be on CLI. The cli.py file should be straight forward to manipulate and add a new flag (you can copy one of the adjacent flags properties).

The second item is tests. We will want a test in the test_execute.py and test_cli.py to test that we can set the option and that it will change the metadata of an output notebook when it executes that notebook.

MSeal · 2018-05-11T16:20:58Z

papermill/execute.py

+
+    # Hide input if report-mode is set to True.
+    if report_mode:
+       nb.metadata.inputHidden = True


@mpacer I saw some of the back-and-forth on jupyter threads about hidden inputs, is this the metadata field we want papermill to populate for other tools going forward?

cloud-princess · 2018-05-12T00:01:09Z

will look into the error tmr!

MSeal · 2018-05-12T14:10:29Z

papermill/cli.py

@@ -36,6 +36,8 @@
              help="Flag for writing notebook output to stderr.")
 @click.option('--start_timeout', type=int, default=60,
              help="Time in seconds to wait for kernel to start.")
+@click.option('--report-mode/--not-report-mode', default=None,


You'll want this to be default=False

MSeal · 2018-05-12T14:10:54Z

papermill/execute.py

@@ -147,7 +147,8 @@ def execute_notebook(notebook,
                     kernel_name=None,
                     progress_bar=True,
                     log_output=False,
-                     start_timeout=60):
+                     start_timeout=60,
+                     report_mode=None):


I'd do report_mode=False here too as it's a flag

cloud-princess · 2018-05-13T18:43:56Z

Have attempted writing tests in both test_execute.py and test_cli.py.

cloud-princess · 2018-05-13T18:50:57Z

I have read the errors(EDIT: this was written before squashing), and the following is my understanding:

test_cli.py
it says that the arguments in the call aren't consistent with the expected arguments but I see no difference - what am I missing?
test_execute.py
the test notebook 'report_mode_test.ipynb' is not found in the path because I've only added the notebooks in my branch instead of nteract/papermill/... - presumably that's an error I can't overcome?

The input and output notebooks I've made may or may not make a very good test case...

Thanks!

MSeal · 2018-05-21T21:33:38Z

Try pasting the delta into a diff tool, should pop up quickly. Also try adding a debug point (or print statements) in the execute call to make sure you're getting expected values. You can isolate to a single test with https://stackoverflow.com/questions/36456920/is-there-a-way-to-specify-which-pytest-tests-to-run-from-a-file to narrow down the issue. I don't see the problem from a peripheral scan of the code.

MSeal · 2018-05-21T21:35:52Z

For test_execute you do not need to have the notebooks pushed anywhere, just make sure they're in the repo branch and added. Check that self.notebook_path is getting set to something correct, but I don't see any obvious issue there either so it's probably a silly typo error someplace.

cloud-princess · 2018-05-22T17:26:29Z

Yes, it was a typo. Thanks!!

MSeal · 2018-05-22T18:15:16Z

Awesome, two things left. First, let's squash these commits down. Second is we're discussing this week with a few nteract team members what the best metadata / library contract should be to respect this report mode. Specifically what should the metadata field look like and how do we want nteract exporters to respect metadata contracts moving forward. Today I don't believe any tools will respect nb.metadata.inputHidden = True, and I'm assuming you want nteract html exporters to respect this field?

rgbkrk · 2018-05-22T21:41:41Z

The original jupyter discussion about hidden input was here: jupyter/notebook#534

nteract frontends all support both inputHidden and hide_input https://github.com/nteract/nteract/pull/1697/files

I totally forgot that at the last dev meetings we ended up on a nested path:

https://github.com/jupyter/nbformat/pull/94/files

metadata.jupyter.source_hidden and metadata.jupyter.outputs_hidden

Let's go with that officially merged stance on nbformat and I'll make the necessary changes to nteract to also set those. 🙃

cloud-princess · 2018-05-23T13:40:54Z

Yes I do want html exporters to respect the field. Ok! Thanks. I'll squash and go through the links!

mpacer · 2018-05-24T16:56:41Z

@cldssty As long as we're namespacing the metadata to provide this functionality — I'm cool with that. With such a change I'm ok with this PR.

Note that that spec that @rgbkrk linked to is a spec on a per cell basis, and so there is no one field to set for the whole notebook.

More generally though, think we should consider the long-term plan for additional customisation (like report-mode) for a couple of reasons. First, the proposed solution will be respected by the nteract front-end and if you want a static html page (which is what I thought you wanted) you'd need to use commuter (which I think supports this?).

As of right now, source_hidden is not respected in nbconvert as there was some disagreement on whether that hidden state was actually intended to be the mechanism for specifying this in static exports as opposed to only in the interactive view of the notebook. Right now there is no --no-input flag in nbconvert, but I plan to make a PR for that later today (as I mentioned in the issue associated with this PR… there is already input removal enabled via the TemplateExporter.exclude_input). See spatialaudio/nbsphinx#65 (comment) for more context.

Part of the reason why there was disagreement around the term hide_input is whether that is intended to remove input altogether or make it so that its visibility can be toggled back and forth (see
https://mpacer.github.io/hiding_tags_nbconvert/hide_cells_based_on_tags.html for an example, for the code see https://github.com/mpacer/hiding_tags_nbconvert ).

MSeal · 2018-05-30T21:42:45Z

@cldssty Let us know if you get stuck in the final PR updates

Feature mentioned in nteract/papermill#135 (comment)

MSeal · 2018-07-18T16:57:35Z

@cldssty Do you think you'll have some time to complete this PR or should we make final touches to get it mergable?

cloud-princess · 2018-07-19T16:58:04Z

I'm really sorry for the delay! I'll be looking at this this weekend and will ask about any problems I'm encountering. Again really sorry about disappearing and thank you so much for your patience. Apologies for any inconvenience caused!!

MSeal · 2018-07-19T17:41:19Z

No problem, and no need to apologize. We wanted to get this nice capability added before the nteract sprints begin in a week so we'll give special attention to this PR and help with getting it finished.

cloud-princess · 2018-07-23T21:42:25Z

I've skimmed through the links and these are my thoughts:

I don't know why I assumed metadata.inputHidden it was a field for notebooks 2 months ago - but yes, it is a field for code cells.

~~If I stick to using a field for the whole notebook(instead of for each cell), then we'd need to~~

~~1. add the field in the nbformat schema?~~

~~2. then we should probably make it so that if this field is set to True, then for each new code cell created, the metadata.jupyter.source_hidden field is set to True?~~

~~Alternatively, I could just make this flag toggle input visibility on a cell-by-cell basis. In that case, I would need to change my code but won't need to add a field to nbformat schema?~~

EDIT: I'm going to go ahead and look into using metadata.jupyter.source_hidden and change my code so that I'm calling this field with cells, not notebooks. Thanks for all the tips.

In any case, since @mpacer says that source_hidden is not currently respected by nbconvert, we'll need to additionally somehow make this flag set TemplateExporter.exclude_input in nbconvert to True as well so that input is hidden for html exports? I need to look into the PR by @mpacer.

I have yet to find out how to do all of this. If there are any misunderstandings or things that cannot be done, please let me know!

Thanks :D

cloud-princess · 2018-07-24T15:43:41Z

Currently have something like this in execute.py:

# Hide input if report-mode is set to True.
if report_mode: 
    for cell in nb.cells:
        if cell.cell_type == 'code':
            cell.metadata.jupyter.source_hidden = True

cloud-princess · 2018-07-24T16:06:49Z

It's saying that it doesn't have the 'jupyter' key... should the key be added somewhere or...?

rgbkrk · 2018-07-24T16:38:32Z

If it doesn't have the jupyter key, go ahead and add it in, then apply source_hidden. We formalized on what the spec would be yet haven't started adopting it. If you set it here, we'll be extra motivated to follow through on the nteract frontend at least. 😄

We can leave it to nbconvert to implement it without bogging this PR down.

cloud-princess · 2018-07-24T18:14:21Z

ok shall I just squash the commits? Going to look into making this work with html exports in a bit more detail.

Oh wait, the field is now metadata.jupyter['source_hidden'] instead of metadata.jupyter.source_hidden so I should probably fix that.

cloud-princess · 2018-07-24T21:54:08Z

I'll keep working on this tmr.

This is what I've done so far:

changed field to metadata.jupyter['source_hidden'] for cells and it passed travis build.
currently trying to turn this into metadata.jupyter.source_hidden but so far my ideas aren't working.

What I've tried so far:

setting metadata.jupyter = lambda: None and then setting attributes source_hidden and outputs_hidden for that. The travis-ci errors didn't seem to show any direct cause of the failed build.
creating a custom object to set metadata.jupyter to. First, I'm not sure whether to put the class definition of this object in iorw.py or not, and secondly, I'm still trying to implement a JSON serialiser for it, but idk if this is the right route to go down.

Please advise! I will look into the exportation part of this and also if it gets too close to the nteract sprint and I still can't find a good solution, I would be happy to hand this over to someone who can wrap this up before the start of the sprint. You could specify a deadline for this if you like. Until then I'm going to keep trying!

Thanks!

MSeal · 2018-07-25T05:50:10Z

papermill/execute.py

+    if report_mode: 
+        for cell in nb.cells:
+            if cell.cell_type == 'code':
+                cell.metadata.jupyter.source_hidden = True


cell.metadata['jupyter'] = cell.get('jupyter', {}) cell.metadata.['jupyter']['source_hidden'] = True

MSeal · 2018-07-25T05:50:44Z

papermill/iorw.py

+        self.source_hidden = False
+        self.outputs_hidden = False
+
+class Dumper(Jupyter):


You don't need these two classes

MSeal · 2018-07-25T05:51:36Z

papermill/iorw.py

@@ -167,6 +190,9 @@ def load_notebook_node(notebook_path):

        if not hasattr(cell.metadata, 'papermill'):
            cell.metadata['papermill'] = dict()
+
+        if not hasattr(cell.metadata, 'jupyter'):


given the snippet I posted you can skip this check/assignment too

MSeal · 2018-07-25T05:53:00Z

papermill/tests/test_execute.py

+        nb = execute_notebook(self.notebook_path, self.nb_test_executed_fname, report_mode = True)
+        for cell in nb.cells: 
+            if cell.cell_type == 'code':  
+                self.assertEqual(cell.metadata.jupyter.source_hidden, True)


self.assertEqual(cell.metadata.get('jupyter', {}).get('source_hidden'), True)

MSeal · 2018-07-25T05:55:48Z

Thanks for coming back to wrap up the PR!

I think leaving metadata with primitive objects (dict/str) will uncomplicated the part you're stuck on and posted come comments to help with making that change. Give that change a whirl and I think you'll be good :)

cloud-princess · 2018-07-25T12:05:23Z

Changes applied and squashed. Thanks for the tips!!

rgbkrk · 2018-07-25T13:47:57Z

This is looking pretty good, I think you can take the WIP title off if you think it's ready. :)

cloud-princess · 2018-07-25T15:02:27Z

So is source_hidden going to be respected by html exporters if it gets adopted by nteract frontends and nbconvert, i.e. something that is probably going to be out of the scope of this PR?

I was going to do a bit more research later today to see if there is anything else I could do regarding html exports but if there isn't then yes I'm happy to take the WIP title off at this stage!

Thanks!

MSeal · 2018-07-25T15:25:25Z

The intention is yes. We'll need to make a PR to nbconvert to respect the same field as nteract. @mpacer has a PR (jupyter/nbconvert#825) which we can help get completed and merged.

cloud-princess · 2018-07-25T16:01:35Z

Thanks for all the tips and being helpful + patient! I'll be looking out for further development in related PRs!

cloud-princess mentioned this pull request May 10, 2018

Hide all input for output notebook #130

Closed

rgbkrk reviewed May 10, 2018

View reviewed changes

MSeal reviewed May 11, 2018

View reviewed changes

MSeal reviewed May 12, 2018

View reviewed changes

cloud-princess force-pushed the patch-1 branch from 81df42e to 43179e9 Compare May 13, 2018 21:14

mpacer added a commit to mpacer/nbconvert that referenced this pull request Jun 5, 2018

add --no-input flag, including new test

5cd757f

Feature mentioned in nteract/papermill#135 (comment)

cloud-princess force-pushed the patch-1 branch 3 times, most recently from ff43d8a to ce7118f Compare July 21, 2018 18:12

cloud-princess force-pushed the patch-1 branch 2 times, most recently from 5e80a6b to 2737a34 Compare July 24, 2018 21:46

MSeal reviewed Jul 25, 2018

View reviewed changes

cloud-princess force-pushed the patch-1 branch from 2737a34 to b6101f5 Compare July 25, 2018 12:00

Add option for hiding input

26799cc

cloud-princess force-pushed the patch-1 branch from b6101f5 to 26799cc Compare July 25, 2018 15:52

cloud-princess changed the title ~~[WIP] Add option for hiding input~~ Add option for hiding input Jul 25, 2018

MSeal approved these changes Jul 25, 2018

View reviewed changes

MSeal merged commit 3a35d8c into nteract:master Jul 25, 2018

cloud-princess deleted the patch-1 branch July 25, 2018 16:13

LeonardAukea mentioned this pull request Nov 13, 2018

Hiding Ingested Parameters when executing with --report-mode #253

Closed

jasongrout mentioned this pull request Mar 26, 2019

outputs_hidden vs collapsed metadata jupyter/nbformat#137

Open

Add option for hiding input #135

Add option for hiding input #135

Conversation

cloud-princess commented May 10, 2018

cloud-princess commented May 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-princess commented May 10, 2018

cloud-princess commented May 10, 2018

codecov bot commented May 10, 2018 • edited Loading

Codecov Report

cloud-princess commented May 10, 2018

rgbkrk commented May 10, 2018 • edited Loading

cloud-princess commented May 10, 2018

MSeal commented May 11, 2018

Choose a reason for hiding this comment

cloud-princess commented May 12, 2018

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

cloud-princess commented May 13, 2018 • edited Loading

cloud-princess commented May 13, 2018 • edited Loading

MSeal commented May 21, 2018

MSeal commented May 21, 2018

cloud-princess commented May 22, 2018

MSeal commented May 22, 2018

rgbkrk commented May 22, 2018 • edited Loading

cloud-princess commented May 23, 2018 • edited Loading

mpacer commented May 24, 2018

MSeal commented May 30, 2018

MSeal commented Jul 18, 2018

cloud-princess commented Jul 19, 2018

MSeal commented Jul 19, 2018

cloud-princess commented Jul 23, 2018 • edited Loading

cloud-princess commented Jul 24, 2018

cloud-princess commented Jul 24, 2018 • edited Loading

rgbkrk commented Jul 24, 2018

cloud-princess commented Jul 24, 2018 • edited Loading

cloud-princess commented Jul 24, 2018 • edited Loading

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

MSeal commented Jul 25, 2018

cloud-princess commented Jul 25, 2018 • edited Loading

rgbkrk commented Jul 25, 2018

cloud-princess commented Jul 25, 2018 • edited Loading

MSeal commented Jul 25, 2018

cloud-princess commented Jul 25, 2018

codecov bot commented May 10, 2018 •

edited

Loading

rgbkrk commented May 10, 2018 •

edited

Loading

cloud-princess commented May 13, 2018 •

edited

Loading

cloud-princess commented May 13, 2018 •

edited

Loading

rgbkrk commented May 22, 2018 •

edited

Loading

cloud-princess commented May 23, 2018 •

edited

Loading

cloud-princess commented Jul 23, 2018 •

edited

Loading

cloud-princess commented Jul 24, 2018 •

edited

Loading

cloud-princess commented Jul 24, 2018 •

edited

Loading

cloud-princess commented Jul 24, 2018 •

edited

Loading

cloud-princess commented Jul 25, 2018 •

edited

Loading

cloud-princess commented Jul 25, 2018 •

edited

Loading