Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement --remove-tagged-cells #738

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

astrofrog
Copy link

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-tag otherwise 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?

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@willingc
Copy link
Member

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.

@MSeal
Copy link
Member

MSeal commented Nov 15, 2023

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.

@MSeal
Copy link
Member

MSeal commented Nov 16, 2023

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.'
Copy link
Member

Choose a reason for hiding this comment

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

What about

Suggested change
'--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:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if remove_tagged_cells is not None:
if remove_tagged_cells:

should be enough – the empty string isn't a valid tag, right..?

Comment on lines +187 to +188
# Copy the notebook to avoid changing the input one
nb = deepcopy(nb)
Copy link
Member

Choose a reason for hiding this comment

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

E.g. remove_error_markers modifies the notebook in place...

@akx
Copy link
Member

akx commented Dec 18, 2023

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.)

@andreytaboola
Copy link

andreytaboola commented Jul 3, 2024

Any updates on this? I am also interested in this feature and will appreciate push on it
Thank you

@wd60622
Copy link

wd60622 commented Sep 19, 2024

What is needed to get this over the finish line? Would love to use this feature and willing to help out if possible 😀

@willingc
Copy link
Member

willingc commented Oct 5, 2024

Ping @MSeal. Do you have time to reboot this? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants