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

FEAT: Support for general static asset source locations #268

Open
wants to merge 34 commits into
base: master
Choose a base branch
from

Conversation

mmcky
Copy link
Contributor

@mmcky mmcky commented Sep 4, 2019

This PR allows for more flexible source files to be specified in the RST documents and will organise them based on type in the BUILD directory. All images will be located in BUILDDIR/_images and all downloads will be placed in BUILDDIR/_downloads. Only the assets referenced in the source RST files will be copied to the build location.

  • images -> _images
  • downloads -> _downloads
  • remove bulk copy of _static assets folder
  • make sure theme is copied to _static
  • copy from BUILDDIR to executed directory
  • copy from BUILDDIR/jupyter/ to BUILDDIR/jupyter_html for make site

@mmcky mmcky added the in-work label Sep 4, 2019
@mmcky mmcky modified the milestones: v0.4.4, v0.5 Sep 4, 2019
@mmcky
Copy link
Contributor Author

mmcky commented Sep 5, 2019

@AakashGfude currently _downloads is taken by download notebook option. We should update this to use _downloads/ipynb/ with all download directive files going into the root level of _downloads?

@mmcky
Copy link
Contributor Author

mmcky commented Sep 6, 2019

Note: The download notebooks won't visually work until the s3 location hosts images in _images.

@mmcky
Copy link
Contributor Author

mmcky commented Sep 11, 2019

@AakashGfude for some reason this branch is no longer placing build files in _build/jupyter but rather in the root level _build. I haven't changed self.outdir in the builder so I don't know why this would have changed. The master branch still works. Any ideas? Solved this in 31c3eba

@mmcky
Copy link
Contributor Author

mmcky commented Sep 11, 2019

  • fix merge conflicts with master branch

  • the _downloads library doesn't seem correct with too many -1 applied:

3ndp-1.pdf           countries-1.csv     employ-1.csv   iteration_notes-1.pdf  realwage-1.csv  ticker_data-1.csv
aiyagari_obit-1.pdf  course_notes-1.pdf  firenze-5.pdf  pi2-7.pdf              test_pwt-1.csv  wb_download-1.py
  • need to figure out how to support _images path to the root level in the context of a jupyter notebook server and as html. If we use /_images then html works but a jupyter notebook instance doesn't find the images as the jupyter notebook opens a server one level down?

@mmcky
Copy link
Contributor Author

mmcky commented Sep 19, 2019

@mmcky mmcky added wait and removed test labels Sep 19, 2019
@mmcky mmcky modified the milestones: v0.5, v0.6 Sep 19, 2019
Copy link
Member

@sglyon sglyon left a comment

Choose a reason for hiding this comment

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

remote images work for me now -- thank you

@mmcky
Copy link
Contributor Author

mmcky commented Sep 23, 2019

thanks for testing @sglyon -- this is on hold for a week as we get pdf feature merged. Then we will revisit this PR.

@mmcky
Copy link
Contributor Author

mmcky commented Oct 4, 2019

Testing

  • check this works for html output
  • check this works for pdf output through jupyterpdf

@mmcky
Copy link
Contributor Author

mmcky commented Oct 8, 2019

  • this breaks JupyterPDFBuilder
Exception occurred:
  File "/home/qebuild/repos/sphinxcontrib-jupyter/sphinxcontrib/jupyter/writers/translate_all.py", line 206, in visit_image
    elif uri not in self.builder.image_library.keys():
AttributeError: 'JupyterPDFBuilder' object has no attribute 'image_library'

@mmcky mmcky added the bug label Oct 8, 2019
@mmcky
Copy link
Contributor Author

mmcky commented Oct 8, 2019

@sglyon @arnavs @AakashGfude -- just wanted to check in now that pdf has been merged on this planned change to how static assets are managed. Before I go and fix the JupyterPDFBuilder with the new _image and _downloads flat file library apporach I thought I would open this to discussion to see if there were any thoughts / comments / improvement ideas on this implementation or approach. I am happy to do the code work but would love to hear if anyone has comments before we make the change.

This is subotimal in one usage case -- which is distributing notebooks (along with static) assets. For example someone may want a notebook and some statics to go along with it when teaching a class. This change would make it hard to distribute a notebook and any static assets as they are embedded in the _image folder rather than being in a local static folder for example.

@mmcky mmcky added the in-work label Oct 8, 2019
@AakashGfude
Copy link
Member

@mmcky no ideas as such popping out now, the implementation looks sleek. Regarding the distribution of assets. Maybe in the later stage maybe we can have a parent directory which has all the folders like _image, _downloads and _static, but then we will have to prepend all the links to have that parent subdirectory, which can be handled by the plugin or passed as a static path to conf.
Having a single entry directory for all of these static assests will definitely be good in the long run.

@jlperla
Copy link
Member

jlperla commented Oct 8, 2019

just wanted to check in now that pdf has been merged on this planned change to how static assets are managed. Before I go and fix the JupyterPDFBuilder with the new _image and _downloads flat file library apporach I thought I would open this to discussion to see if there were any thoughts / comments / improvement ideas on this implementation or approach. I am happy to do the code work but would love to hear if anyone has comments before we make the change.

@mmcky I am not sure I understand the issues here and whether this has any practical effects. Just to be clear, when you talk about notebooks are you talking about the intermediate juptyer format or the final notebooks being spit out for the github repo?

  • If it is the final output jupyter we put in the github, then we want all notebooks to work self-contained, so the static assets shouldn't effect that since evertyhing would point to http?
  • That said, even if they don't require files, it is nice to have things cached. In particular project files, data, etc. which can be in the github repo as well (and is downloaded if it isn't fully cloned). We do this a lot, but luckily those features are already in the

So, assuming we don't want to have any static assets deployed (outside of the stuff we used to discuss of putting the Manifest/Project/etc. in the same directory structure for execution) does this effect us?

@mmcky
Copy link
Contributor Author

mmcky commented Oct 8, 2019

thanks @jlperla for your comments. The primary issue is when julia went over to use nested folders -- the way sphinx parses uri(i.e. link references) became broken. They aren't generated correctly for the nested structure. When we had julia in the jl subfolder this was a big issue as we couldn't make use of the root location and relative links were broken so we put a patch in place that copies _static to every folder. That is how julia is currently served if you look at image links etc.

To be clear there are a lot of different notebook types now. The use cases for our lecture sites is pretty much covered:

  1. html - notebook used for generating the html (emphasis on html use in intermediate ipynb)
  2. pdf - notebooks used for generating the pdf (emphais on pdf / latex compatibility in intermediate ipynb)
  3. coverage - notebooks used in coverage testing (includes unittests)
  4. download - notebooks used to support downloads and are self contained with supporting we locations for images etc.

there is another

  1. jupyter - used primarily for local editing (without execution) and we are placing an emphasis or generating readable notebooks (with as much markdown as possible). This is being used by some lecturers to generate course notes etc. from rst and once feature request is to have local _static objects so if you have a folder lect1 the static assets for those lectures are contained in lect1 rather than some global statics folder. This PR doesn't work particularly well in that use case. In november when we do major rewrite of the extension I think we will support this request.

This PR mimics the standard sphinx hmtl writer and collects only used assets into a image and downloads folder. One benefit is only objects called in RST are copied so legacy static objects aren't hosted and removes complexity around nested folder structures etc.

@jlperla
Copy link
Member

jlperla commented Oct 8, 2019

This all sounds like a reasonable iteration on the design (although I don't need to follow it all!)

Does this have a concrete impact on the julia and datascience lectures, how we write the RST, changing what we currently deploy, etc? Or are you mostly just giving us a heads up that large changes are afoot?

@mmcky
Copy link
Contributor Author

mmcky commented Oct 8, 2019

hey @jlperla no -- all of these changes are internal to the extension. No changes to RST required -- just disussing this as it changes how static assets are managed and keen to see if there are better ideas on the change.

It will change config -- (i.e. web links for images etc.) -- but that will need to managed if this get's merged.

@jlperla
Copy link
Member

jlperla commented Oct 8, 2019

Sounds great to me! If I think of anything, I will mention it. Though I don't have a mental model of sphinx, so it is unlikley.

@mmcky mmcky modified the milestones: v0.6, v0.7 Oct 10, 2019
@mmcky
Copy link
Contributor Author

mmcky commented Oct 10, 2019

Note: Placing this on hold as we evaluate the extension in November for rewrite. This will be included at that time.

@mmcky
Copy link
Contributor Author

mmcky commented Jan 31, 2020

this PR provides one implementation idea for managing static assets. This PR will not be merged as the underlying extension has significantly changed. But this PR contains some interesting ideas.

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

Successfully merging this pull request may close these issues.

4 participants