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 as git subtree for tutorials with nbsphinx #133

Conversation

KennethNielsen
Copy link

This PR to a development branch includes the following:

  • The tutorials have been added with the git subtree functionality, which means that the entire state of the tutorials repo is copied in as a normal
  • Instructions have been added for how to update the subtree
  • The tutorials index in the docs have been updated with the location of the new links
  • One link (to ECMS quantification was change entirely, since it seemed to be broken, possibly due to a move of the document)
  • Exclude patterns have been added to sphinx' conf.py to make sure that we don't get warnings about documents not being include, which are really just normal RST documents in the tutorials repo, which are not supposed to be used in docs

I did not include an invoke task for updating the subtree. I will if you think the idea is sound.

Kenneth Nielsen added 7 commits November 2, 2023 22:03
…ent from commit d57487c

git-subtree-dir: docs/source/getting_started/tutorials/tutorials_repo
git-subtree-split: d57487cf140a72bd415f54a881007c41fdf63d96
…ce/getting_started/tutorials/tutorials_repo'
…he tutorials repo that are not documentation
Copy link
Member

@ScottSoren ScottSoren left a comment

Choose a reason for hiding this comment

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

Hi Kenneth,
Thanks for this! Git subtree looks like a great way to copy over the tutorials repo into ixdat/docs.
The main remaining issue is that I want the ixdat repo's version control repeating as little as possible of what the tutorials repo's version control does. That means adding a bunch of stuff to ixdat's gitignore (it took me a little while to figure out that it is ixdat/.gitignore and not ixdat/docs/source/getting_started/tutorials/tutorials_repo/.gitignore which should include the files to ignore). In fact, the only thing from ixdat/docs/...tutorials_repo that should not be ignored by ixdat/.gitignore is the .ipynb's (which in some cases will be pushed in a compiled state).
Enough went wrong on the commit right after copying over the tutorials repo that, even if it's silly, I kind of want to not merge in that commit...
I also want to change the path (removing the "getting_started" layer) and delete some extra stuff.

Three ways to proceed:

  • Merge this and I make the changes suggested above in subsequent commits
  • Close this PR unmerged, and then I copy over the desired parts of this PR and make a new PR
  • You undo (with some git magic, reseting to before and then re-applying the subsequent commits?) this commit: KennethNielsen@42b8734 (keeping all other commits in this PR) and then we update ixdat's .gitignore and the other stuff before copying the .ipynb's over again and committing.

Let me know which you prefer!

@@ -58,7 +58,10 @@
# List of patterns, relative to source directory, that match files and
# directories to ignore when looking for source files.
# This pattern also affects html_static_path and html_extra_path.
exclude_patterns = []
exclude_patterns = [
"getting_started/tutorials/tutorials_repo/README.rst",
Copy link
Member

Choose a reason for hiding this comment

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

The tutorials repo README.rst might not stay excluded, but good for now and good to see how it's done.
Good idea to exclude the demos!

is maintained via a git feature called ``git subtree``. The copy of the tutorials in ixdat can
be updated with new changes with the following command::

git subtree pull --prefix docs/source/getting_started/tutorials/tutorials_repo https://github.com/ixdat/tutorials.git main --squash
Copy link
Member

Choose a reason for hiding this comment

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

Nice! This works for me :)

Copy link
Member

Choose a reason for hiding this comment

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

Ah... I had updated the file (with the right url paths) in ixdat/docs but not in the tutorials repo, so it gets overwritten here. No problem, lots of updates are needed in the tutorials repo. Nice that the one true version of the jupyter notebooks will soon be in one place!

Copy link
Member

Choose a reason for hiding this comment

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

Same as before, I will need to update this file in tutorials for the change to manifest stably here.

Copy link
Member

Choose a reason for hiding this comment

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

... but wait a second! Why does this file even show up on the PR? Shouldn't ixdat's git ignore this sub-repo, since its version control is delegated to the git subtree?

Copy link
Member

Choose a reason for hiding this comment

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

The tstamp change is due to the time zone in which the jupyter notebook producing this export file was compiled, haha. But wait a second, why did the ixdat version go down? Do you have a really old version of ixdat pip-installed and never updated because you are using venv's?

Copy link
Member

Choose a reason for hiding this comment

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

In any case we should leave this file out!

Copy link
Member

Choose a reason for hiding this comment

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

Should be ignored.

Copy link
Member

Choose a reason for hiding this comment

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

same, this should be .gitignored... by ixdat's .gitignore, I just realized!

Copy link
Member

Choose a reason for hiding this comment

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

Could we leave out the demos folder of the subrepository by adding it in the .gitignore here? Or would that be just overwritten by the .gitignore in the tutorials repository?

Copy link
Member

Choose a reason for hiding this comment

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

same as some others, the updated paths are the ones that got overwritten.

Copy link
Member

Choose a reason for hiding this comment

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

excellent! But this should also be in the tutorials repo, not here! Here some of the jupyter notebooks need to be committed and pushed in a compiled state so that they don't need to be compiled on readthedocs' server, which might not have the needed experimental data available.

@KennethNielsen
Copy link
Author

Thanks for the review. I can see from the amount of comments that this was not quite the easy fix I thought it would be. I will take a close look as soon as I get a little time.

@KennethNielsen
Copy link
Author

  1. At least part of the problems here seems to be that we are subtree'ing the entire tutorials repo. I've searched a little around, and it seems that subtree has the ability to copy only a subfolder of the tut repo into the main one. So, I think that will take care of part of the problem.

  2. The second problem seems to be something with ignore rules and of course subtree immediately merging the other repo in with a commit means that I did not stop midway to evaluate whether the ignore rules makes sense. I think some of the problems with ignore rules will go away, if we copy only the subfolder as mentioned above and then then next time I try. I will do the following to try and fix the problem:

a) Manually copy the files first to evaluate global ignore rules
b) Figure out a way to make subtree add the files but not make the commit, and make that the standard, because then it will always be possible to evaluate that the right stuff is being added.

  1. The last problem seems to be that I started this work of a bad point, when you were in the middle of something. Just let me know what I should base it on and against which branch I should make the PR.

@ScottSoren
Copy link
Member

ScottSoren commented Nov 27, 2023

I have been playing around with it today, mainly to my frustration. As you pointed out, git subtree automatically commits everything every time it merges, without paying attention to the host repository's .gitignore :( .

I don't think just subtree'ing a subfolder of the tutorials repo would solve it, since the tutorials repo might have exported .png's and .csv's and stuff a bit messily about. We could of course clean up the tutorials repo, but I like it if that is, apart from the jupyter notebooks themselves, a bit of a playground, so that the barrier to entry for making a PR to the tutorials repo is as low as possible - and that it can also be used as a place for exported data for other tutorials.
So if that is accepted, it has to be just the .ipynb's which get included by git in ixdat/docs/source/tutorials/tutorials_repo.

I got the "global ignore rules" behaving as I want here:

docs/source/tutorials/tutorials_repo/**/*

(that was a bit tricky to figure out, but it works!)

But then typing
git subtree add --prefix docs/source/tutorials/tutorials_repo https://github.com/ixdat/tutorials.git update_paths --squash
would immediately create two commits indiscriminately adding all the files.

The way I could get the ignored files being ignored as wished was by following it with:

git reset --soft HEAD~2
git rm -r --cached .
git add .
git commit -m "subtree'd but like I want it."

But having that in the workflow makes it easier to just copy+paste the tutorials repository. Then git gets it right from the first commit.

I think lets leave git subtree on the shelf for a bit and have a copy+paste in the tutorials development workflow for now. I'll leave this PR open unmerged but continue work from #133.

@ScottSoren ScottSoren force-pushed the tutorials_with_nbsphinx branch 7 times, most recently from 6642bda to 45445c9 Compare December 1, 2023 10:17
This was referenced Dec 3, 2023
@ScottSoren ScottSoren closed this Jan 16, 2024
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.

None yet

2 participants