Implement --remove-tagged-cells #738
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Thanks @astrofrog (nice to see you here). It looks like some of the CLI tests still need updating. It also looks like botocore is being grumpy with python 3.12. |
|
Thanks @astrofrog -- on the botocore build part I am working on getting a new release out anyway and need to fix dep updates since the last one regardless. |
|
Looks like tests are failing now due to some test scaffolding including the input_path in the arguments. |
papermill/cli.py
Outdated
| @@ -61,6 +61,9 @@ def print_papermill_version(ctx, param, value): | |||
| @click.option( | |||
| '--parameters_base64', '-b', multiple=True, help='Base64 encoded YAML string as parameters.' | |||
| ) | |||
| @click.option( | |||
| '--remove-tagged-cells', type=str, help='Remove cells with the specified tag before execution.' | |||
There was a problem hiding this comment.
What about
| '--remove-tagged-cells', type=str, help='Remove cells with the specified tag before execution.' | |
| '--remove-cells-tagged', type=str, help='Remove cells with the specified tag before execution.' |
Then it'd be pretty fluent on the command line:
--remove-cells-tagged=a-tag
| @@ -104,6 +109,8 @@ def execute_notebook( | |||
| ) | |||
|
|
|||
| nb = prepare_notebook_metadata(nb, input_path, output_path, report_mode) | |||
| if remove_tagged_cells is not None: | |||
There was a problem hiding this comment.
| if remove_tagged_cells is not None: | |
| if remove_tagged_cells: |
should be enough – the empty string isn't a valid tag, right..?
| # Copy the notebook to avoid changing the input one | ||
| nb = deepcopy(nb) |
There was a problem hiding this comment.
E.g. remove_error_markers modifies the notebook in place...
|
Is there something I could do to move this PR forward, having been the instigator of #429 back in 2019? 😁 (We've used a custom fork of Papermill in the meantime, but it's fallen quite out of repair, so it'd be nice to use the upstream version.) |
|
Any updates on this? I am also interested in this feature and will appreciate push on it |
|
What is needed to get this over the finish line? Would love to use this feature and willing to help out if possible 😀 |
|
Ping @MSeal. Do you have time to reboot this? Thanks. |
|
If it's helpful - I have this branch which builds off of this PR with @akx's feedback merged + fixing the failing tests. |
This implements a new --remote-tagged-cells option as described by @MSeal in #429 (comment). The idea of the current implementation is to keep it as simple as possible – only being able to specify a single tag and requiring an exact match (no regular expressions etc). My personal use case for this is being able to run notebooks with/without diagnostic plotting but there are other use cases described in #429.
If the maintainers are on board with this PR so far, I can add documentation and anything else required.
One thought I had was that perhaps this option would read more clearly as
--remove-cells-with-tagotherwise it sounds like a boolean option, and also doesn't make it clear that a single tag is what should be passed in. What do people think about this?