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 Documentation #21

Merged
merged 23 commits into from
Jun 17, 2024
Merged

Update Documentation #21

merged 23 commits into from
Jun 17, 2024

Conversation

elbeejay
Copy link
Contributor

@elbeejay elbeejay commented Feb 28, 2024

Working to improve and update the documentation.

For some reason the configuration to execute the jupyter notebooks isn't working for me locally:

execute:
  execute_notebooks: force

Any suggestions @g4brielvs?

This relates to #16

@g4brielvs g4brielvs self-requested a review February 28, 2024 22:24
@g4brielvs g4brielvs added the documentation Improvements or additions to documentation label Feb 28, 2024
.github/workflows/gh-pages.yml Outdated Show resolved Hide resolved
- file: ../Notebooks/Tutorials/UrbanAreas_tutorials.ipynb
- file: ../Notebooks/Tutorials/LEI_Example.ipynb

- file: notebooks/Tutorials/UrbanAreas_tutorials.ipynb
Copy link
Contributor

@g4brielvs g4brielvs Feb 28, 2024

Choose a reason for hiding this comment

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

@elbeejay Thanks for adding the content. @bpstewar Would you please review which of these notebooks should be added to the gh-pages documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've commented out everything except the Tutorials/ notebooks for now, so currently only those two will be included in the built documentation

@g4brielvs g4brielvs requested a review from bpstewar February 28, 2024 22:30
@g4brielvs
Copy link
Contributor

g4brielvs commented Feb 28, 2024

For some reason the configuration to execute the jupyter notebooks isn't working for me locally:

execute:
 execute_notebooks: force

@elbeejay Thanks! My guess is that the symlink was the issue. Could you please pull and retry?

@elbeejay
Copy link
Contributor Author

elbeejay commented Feb 29, 2024

Unfortunately still not having luck with the notebook execution. The symlink is working as expected and it seems slick @g4brielvs, just need to figure out why the documentation isn't running the notebooks as it should... they are showing up in the locally hosted documentation just without outputs which suggests they aren't being executed

will continue to tweak and edit

@g4brielvs
Copy link
Contributor

g4brielvs commented Feb 29, 2024

Unfortunately still not having luck with the notebook execution.

@elbeejay Because sphinx-build is being used (to generate the docs from docstrings instead of jupyter-boook build), the configuration is docs/conf.py. This was generated from docs/_config.yml, which is inert. Maybe we should remove docs/_config.yml in these cases.

Here the config on docs/conf.py

nb_execution_mode = "force"

@elbeejay
Copy link
Contributor Author

Unfortunately still not having luck with the notebook execution.

@elbeejay Because sphinx-build is being used (to generate the docs from docstrings instead of jupyter-boook build), the configuration is docs/conf.py. This was generated from docs/_config_yml, which is inert. Maybe we should remove docs/_config_yml in these cases.

Here the config on docs/conf.py

nb_execution_mode = "force"

That sounds right. I am thinking we can continue to use _config.yml and use .gitignore to stop tracking the conf.py file. Instead we can use jupyter-book to generate the sphinx configuration file in the CI docs workflow. This aligns with the warning in the jupyter book documentation on the subject.

Unfortunately it looks like the sphinx conf.py file was messed with in isolation from the _config.yml file, so I'm going to work on updating the _config.yml file so that we get a working conf.py file and eventually a working docs build.

@elbeejay
Copy link
Contributor Author

elbeejay commented Mar 1, 2024

Cool, thanks for pointing me in the right direction there @g4brielvs, we've got a successful documentation build now.

I'll summarize the PR below as it looks like a lot of commits and lines changed, but is really fewer than it seems. In this PR:

  • the documentation CI process is changed so the docs job is run on all push and pull request events, but is only deployed when a push is made to the "main" branch
  • the conf.py file was removed and added to the .gitignore to avoid confusion in the future, now the docs build generates the necessary conf.py file on the fly using jupyter-book and the _config.yml file
  • execution of the notebooks and the necessary sphinx extensions have been specified in the _config.yml file
  • the symlink to the notebooks subdirectory was set up and the table of contents was updated to currently display the "Tutorials" notebooks (although the docs build attempts to execute all .ipynb files in the repo)
  • all of the .ipynb files had their outputs and metadata cleared (this now happens during the documentation build as well), we need to do this as otherwise the "environment" data encoded into the metadata causes the notebook execution to fail when the documentation is building and trying to run the notebooks (relates to pre-commit hook for clearing ipynb notebooks #24 to avoid this in the future)
  • added nbconvert to the pyproject.toml for the docs install

Edit: Due to the size of this PR I am going to continue documentation revisions in a new PR - I'd like to merge this if that's ok with you @g4brielvs so we can see how the runner builds the documentation and make further edits and modifications to the documentation from there. I will be unlinking #16 in the main PR text above.

@elbeejay elbeejay marked this pull request as ready for review March 1, 2024 00:16
@elbeejay elbeejay requested a review from g4brielvs March 1, 2024 00:17
@g4brielvs
Copy link
Contributor

g4brielvs commented Mar 1, 2024

@elbeejay I agree totally and thank you for the changes. I'll defer to @bpstewar to when to merge the PR.

@elbeejay
Copy link
Contributor Author

@elbeejay I agree totally and thank you for the changes. I'll defer to @bpstewar to when to merge the PR.

@bpstewar - any thoughts on these changes?

@elbeejay
Copy link
Contributor Author

elbeejay commented May 27, 2024

Added the https://github.com/kynan/nbstripout tool as a pre-commit hook to remove notebook metadata/output automatically. This closes #24.

CI on this PR will still fail because many of the documentation fixes, typos, ruff style fixes, etc. are not implemented until PR #26 which was originally split out to make this PR and that one easier to review...

Copy link
Contributor

@g4brielvs g4brielvs left a comment

Choose a reason for hiding this comment

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

@elbeejay Thank you for proposing these changes. Could you please resolve the conflicts?

@elbeejay
Copy link
Contributor Author

@g4brielvs conflicts are resolved, the pre-commit stuff gets taken care of in #26, which probably also has these new conflicts I need to go resolve...

@g4brielvs g4brielvs self-requested a review June 17, 2024 14:49
@g4brielvs g4brielvs merged commit a1c5366 into main Jun 17, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Development

Successfully merging this pull request may close these issues.

pre-commit hook for clearing ipynb notebooks
2 participants