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

[WIP] Implementation of #86 #87

Merged
merged 14 commits into from
Sep 25, 2020
Merged

[WIP] Implementation of #86 #87

merged 14 commits into from
Sep 25, 2020

Conversation

torfjelde
Copy link
Member

Ref #86 and its discussion. This PR is WIP for converting TuringTutorials.jl into a similar structure as https://github.com/SciML/SciMLTutorials.jl.

TODOs

  1. Convert tutorials to JMD format and make sure they're working.
  2. Add proper build-script/CI.
    • Worth noting that we don't need this to merge the PR, as we can just manually generate the markdown files and push.

@devmotion
Copy link
Member

Worth noting that we don't need this to merge the PR, as we can just manually generate the markdown files and push.

I'd suggest to keep this PR as simple as possible, and just focus on the Weave commands + converting all notebooks to jmd here (I guess one might be able to do this in a semi-automated fashion using Weave?). Then we should just run them locally, using the Weave scripts, before merging the PR (SciMLTutorials used a manual build process for years before switching to CI some months ago).

@devmotion
Copy link
Member

I would maybe go even further and not even touch the existing environment nor update the tutorials in this PR, since I guess there is a lot to update and it might become confusing quite quickly.

@torfjelde
Copy link
Member Author

torfjelde commented Sep 24, 2020

I would maybe go even further and not even touch the existing environment nor update the tutorials in this PR, since I guess there is a lot to update and it might become confusing quite quickly.

100% agree with this! (Though I did make a tiny change to the VI tutorial, but not planning on making any more!)
What I've done now is literally just go through the tutorial, added results = "hidden" where it's intended to now show output (Weave.jl doesn't seem to respect the ; at the end of the block) and made sure that the tutorial runs with updated packages.

BUT I also realize that because I'm moving files around, etc. the review process is going to be horrible.

Maybe it would be a good idea (this is what you're also getting at, right?) to do it in several PRs:

  1. Only change the files to JMD format (without changing the file extension), e.g. adding results = "hidden". EDIT: Easier to just combine this step with (3).
  2. Move all files to the tutorials/... folder and add jmd extension + remove all notebooks and clean the markdown folder, i.e. only move + delete files, but no actual changing of the files.
  3. Add Project.toml to each of the tutorials and make sure tutorials are running, fixing accordingly.

Also, all of the above should probably be merged into a staging branch or something, and then we do a complete final merge to master.

Thoughts?

@devmotion
Copy link
Member

Maybe it would be a good idea (this is what you're also getting at, right?) to do it in several PRs:

Yes, exactly, that's mainly what I had in mind.

Also, all of the above should probably be merged into a staging branch or something, and then we do a complete final merge to master.

That's a good idea! Let's just make sure that every change to master before the branch is merged, goes into the staging branch as well.

So, yes, I think this sounds like a good plan 👍

@torfjelde
Copy link
Member Author

Great!
Btw, I don't need reviews for just moving files around, right? So should I just create such a branch with the files moved and deleted, and then simply make PRs to that.

@torfjelde torfjelde changed the base branch from master to jmd-staging September 24, 2020 13:13
@torfjelde
Copy link
Member Author

How's this @devmotion ? jmd-staging is a branch with all the files moved and deleted, but without any changes made to the contents + added src/TuringTutorials and cleaned up Project.toml.

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

Looks OK, I added some minor comments.

In general, I think the results = "hidden" statements are applied a bit inconsistently here - shouldn't we remove all the semicolons that are used to suppress output currently if we add them? Or just not add them for now?

@torfjelde torfjelde merged commit f780124 into jmd-staging Sep 25, 2020
@delete-merged-branch delete-merged-branch bot deleted the tor/using-jmd branch September 25, 2020 08:58
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 this pull request may close these issues.

2 participants