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

Allow exporters to be enabled or disabled via config #1273

Merged
merged 3 commits into from
Jun 11, 2020

Conversation

ianabc
Copy link
Contributor

@ianabc ianabc commented May 27, 2020

This change allows people to configure which Exporters are enabled by adding a .enabled attribute/traitlet to the Exporter class. It allows users to control functionality such as the "Download as" entry in the notebook and addresses issues like #1065 or this discourse post.

To disable a single exporter, configuration looks like

c.ASCIIDocExporter.enabled = False

Or, since everything inherits from Exporter, you could disable all but one with

c.Exporter.enabled = True
c.NotebookExporter.enabled = True

People wanting to tweak the notebook "Download as" options those could go in their jupyter_notebook_config.py but to get complete control over those entries you also need a change to the notebook (e.g. this hack). In principal you could do everything there, but I felt that the disable/enable functionality was more general than just the notebook so I thought I'd ask for feedback here first.

Signed-off-by: Ian Allison <iana@pims.math.ca>
@meeseeksmachine
Copy link

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/remove-download-as-options-in-jupyter-notebook/4374/5

@@ -52,6 +52,8 @@ class Exporter(LoggingConfigurable):
accompanying resources dict.
"""

enabled = Bool(True).tag(config=True)
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a short "doc string"/help text to this to explain how/why people would use this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, thanks, I've added a one line help, if you think it needs more explanation I can add it.

@betatim
Copy link
Member

betatim commented May 27, 2020

Looks nice!

Do we have any data on how many exporters exist that don't inherit from Exporter and would not have this attribute? I was wondering if the selection loop should have or True in it for classes that don't have the traitlet in an attempt to not break existing code.

I like it but not very familiar with the nbconvert code and conventions to know what else we should look at/think about before merging.

ps. did you run this with lab and classic notebook to see it in action?

Signed-off-by: Ian Allison <iana@pims.math.ca>
@ianabc
Copy link
Contributor Author

ianabc commented May 27, 2020

All of the exporters in nbconvert/exporters seem to inherit from either Exporter or TemplateExporter (which inherits from TemplateExporter) so I think they are all covered. If there are other mechanisms for adding exporters I think I would need to extend the or True to work when people have explicitly set .enabled = False, maybe

[e for e in names if getattr(get_exporter(e)(config=c), 'enabled', True)] 

I'll leave this out for now, but if there are other exporters that seems to work OK.

I've been testing the changes in the notebook but they seem to work in jupyterlab as well. I haven't figured out exactly how lab builds it's list of exporters but it does seem to "just work". I'll try to dig into lab and make sure this is true in general.

@ianabc
Copy link
Contributor Author

ianabc commented May 27, 2020

The doc build failure seems to be related to nbconvert/docs/example.ipynb. Something goes wrong extracting the output as a png and then LaTeX chokes on the broken png file.

@MSeal
Copy link
Contributor

MSeal commented May 29, 2020

Yes the docs build failure I need to go fix. Don't worry about it for this PR.

Copy link
Contributor

@MSeal MSeal left a comment

Choose a reason for hiding this comment

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

This seems like it's only filtering the names that are visible to entrypoints rather than the actual exporters. Does this need to affect get_exporter as well?

This commit moves the logic for disabling exporters into the
get_exporter function rather than just updating the list of available
exporters. If someone explicitly asks for an exporter which is disabled
in their configuration they will get a ExporterDisabledError exception.
The logic in get_exporter_names can be simplified so it only has to
catch (and ignore) that exception when building the list of enabled
exporters.

Signed-off-by: Ian Allison <iana@pims.math.ca>
@ianabc
Copy link
Contributor Author

ianabc commented Jun 9, 2020

I took a look at moving the logic to get_exporter, it's a bit rough and ready but seems to be working OK. I define a new exception (ExporterDisabledError) which is raised when get_exporter is called on an exporter which is disabled in config. get_exporter_names can then use that exception to weed out the list of enabled exporters. Is this the sort of thing you were thinking of?

I think most things (notebook in particular) check to see what exporters are available with get_exporter_names before calling get_exporter on a specific exporter. Those uses should continue to work fine but if someone calls get_exporter blindly on something that happens to be disabled they will need to react to the exception (as in get_exporter_names).

Copy link
Contributor

@MSeal MSeal left a comment

Choose a reason for hiding this comment

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

Yeah that works. Covers both function to respect the attribute. Thanks for following through on the PR feedback!

@MSeal MSeal merged commit d8a6e34 into jupyter:master Jun 11, 2020
@betatim
Copy link
Member

betatim commented Jun 12, 2020

Nice work and thank you for contributing!

@MSeal MSeal added this to the 6.0 milestone Sep 8, 2020
vidartf added a commit to vidartf/jupyterlab_nbconvert_nocode that referenced this pull request Apr 23, 2021
[This change in nbconvert](jupyter/nbconvert#1273) meant that importing `from nbconvert.nbconvertapp import NbConvertApp` in  the global scope causes a circular import. That change seems to undo the efforts of jupyter/nbconvert#423, so there might be case to be made to clean it up on the nbconvert side, but this change should at least make this module workable with nbconvert 6 for now.
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.

4 participants