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

Fix missing templates by porting to nbconvert 6.* #1405

Closed
wants to merge 7 commits into from

Conversation

rkdarst
Copy link
Contributor

@rkdarst rkdarst commented Jan 11, 2021

nbconvert 6.* broke nbgrader somehow. This uses hints from #1373 to fix the problem. Individual commit messages describe what is happening, but basically I change some template names and fix a variable.

Previously it was pinned to solve the problem, but a pin is eventually going to cause un-installable software when combined with other things. This is one of the banes of my daily work, so we need to do something about that.

Closes: #1373, Closes: #1382

Review:

  • I am by no means an expert here, so I can't vouch for if I have made the correct answers to things. I have done some testing, and gotten others to as well, but it's not fully tested manually.
  • Check if CI passes (I haven't gotten tests to run locally yet).

- From jupyter#1373, thanks @gzagatti
- I can't quickly find the latest version of jinja2 that requires this
- Solution from jupyter#1373, thanks to @gzagatti
- I don't know how or why this works
- nbconvert has changed the name of its basic.tpl to
  classic/index.html.j2.  Found via looking into the source code.
- This seems to be about where these changes are.  Could be slightly
  older but not too much.
@rkdarst
Copy link
Contributor Author

rkdarst commented Jan 11, 2021

Also fixes #1367.

@rkdarst rkdarst marked this pull request as draft January 19, 2021 00:29
@rkdarst
Copy link
Contributor Author

rkdarst commented Jan 19, 2021

I gave up on this.

I got most of the problems fixed (took some work but wasn't too hard), but the feedback template has problems remaining. I will elaborate later, but at first it seems that, while nbconvert tries to preserve a backwards-compatible template, the interaction with nbgrader customizations doesn't really work properly. Before I could get there, I had to work around some problems with the nbconvert classic template not finding static/style.css from within the classic/ directory - I think there is some bug with template paths and so on.

I will try to document my findings better later. We could use someone that understands the nbgrader feedback template customizations.

@BertR
Copy link
Contributor

BertR commented Jan 20, 2021

I gave up on this.

I got most of the problems fixed (took some work but wasn't too hard), but the feedback template has problems remaining. I will elaborate later, but at first it seems that, while nbconvert tries to preserve a backwards-compatible template, the interaction with nbgrader customizations doesn't really work properly. Before I could get there, I had to work around some problems with the nbconvert classic template not finding static/style.css from within the classic/ directory - I think there is some bug with template paths and so on.

I will try to document my findings better later. We could use someone that understands the nbgrader feedback template customizations.

Thanks for this! Please share your thoughts when you have the time to we kind think about how we could progress this.

@rkdarst
Copy link
Contributor Author

rkdarst commented Feb 4, 2021

Here is what I remember:

  • nbconvert template system completely reworked (see link in nbconvert version 6.0.0 breaks nbgrader feedback #1367)
  • You might think you could simply change the template you inherit from. I did this, and it got further.
  • But, the nbgrader template seemed to have been made by copy-pasting from the nbconvert templates. Most likely the old system wasn't flexible enough. Since the new compatibilty templates are slightly different, it doesn't work as a drop-in replacement. I took and edited some, trying to port (what I thought were) the nbgrader specific parts to the new template system.
  • But then... there was another template error! I seem to recall it couldn't find static/style.css from within the template. I didn't dig too deep, but I think it is something like this: .../templates/ is added to the template search path. The template classic tries to load static/style.css, but the path to that is classic/static/style.css, but tries to load it by a relative path. But the relative path doesn't work, when called from nbgrader. Somehow this is all tied up with template inheritance.
  • I think I tried to run some manual nbconvert commands, specifying the other templates, to reproduce the static/style.css problem (to verify if it is upstream or not). I think I decided it was an upstream problem, but I don't have those commands anymore, so I would try to verify before trusting this (also since more time has passed).
  • I convinced myself that it was better to take a break, and see if something upstream could be worked out, then come back to it.

If anyone wants to work on this, please do! Also feel free to fix up this PR and make it work, even if I don't have time to work on it.

@jhamrick
Copy link
Member

I have started trying to work on this too. So far have just fixed a few broken tests but haven't actually looked at the template stuff yet. Here are my changes for reference: https://github.com/jhamrick/nbgrader/commits/fix-nbconvert Hopefully I'll have some more time tomorrow to look at this too.

@jhamrick
Copy link
Member

And I should say as well, thanks so much @rkdarst for the work you've put into trying to fix it so far! Hopefully we can figure out the last bit that's needed and remove this dependency on the old version of nbconvert...

@jhamrick
Copy link
Member

I think I've figured this out! The missing piece was that in the new version of nbconvert, it expects a file called confg.json in the directory where the templates are, and this json file determines what the base template is you are inheriting from. Setting this to the 'classic' value seems to have fixed the inheritance problems and it no longer has trouble finding the relevant static assets.

I'll make a new PR shortly and hopefully all the tests will still pass with that.

Thanks again for all the work you put into this @rkdarst , it was hugely helpful to have as a place to start from!

@jhamrick
Copy link
Member

jhamrick commented May 5, 2022

Closing as this is superseded by #1421

@jhamrick jhamrick closed this May 5, 2022
@jhamrick jhamrick added this to the No action milestone May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants