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

Update arepo docs notebook to be pre-run #2120

Merged
merged 2 commits into from
Sep 12, 2022

Conversation

AlexHls
Copy link
Member

@AlexHls AlexHls commented Aug 22, 2022

📝 Description

Type: 🪲 bugfix | 📝 documentation

Updates the arepo-parser documentation to use a pre-run jupyter notebook to avoid downloading reference files on each build. Additionally the documentation now uses the actual snapshot now since notebook is pre-run anyway, i.e. arepo-snap-utils is required to pre-run the notebook.

🚦 Testing

How did you test these changes?

  • Testing pipeline
  • Other method (describe)
  • My changes can't be tested (explain why)

☑️ Checklist

  • I requested two reviewers for this pull request
  • I updated the documentation according to my changes
  • I built the documentation by applying the build_docs label

Note: If you are not allowed to perform any of these actions, ping (@) a contributor.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Aug 22, 2022

Codecov Report

Merging #2120 (ad9a979) into master (a8a9540) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2120   +/-   ##
=======================================
  Coverage   61.54%   61.54%           
=======================================
  Files          75       75           
  Lines        8635     8635           
=======================================
  Hits         5314     5314           
  Misses       3321     3321           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tardis-bot
Copy link
Contributor

*beep* *bop*

Hi, human.

The docs workflow has succeeded ✔️

Click here to see your results.

andrewfullard
andrewfullard previously approved these changes Aug 22, 2022
@andrewfullard andrewfullard enabled auto-merge (squash) August 22, 2022 14:03
Copy link
Member

@isaacgsmith isaacgsmith left a comment

Choose a reason for hiding this comment

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

I am not a huge fan of the pre-running idea, especially since now it cannot be interactively run as it uses the actual snapshot. If we do go with this though, the few things I would add to this PR:

  • In .gitignore make sure all generated files are included, and make sure any files no longer generated are removed.
  • Make sure all files needed to run the notebook (assuming you have the correct packages installed) like the snapshot are included in the repo so the notebook can be updated when needed.

Edit: I also didn't know that the notebook was switched to being pre-run -- I think I missed that PR. As mentioned before I think it's best to clear output.

@AlexHls
Copy link
Member Author

AlexHls commented Aug 31, 2022

I am not a huge fan of the pre-running idea, especially since now it cannot be interactively run as it uses the actual snapshot. If we do go with this though, the few things I would add to this PR:

* In `.gitignore` make sure all generated files are included, and make sure any files no longer generated are removed.

* Make sure all files needed to run the notebook (assuming you have the correct packages installed) like the snapshot are included in the repo so the notebook can be updated when needed.

Edit: I also didn't know that the notebook was switched to being pre-run -- I think I missed that PR. As mentioned before I think it's best to clear output.

As discussed in the slack, the whole reason to set this notebook to being pre-run is to avoid having to download the huge snapshot files from the LFS every time the docs have to be built. If this is no longer desired, this PR could be closed.
I don't see any other solution to the problem other than having the notebook being pre-run in order to avoid the file download from the LFS (it probably is even less desired to include a several 100MB file in the main repo just to build the docs), but if you have a better solution @smithis7 , let me know.
Also I don't really see people running this notebook interactively unless they are already familiar with Arepo, in which case they most certainly have their own snapshot. The whole JSON thing was a hack to begin with to avoid the arepo-snap-util dependency, and is an edge case at the very best. To generate the JSON file, you will most likely have a valid snapshot anyway.

@andrewfullard what do you think?

@isaacgsmith
Copy link
Member

I am not a huge fan of the pre-running idea, especially since now it cannot be interactively run as it uses the actual snapshot. If we do go with this though, the few things I would add to this PR:

* In `.gitignore` make sure all generated files are included, and make sure any files no longer generated are removed.

* Make sure all files needed to run the notebook (assuming you have the correct packages installed) like the snapshot are included in the repo so the notebook can be updated when needed.

Edit: I also didn't know that the notebook was switched to being pre-run -- I think I missed that PR. As mentioned before I think it's best to clear output.

As discussed in the slack, the whole reason to set this notebook to being pre-run is to avoid having to download the huge snapshot files from the LFS every time the docs have to be built. If this is no longer desired, this PR could be closed. I don't see any other solution to the problem other than having the notebook being pre-run in order to avoid the file download from the LFS (it probably is even less desired to include a several 100MB file in the main repo just to build the docs), but if you have a better solution @smithis7 , let me know. Also I don't really see people running this notebook interactively unless they are already familiar with Arepo, in which case they most certainly have their own snapshot. The whole JSON thing was a hack to begin with to avoid the arepo-snap-util dependency, and is an edge case at the very best. To generate the JSON file, you will most likely have a valid snapshot anyway.

@andrewfullard what do you think?

If that's the issue then I understand, then I would just still remove unnecessary files from the .gitignore. Perhaps also adding a note that the notebook can't be run interactively without the proper files.

auto-merge was automatically disabled September 1, 2022 09:35

Head branch was pushed to by a user without write access

@andrewfullard andrewfullard enabled auto-merge (squash) September 7, 2022 14:01
@andrewfullard
Copy link
Contributor

It's fine by me

@andrewfullard andrewfullard merged commit 84e24af into tardis-sn:master Sep 12, 2022
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.

5 participants