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

added path to the resources metadata, the same as in from_filename(...) in nbconvert.exporters.py #2753

Merged
merged 3 commits into from Oct 5, 2017

Conversation

Projects
None yet
4 participants
@gabyx
Copy link
Contributor

gabyx commented Aug 12, 2017

No description provided.

@gabyx gabyx referenced this pull request Aug 12, 2017

Merged

Fix html_embed exporter #1044

@gabyx

This comment has been minimized.

Copy link
Contributor Author

gabyx commented Aug 12, 2017

analogously to nbconverter.exporterts.py from_filename

@takluyver takluyver added this to the 5.2 milestone Aug 18, 2017

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Aug 18, 2017

Thanks, I think this makes sense. We should, however, make sure to convert the URL path into an absolute filesystem path using contents_manager.get_os_path(). And this will only be possible with the default file-based contents manager, not with a contents manager that stores notebooks in a database or something.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Aug 18, 2017

We're just preparing to release 5.1, so I've marked this as 5.2 - hopefully we can merge it soon after the release.

@gabyx

This comment has been minimized.

Copy link
Contributor Author

gabyx commented Aug 18, 2017

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Aug 18, 2017

I think it should work for other content managers, but the path should be 'progressive enhancement' - i.e. nbconvert should work without it where possible, even if it doesn't work as nicely.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Sep 15, 2017

I've pushed a commit to use filesystem paths rather than API paths, and only where the notebook is actually on the filesystem. I don't think there's any way to do this without using a private method of FileContentsManager, unfortunately.

@minrk minrk merged commit 9b4660f into jupyter:master Oct 5, 2017

4 checks passed

codecov/patch 75% of diff hit (target 0%)
Details
codecov/project 79.37% (+0.02%) compared to 63c7c33
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Oct 25, 2017

What was the purpose of this PR? I can't figure out what bug it was supposed to fix or feature it was supposed to implement and it broke PDF exports…

I'm really confused.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Oct 25, 2017

When nbconvert loads the notebook from a named file (i.e. normal use at the command line), the path is available to exporters and templates in the resources dict. They might use this, for instance, to locate associated files referenced by the notebook.

The point of this PR was that the path should also be available in the same way when nbconvert is used through the notebook web interface.

Sorry if this did break the PDF export. I think the fact that it's actually doing a conversion to Latex in a temporary directory and then a PDF build from there means it tends to hit edge cases in dealing with paths.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Oct 25, 2017

Ah, I see from your other comment the issue now. We didn't look carefully enough at nbconvert. path is supposed to be only the directory path containing the notebook, not the full file path. :-( That's neither the first nor the last time we've been bitten by our confusing use of the word 'path'. I only just caught the same mistake in #2949 before merging it.

Now that we can see that, do you want to do a PR to fix it (i.e. separate out 'name')?

@gabyx

This comment has been minimized.

Copy link
Contributor Author

gabyx commented Oct 25, 2017

What was path to be supposed for before? It did not exist I think, we added this, no?
maybe we should rename this to something more clear: filePath

@gabyx

This comment has been minimized.

Copy link
Contributor Author

gabyx commented Oct 25, 2017

We should really make it clear: notebookFilePath or (notebookDir).

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Oct 25, 2017

I think the naming is effectively part of nbconvert's API, so we can't really change that, even though it's confusing. We just need to make what this does match what nbconvert's from_filename does.

@gabyx

This comment has been minimized.

Copy link
Contributor Author

gabyx commented Oct 25, 2017

ah ok,

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Oct 25, 2017

What I don't understand is why this needs to have access to a file_path anyway? This will fail for contentsmanagers that don't work with filepaths (e.g., pgcontents which works on postgres)…

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Oct 25, 2017

@minrk for pointing out that prior to this PR, nbconvert only relied on the contentsmanager to get the content of the notebook

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Oct 25, 2017

Right, it's progressive enhancement. We provide the path if there is one, which is the case most of the time, but it's not guaranteed to be there, because the notebook may not have come from a file. This is the same in nbconvert.

@gabyx

This comment has been minimized.

Copy link
Contributor Author

gabyx commented Oct 25, 2017

we need the path where the notebook is located because we have exporters (for example embedders in html) which embed images with paths relative to the notebook location.

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Oct 25, 2017

Except that nbconvert can't be guaranteed to interpret path that way. I'll just construct the dict ahead of time conditionally instead of on the fly. it'll conflict with #2413 but that'll just have to do.

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Oct 25, 2017

Note #2981 addresses this.

@gabyx

This comment has been minimized.

Copy link
Contributor Author

gabyx commented Oct 26, 2017

Can somebody enlighten me what the content manager does? is it the abstraction when the notebook runs on a server? I am kind a of a newbie here...

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Oct 28, 2017

It deals with loading and saving notebooks and other files. Normally that just means reading and writing files in directories, but you can plug in another content manager to store notebooks in a database, or to store them in files in a different format.

If you know what FUSE is, it's a similar concept to that - a way to use other storage systems kind of like a filesystem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.