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

Docs #67

Merged
merged 18 commits into from
Dec 15, 2021
Merged

Docs #67

merged 18 commits into from
Dec 15, 2021

Conversation

zmoon
Copy link
Contributor

@zmoon zmoon commented Oct 27, 2021

I put together an initial docs build, addressing #60 and #13.

  • new Conda env xmovie-docs for docs build placed in ci/
    • some of the things can be removed since the nbs don't need to be run
  • using sphinx-book-theme like xarray
  • some API references for the public stuff, adding to the docstrings
  • example gifs/pics currently used in the readme now all generated by the main example nb
    • probably a lot of the readme examples could be removed since they are duplicated in the quickstart nb

To build the docs locally to check it out:

conda env create -f ci/environment-docs.yml
conda activate xmovie-docs
cd docs
sphinx-autobuild . _build/html

Remaining:

  • RTD configuration and link to docs in readme (maybe as the GH page website as well)
  • remove most stuff from readme that is duplicated by the quickstart nb
  • more docstrings?

@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 Oct 27, 2021

Codecov Report

Merging #67 (b35750e) into master (63dd673) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #67   +/-   ##
=======================================
  Coverage   79.62%   79.62%           
=======================================
  Files           3        3           
  Lines         324      324           
  Branches       59       59           
=======================================
  Hits          258      258           
  Misses         43       43           
  Partials       23       23           
Flag Coverage Δ
unittests 79.32% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
xmovie/core.py 87.04% <ø> (ø)
xmovie/presets.py 69.76% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63dd673...b35750e. Read the comment docs.

@jbusecke
Copy link
Owner

jbusecke commented Dec 7, 2021

Hey @zmoon, first of all sorry for the late reply. I was just completely swamped with other projects. Is this PR ready for review or can I help with anything?

@zmoon
Copy link
Contributor Author

zmoon commented Dec 7, 2021

Hi @jbusecke yeah I think it is ready for your initial review. I didn't do this yet:

probably a lot of the readme examples could be removed since they are duplicated in the quickstart nb

I think I was waiting to see what you thought

Also have to configure ReadTheDocs and add link to readme, etc., but can do that after you review the initial docs.

@zmoon zmoon marked this pull request as ready for review December 7, 2021 23:29
@zmoon
Copy link
Contributor Author

zmoon commented Dec 8, 2021

Also, for the rotating globe examples, I think a dataset that covers the whole globe would be more exciting, maybe you have something in mind that would be easy to use?

@jbusecke
Copy link
Owner

jbusecke commented Dec 8, 2021

I think it would be nice to get started with a smaller example and then build on it with further PRs?

I just activated RTD builds for PRs, but I am not sure if it will run if I just restart the GH actions (which I just did).

@jbusecke
Copy link
Owner

jbusecke commented Dec 8, 2021

It seems like it is not triggering. So I guess that is even more reason to merge this now and see how it looks haha.

Copy link
Owner

@jbusecke jbusecke left a comment

Choose a reason for hiding this comment

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

Thanks a ton @zmoon for all these changes. I really appreciate all the work you put into this (I only just now realized how much is fixed here 🤪).

I made a few suggestions. Once those are implemented I will build it locally and then merge?

#
- pip
- pip:
- '-e ../' # xmovie
Copy link
Owner

Choose a reason for hiding this comment

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

I am not sure about this part here. I would prefer to add a short contributor guide, with instructions on how to set up the env and then install xmovie with pip install -e ., because this here might mess with RTD?

Copy link
Contributor Author

@zmoon zmoon Dec 8, 2021

Choose a reason for hiding this comment

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

The issue is that xmovie needs to be installed or at least importable (for setuptools_scm to have worked) since I am loading the version from it. This is a pretty foolproof method. I can confirm it is valid Conda env spec and doesn't mess with RTD.
Xarray does the same thing.
I do think contributor guide with instructions for setting up the envs for normal work and docs work would be a good addition!

docs/conf.py Outdated Show resolved Hide resolved
docs/whats-new.rst Show resolved Hide resolved
xmovie/core.py Show resolved Hide resolved
@zmoon
Copy link
Contributor Author

zmoon commented Dec 8, 2021

It seems like it is not triggering. So I guess that is even more reason to merge this now and see how it looks haha.

Yeah, I don't think it will until the config file is in the default branch. And then if you want to build for PRs you have to enable in your settings (Edit: now I see that you said earlier that you already did this).

@zmoon
Copy link
Contributor Author

zmoon commented Dec 8, 2021

I think it would be nice to get started with a smaller example and then build on it with further PRs?

Ok yeah, future PR for changing or adding example makes sense, just wanted to mention it.

@jbusecke
Copy link
Owner

jbusecke commented Dec 8, 2021

Yeah, I don't think it will until the config file is in the default branch. And then if you want to build for PRs you have to enable in your settings.

Definitely did that, but I think I have encountered this before. Ill merge this once the changes are implemented.

- more detailed install instructions
- trim install instructions in readme
@zmoon
Copy link
Contributor Author

zmoon commented Dec 10, 2021

@jbusecke I think it is ready for you to check out the build locally and let me know what you think

@zmoon zmoon mentioned this pull request Dec 10, 2021
@jbusecke
Copy link
Owner

Just built it locally and it looks great! Ill merge this now and we can see how it looks on RTD

@jbusecke jbusecke merged commit 023abcb into jbusecke:master Dec 15, 2021
@zmoon
Copy link
Contributor Author

zmoon commented Dec 15, 2021

Looks like it got Command killed due to excessive memory consumption during the conda create. Are you able to request a rebuild?

@jbusecke
Copy link
Owner

Oh darn. I retriggered the build, but I am pretty sure it will fail again. Are we executing the notebooks on RTD? We might want to switch back to just pushing rendered notebooks...xmovie might be a bit resource hungry for the RTD resources?

@jbusecke
Copy link
Owner

I also have the option to do this:
image

But I am not quite sure about the implications. Do you have any experience with this @zmoon ?

@zmoon
Copy link
Contributor Author

zmoon commented Dec 16, 2021

Oh darn. I retriggered the build, but I am pretty sure it will fail again. Are we executing the notebooks on RTD? We might want to switch back to just pushing rendered notebooks...xmovie might be a bit resource hungry for the RTD resources?

By default nbsphinx won't execute the nbs unless they are output-free. So we don't have to worry about that (they are stored in the repo with the outputs). That first failure was during the Conda env creation; it didn't even get to the docs build. And I see this time it has been stuck there for a while too. Given that we are not executing the nbs, maybe after #71 I can create a minimal requirements.txt for RTD to use to make this part faster.

I haven't tried the "Remove build environment for latest", but it doesn't sound like it would hurt anything, might as well try it.

@jbusecke
Copy link
Owner

I think reducing the dependencies is a good thing, no matter what. We could also try to use mamba to solve this issue. I believe I have done that in another project of mine (Ill dig through some repos later).

@zmoon
Copy link
Contributor Author

zmoon commented Dec 16, 2021

Also, I didn't properly link them, but I believe you could close #13 and #60 now

@zmoon zmoon deleted the docs branch May 3, 2022 22:35
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