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

Tutorials are largely outdated #86

Closed
torfjelde opened this issue Sep 23, 2020 · 91 comments · Fixed by #113
Closed

Tutorials are largely outdated #86

torfjelde opened this issue Sep 23, 2020 · 91 comments · Fixed by #113

Comments

@torfjelde
Copy link
Member

A lot of the tutorials are unfortunately outdated due to recent improvements in MCMCChains, e.g. TuringLang/Turing.jl#1411.

I'm opening an issue here so as to bring attention to the, well, issue.

@devmotion
Copy link
Member

I think maybe it would help to 1) add separate environments for different tutorials (so people can reproduce it even though it is based on an older version of some package) and 2) use CI tests and builds for the tutorials.

@torfjelde
Copy link
Member Author

100%. But for (2), we likely have to move away from Jupyter notebooks, right? Which, honestly, would be it's own benefit.

@torfjelde
Copy link
Member Author

And regarding (1), I would say that ideally we'll always have the tutorials work with the most recent release of Turing, since people will look to this for inspiration when writing their own code.

Maybe we should run Pkg.status() at the beginning or end of every notebook too, so that people can view the dependencies.

@devmotion
Copy link
Member

Maybe we should run Pkg.status() at the beginning or end of every notebook too, so that people can view the dependencies.

That's how it is done in SciML: https://github.com/SciML/SciMLTutorials.jl An appendix is added automatically to every tutorial which shows the packages and Julia version that was used to run it

@torfjelde
Copy link
Member Author

That's how it is done in SciML

Nice 👍 And they use jmd as the "base" file?

@devmotion
Copy link
Member

Yes. And then notebook, HTML, and PDF files are generated with Weave.

@torfjelde
Copy link
Member Author

Super-dope. Would it be a good idea to just try and duplicate their approach?

@devmotion
Copy link
Member

Yes, I think that would be great. In particular also thinking about https://github.com/SciML/RebuildAction which we use over at SciML to rebuild tutorials automatically (together with CompatHelper for individual environments this helps to figure out any possible issues).

@torfjelde
Copy link
Member Author

Yeah I just noticed that; awesome stuff! Cool, well I'm very down for doing that.

@cpfiffer
Copy link
Member

All of this is a great idea. I hate the way the tutorials currently work -- they're difficult to write in, difficult to automate, and often out-of-date. Switching to a more general jmd approach is something I'm all for.

@trappmartin
Copy link
Member

Super cool idea.

@torfjelde
Copy link
Member Author

I've now updated all the tutorials to work with the new system (which is in a separate branch jmd-staging, which we'll merge into master when everything is ready for a full update).

Do have a look:) We also need an updated script to publish the tutorials. One approach could be to just generate the markdown from the jmd file and then use the current Jekyll setup. Maybe that's a good place to start?

@devmotion
Copy link
Member

One approach could be to just generate the markdown from the jmd file and then use the current Jekyll setup. Maybe that's a good place to start?

Yes, I think that's what we should do (at least initially). IMO we should not update too many things in one step 🙂

torfjelde added a commit that referenced this issue Sep 25, 2020
@torfjelde
Copy link
Member Author

@cpfiffer I might have some questions for you about the documentation setup later, because right now I'm trying to run it locally but using a different branch of TuringTutorials, and I swear I'm loosing hair as I type. It for some reason refuses to render the the pages, despite the markdown files being available in _tutorials/ on the top-level (I messed around with the copy_tutorials method to do so). It's on the jmd-staging branch if you want to have a go yourself. You generate markdown just by doing

julia --project -e "using Pkg; Pkg.instantiate(); using TuringTutorials; TuringTutorials.weave_folder(\"introduction\", (:github, ))"

while in the project folder.

@cpfiffer
Copy link
Member

Do the rendered markdown files have YAML headers? Because otherwise they won't render. The old tutorials code would handle this automatically:

https://github.com/TuringLang/TuringTutorials/blob/38257ab7036575c63113a5b5556c8cd5fd20cb31/weave-examples.jl#L27

@torfjelde
Copy link
Member Author

Aaah, that might be it

@torfjelde
Copy link
Member Author

Btw, a small update: I can get the tutorials to render but the formatting is completely broken. Honestly don't understand why, as the output is standard markdown.

@devmotion
Copy link
Member

Do you have a reproducible example?

@torfjelde
Copy link
Member Author

Well, it's quite annoying to replicate but here it is:

  1. ] dev TuringTutorials and checkout tor/jmd-weaving
  2. Go to ~/.julia/dev/TuringTutorials and run julia --project -e "using TuringTutorials; TuringTutorials.weave_folder("introduction", (:github, ))" for testing
  3. Change the md_path in copy_tutorial in Turing/docs/make-utils.jl to HOME/.julia/dev/TuringTutorials (you have to replace HOME manually)
  4. julia --project make.jl
  5. bundle exec jekyll serve

@devmotion
Copy link
Member

Should it be tor/jmd-files? I can't find tor/jmd-weaving

@torfjelde
Copy link
Member Author

torfjelde commented Oct 7, 2020

Nah, tor/jmd-weaving should be there. Alternatively, you could try jmd-staging which should also work.

EDIT: I think you should use jmd-staging.

@devmotion
Copy link
Member

I finally managed to obtain a local preview of the docs, and formatting seems to be fine? It took me a while to figure out that the new setup (or maybe just my weird way of changing make-utils.jl) leads to different paths, and I had to go to http://localhost:4000/dev/tutorials/01-introduction/ instead of just following the links to the first tutorial. So math rendering and text formatting seems fine, it seems there's just some problem with the images and gifs (they are copied to the docs folders but instead of displaying them some text is shown on the page).

@devmotion
Copy link
Member

Regular code output works fine though.

@torfjelde
Copy link
Member Author

Really? On my end it looks super-strange. Hmm, well, that's good news!

Yeah, I think we have to change where the figures are outputed.

@torfjelde
Copy link
Member Author

Btw, I realized I had a mistake in the step-by-step guide above. You also need to make sure that the files are "lifted" to the top-level in _tutorials, i.e. copy_tutorial needs to be

function copy_tutorial(tutorial_path)
    isdir(tutorial_path) || mkpath(tutorial_path)
    # Clone TuringTurorials
    tmp_path = tempname()
    mkdir(tmp_path)
    # clone("https://github.com/TuringLang/TuringTutorials", tmp_path)

    # Move to markdown folder.
    md_path = joinpath("/home/tor/.julia/dev/TuringTutorials/", "markdown")

    # Copy the .md versions of all examples.
    try
        @debug(md_path)
        # recursively find files and copy to top-level in `tutorial_path`
        for (curr_dir, dirs, files) in walkdir(md_path)
            for file in files
                full_path = joinpath(md_path, curr_dir, file)
                target_path = joinpath(tutorial_path, file)
                println("Copying $full_path to $target_path")
                cp(full_path, target_path, force=true)
                if endswith(target_path, ".md")
                    # remove_yaml(target_path, "permalink")
                    fix_header_1(target_path)
                    fix_image_path(target_path)
                end
            end
        end
        index = joinpath(@__DIR__, "src/tutorials/index.md")
        cp(index, tutorial_path * "/index.md", force=true)
    catch e
        rethrow(e)
    finally
        rm(tmp_path, recursive=true)
    end
end

But now it works nicely for me to!

A couple of issues:

  1. Stuff like {julia; eval = false} at the beginning of code-blocks doesn't seem to be supported by Jekyll (it's apparently supported by Github though).
  2. Figures aren't referenced correctly (as @devmotion also mentioned)
  3. Naming convention is different. Personally I quite like the convention from SciMl, where you have folders representing different categories. But at the same time, just to get this done asap, it might be preferable to just change the weave-process to use a flattened structure.

@torfjelde
Copy link
Member Author

"Fixed" (2) and (3) locally, but (1) I can't seem to be able to fix. Even tried using what's supposed to be GFM markdown parsers in Jekyll, but none of them actually handle {julia; eval = false} correctly.

@theogf
Copy link
Member

theogf commented Mar 4, 2021

Maybe we should run Pkg.status() at the beginning or end of every notebook too, so that people can view the dependencies.

That's how it is done in SciML: SciML/SciMLTutorials.jl An appendix is added automatically to every tutorial which shows the packages and Julia version that was used to run it

To bounce back on this, we could add a little warning at the beginning of each tutorial saying "Check that you are using the correct version of Turing for this tutorial (see end of Tutorial), if it does not work with the latest version of Turing please open an issue"

@torfjelde
Copy link
Member Author

torfjelde commented Mar 5, 2021

Sorry, I haven't been looking at this for ages now. It first got postponed because the computer I had access to here back in Norway had very limited RAM, and so it was a hassle to do any development on this. I wanted to wait until I got back to Cambridge where I don't have this issue but the pandemic had other ideas. Got an upgrade recently though, so I'll have a look again 👍

Hey I thought it would be good to revive this issue as it is obviously very needed :D
@torfjelde What still needs to be done?

If I remember correctly, the following issues remain:

  1. How do we deal with plots? See the last couple of comments above.
  2. Need to flatten the project-structure, which I already did but then left those changes in Cambridge 🙃

But I figured out a fix to (1) this morning, so now I'll address (2) and try to generate everything; hopefully I'll have a working PR today!

@theogf
Copy link
Member

theogf commented Mar 5, 2021

By the way, why don't we use Literate.jl instead of Weave.jl, Literate handles plots nicely I think.

@torfjelde
Copy link
Member Author

Hey @theogf! Yeah you, you who are now suggesting that we use Literate.jl instead of Weave.jl after I've gone through hell to get this stuff working. Shut your face.

@theogf
Copy link
Member

theogf commented Mar 5, 2021

Hahaha can't argue with that! 😆😆😆 (although it would be easier to switch now 👀 )
More seriously can I help you with the branch? Is there something to modify on each tutorial?

@torfjelde
Copy link
Member Author

torfjelde commented Mar 5, 2021

No but on a serious note, Weave.jl handles plots nicely too. But Weave.jl respects the MIME-type, and that is messed up for gifs for some reason, and so Weave.jl treats it as text rather than a file. I would argue this is not to be completely blamed on Weave.jl.

And more generally, I think both Weave.jl and Literate.jl are very much the same when it comes to features but Weave.jl is more mature, is under JunoLabs and thus we'd expect it to me maintained, and finally, and this is the main reason why we went with Weave.jl in the first place, jmd is a super-straightforward conversion from our current setup since we can just take the markdown files and convert to jmd while Literate.jl is a completely different approach AFAIK.

@torfjelde
Copy link
Member Author

More seriously can I help you with the branch? Is there something to modify on each tutorial?

So right now I think the PRs #102 and #103 should be good to get things working. The next step is to make sure that everything renders correctly on the website (@cpfiffer pls halp). I have a working branch of Turing that manages to do this locally, but need to ensure that this is also the case for the actual website.

After all that, I think we should really just go through the tutorials to:

  1. Update dependencies.
  2. Make sure everything is working correctly.

@theogf
Copy link
Member

theogf commented Mar 5, 2021

Oh ok so the idea is to work on jmd-staging until everything is ready and then push to master.

@torfjelde
Copy link
Member Author

Exactly 👍

@cpfiffer
Copy link
Member

cpfiffer commented Mar 5, 2021

I wouldn't worry too much about correct rendering at the outset, we can fix it as we go along.

@cpfiffer
Copy link
Member

cpfiffer commented Mar 5, 2021

Plus the changes need to happen elsewhere.

@torfjelde
Copy link
Member Author

So the only thing that remains now is making sure that all tutorials actually work. There are some that are just failing, e.g. regression tutorial fails because of JuliaStats/RDatasets.jl#117. I'll go through, fix the ones I can and list the ones that are still todo, then I'll make another PR.

@theogf
Copy link
Member

theogf commented Mar 6, 2021

I will make a new PR for the BNN tutorial (I will try to include GPU support for vi this time)

@torfjelde
Copy link
Member Author

Another issue I'm currently running into is that the footer thingy we have at the end of the tutorials suddenly stopped working. When I do:

julia --project -e "using TuringTutorials; TuringTutorials.weave_folder(\"00-introduction\", (:github, ))"

it complains that TuringTutorials is not available which it clearly is, unless Weave is now evaluating in the file in a different scope? If so, no one sent me that memo!

@torfjelde
Copy link
Member Author

torfjelde commented Mar 6, 2021

So seems like it actually does: https://github.com/JunoLab/Weave.jl/blob/196d4ca7ce80e65f7365642218c43a8c94d5f286/src/run.jl#L41-L42.

But how about changing

using TuringTutorials
TuringTutorials.tutorial_footer(WEAVE_ARGS[:folder],WEAVE_ARGS[:file])

to

if isdefined(Main, :TuringTutorials)
    Main.TuringTutorials.tutorial_footer(WEAVE_ARGS[:folder], WEAVE_ARGS[:file])
end

?

This way one can still weave the files "outside" of TuringTutorials, but it will do the right thing if TuringTutorials is exposed in Main.

@cpfiffer
Copy link
Member

cpfiffer commented Mar 6, 2021

Yeah, I love it!

@torfjelde
Copy link
Member Author

Wow, I don't think I've ever seen anyone that enthusiastic about changing footers before! I guess it makes sense, eh Mr. Footman?
Anyways, things are running fine now. Just waiting for the diff-eq tutorial to finish bobbing and weaving then I'll make a PR with the generated markdown files?

@torfjelde
Copy link
Member Author

Getting there: #105 and #106.

Is there anything else that needs to be done?

@torfjelde
Copy link
Member Author

Could you maybe have a check that everything is fine @cpfiffer ? Just checkout the tor/doc-updates branch for Turing.jl and do ] add TuringTutorials#jmd-staging to docs, then it should just work.

@torfjelde
Copy link
Member Author

Don't need to check every tutorial, but mainly that the setup makes sense

@cpfiffer
Copy link
Member

cpfiffer commented Mar 7, 2021

Looks fine to me. We can test it live by pushing the jmd-staging branch and then switching over from TuringLang/turing.ml, where the site currently lives. It'll be a good way to check to see if the site builds correctly without having to keep the site live if something is off.

@torfjelde
Copy link
Member Author

Sounds good!

@torfjelde
Copy link
Member Author

Aight, so now it's ready to go!

Need to merge #109 and then #111 for TuringTutorials, and then finally TuringLang/turinglang.github.io#7 over at turing.ml.

@torfjelde
Copy link
Member Author

Is there anything remaining for us to completely move over to jmd staging at this point?
AFAIK the only thing that's left-over is fixing the maths in the non-tutorial sections over at https://github.com/TuringLang/turing.ml/. Once that's done, do we just deploy to /dev?

@cpfiffer
Copy link
Member

Yeah, that sounds about right. I'll try to fit in the time this week to get the turing.ml stuff ready to go.

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 a pull request may close this issue.

7 participants