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

Api docs #164

Merged
merged 31 commits into from
Aug 17, 2023
Merged

Api docs #164

merged 31 commits into from
Aug 17, 2023

Conversation

Peter9192
Copy link
Collaborator

@Peter9192 Peter9192 commented Jul 11, 2023

  • Generates API docs using mkdocstrings and mkdocs gen-pages, with slightly tweaked css
  • Cleans up docstrings and makes them consistent accross datasets
  • Auto-generate documentation of cli recipes similar to api generation
  • Moves dataset recipes from tests to src/recipes/datasets

To do (in follow-up):

  • Transpose documentation? Instead of (notebook/api/cli -> dataset) list like (dataset -> notebook/api/cli)
  • Split up example recipes into individual datasets?

closes #149

@Peter9192
Copy link
Collaborator Author

Peter9192 commented Jul 14, 2023

pytest-dev/pytest#4030

Running doctest as standalone behaves differently then running it through pytest:

# With doctest
python -m doctest -v src/springtime/datasets/NPNPhenor.py

# With pytest
pytest --doctest-modules src/springtime/datasets/NPNPhenor.py -v

Pytest correctly limits terminal width to 80 (the default value), whereas doctest let's it depend on terminal width. Thus, to get reproducible behaviour, the best option is to always run doctests through pytest. We have two options for the pandas repr behaviour:

  • Option 1: Use pytest with default of 80 chars
  • Option 2: Use pytest fixture to customize the printed dfs:
import pandas as pd
import pytest

@pytest.fixture(autouse=True, scope='session')
def pandas_terminal_width():
    pd.set_option('display.width', None)
    pd.set_option("display.max_columns", 7)
    pd.set_option("display.max_colwidth", 20)

The fixture needs to be in conftest.py in the same dir as the module that's being tested.

@Peter9192
Copy link
Collaborator Author

To get copy-pastable output from doctest: --doctest-report none
https://docs.pytest.org/en/7.1.x/how-to/doctest.html#output-format

@Peter9192
Copy link
Collaborator Author

  • Option 1: Use pytest with default of 80 chars

  • Option 2: Use pytest fixture to customize the printed dfs:

Prefer option 1. More reproducible, and also keeps linter happy :-)

@Peter9192
Copy link
Collaborator Author

Code blocks formatted like this render nicely and also work with doctest:

class A:
    """
    Test

    Example:

        ```pycon
        >>> import time
        >>>
        >>> print('something')
        something

        ```

    """

Notes:

@Peter9192
Copy link
Collaborator Author

Having a hard time deciding whether to keep the example notebooks, doctests (or api docs in general), or both. There's signification overlap, but they also have their unique advantages.

  • Notebooks are easier to work with locally to fix them if they're broken
  • Notebooks have a nicer rendering of input vs output (doctest has these >>>, notebook have separate cells)
  • Doctests serve both as examples with output illustration, and tests
  • Notebooks could also be tested, e.g. with nbval, nbmake, or pytest-jupyter
  • Doctests are closer to the source code
  • In both cases, execution time is a challenge (and dependency on server calls)

@Peter9192 Peter9192 marked this pull request as ready for review August 11, 2023 10:49
@Peter9192
Copy link
Collaborator Author

Decided to keep both apidocs examples and notebooks.

Copy link
Collaborator

@sverhoeven sverhoeven left a comment

Choose a reason for hiding this comment

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

I have spunoff #182 to continue doc improvements.

I fixed most of my own problems/suggestions. I hope you like my additions.
I learned a lot about mkdocs during my review.
Having this extra nesting in src/datasets might bite us in the ass when we have a dataset that is both meteo and satellite, but will solve that if that occurs.

Nice work figuring out how to render everything.

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
mkdocs.yml Show resolved Hide resolved
src/springtime/datasets/__init__.py Outdated Show resolved Hide resolved
Comment on lines +24 to +27
years: timerange. For example years=[2000, 2002] downloads data for
three years.
resample: Resample the dataset to a different time resolution. If None,
no resampling.
Copy link
Collaborator

Choose a reason for hiding this comment

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

On subclasses the type of years and resample attributes is rendered incorrectly on the documentation site.
I would expect YearRange and Optional[ResampleConfig], but for example for PEP725Phenor it is Path and Path.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Tried to replicate at https://gist.github.com/sverhoeven/4d4b354e53f473b90378e2f18f30e914 , but got

Name Type Description
weight   The weight of the vehicle
wheels int The number of wheels on the car

on Car subclass, which is not looks like in springtime.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It had to do with the order of the fields, apparently when you docstring inherited fields first then you get your behaviour, if you put them last, you get mine. Final optimal solution could come from mkdocstrings/python#58

mkdocs_gen_files.set_edit_path(full_doc_path, path)

with mkdocs_gen_files.open("clireference/SUMMARY.md", "w") as nav_file:
nav_file.writelines(nav.build_literate_nav())
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice to have some explanation near the yaml content.

I tried to add a clireference/README.md, but got extra SUMMARY item in Recipe Reference nav.

with mkdocs_gen_files.open("clireference/README.md", "w") as index_file:
    intro = ['Recipes to download a dataset. To prevent conflicts please comment out all datasets except one. Run by copying yaml text to file called `recipe.yaml` and run with `springtime recipe.yaml`', '']
    index_file.writelines(intro)

with mkdocs_gen_files.open("clireference/SUMMARY.md", "w") as nav_file:
    nav_file.writelines(['clireference/README.md'] + list(nav.build_literate_nav()))

Do you know a way?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could try static file instead of gen_cliref_datasets.py

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 868fe1b

mkdocs.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

The src/springtime/recipes are included in whl and sdist of hatch build.

It could be nice to have springtime recipe get simple_usecase.yaml command. Similar to https://docs.esmvaltool.org/en/latest/quickstart/running.html#available-diagnostics-and-metrics .

Or we could just not included them on PyPI, see #181

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

let's discuss further in #181

docs/notebooks/datasets_modis_appeears.ipynb Outdated Show resolved Hide resolved
@Peter9192
Copy link
Collaborator Author

Thanks for the review and adding your own suggestions! We now seem to have a new issue with displaying type inheritance:

image

I think the issue appeared only recently due to the latest release of griffe including mkdocstrings/griffe#170.

ps in the screenshot also the internal links appear to have broken.

@Peter9192
Copy link
Collaborator Author

Okay, after a lot of tinkering I figured out that the order of the docstrings is what causes this weird behaviour.

Option 1: first own attributes, then inherited attributes --> repeated type of last own attribute in all inherited docstrings
Option 2: first inherited attributes, then own attributes --> empty type for inherited docstrings

@Peter9192 Peter9192 merged commit dd6020a into main Aug 17, 2023
1 of 2 checks passed
@sverhoeven
Copy link
Collaborator

Ok so you went with option 2, so we are not displaying incorrect data just empty types.

@Peter9192
Copy link
Collaborator Author

Indeed.

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.

Generate API documentation
2 participants