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

gabyx
Copy link
Contributor

@gabyx gabyx commented Aug 12, 2017

No description provided.

@gabyx
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
Copy link
Member

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

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
Copy link
Contributor Author

gabyx commented Aug 18, 2017 via email

@takluyver
Copy link
Member

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

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
@mpacer
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
Copy link
Member

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

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
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
Copy link
Contributor Author

gabyx commented Oct 25, 2017

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

@takluyver
Copy link
Member

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
Copy link
Contributor Author

gabyx commented Oct 25, 2017

ah ok,

@mpacer
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
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
Copy link
Member

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

mpacer commented Oct 25, 2017

Note #2981 addresses this.

@gabyx
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
Copy link
Member

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants