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

Migrate to mkdocs #81

Draft
wants to merge 46 commits into
base: main
Choose a base branch
from
Draft

Migrate to mkdocs #81

wants to merge 46 commits into from

Conversation

thomasmarwitz
Copy link

@thomasmarwitz thomasmarwitz commented Aug 14, 2024

Demonstrate how mkdocs-jupyter plugin can be used to execute and render Jupyter Notebooks in a similar way to MyST.

Output: https://metalearners--81.org.readthedocs.build/en/81/

TODO:

Copy link

codecov bot commented Aug 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.73%. Comparing base (a4858da) to head (cb6c281).

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #81   +/-   ##
=======================================
  Coverage   94.73%   94.73%           
=======================================
  Files          15       15           
  Lines        1806     1806           
=======================================
  Hits         1711     1711           
  Misses         95       95           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thomasmarwitz thomasmarwitz changed the title Remove sphinx specific formatting, adjust heading syntax Migrate to mkdocs Aug 21, 2024
@thomasmarwitz
Copy link
Author

One issue I discovered with math blocks (= math formulae that are not rendered inline).

For some reason KaTeX is only able to render math blocks if there is no indentation before them which is annoying when dealing with enumerated lists:

Math works, enumeration doesn't (1. and 1.):

Screenshot 2024-08-21 at 10 01 11 Screenshot 2024-08-21 at 10 00 29

Enumeration works, math doesn't:

Screenshot 2024-08-21 at 10 00 59 Screenshot 2024-08-21 at 10 00 46

I haven't looked into MathJax yet. Other solutions could be to use unordered lists or use headings instead:
Screenshot 2024-08-21 at 10 00 02

@kklein
Copy link
Collaborator

kklein commented Aug 27, 2024

Thanks for looking into this @thomasmarwitz !
Is it possible that the subscripts no longer need escaping?

See

Screenshot 2024-08-27 at 9 57 13 AM
from this PR compared to
Screenshot 2024-08-27 at 9 58 59 AM

from https://metalearners.readthedocs.io/en/latest/background.html#x-learner

@thomasmarwitz
Copy link
Author

@kklein Thanks for pointing that out, looks like I completely missed this. You are right, you don't need to escape subscript anymore.

@kklein
Copy link
Collaborator

kklein commented Aug 27, 2024

Aside from the math and enumeration topic mentioned above, what do you see as hurdles and next steps?
Might it make sense to test whether our readthedocs setup works fine with mkdocs?

@pavelzw
Copy link
Member

pavelzw commented Aug 27, 2024

BTW an alternative to readthedocs could be github pages in combination with https://github.com/jimporter/mike. Disadvantage would be that there is no preview for PRs. Advantage would be that there is no external configuration necessary.

@kklein
Copy link
Collaborator

kklein commented Aug 27, 2024

Sounds interesting! Can we have branch-specific deployments with GitHub-pages? Given our somewhat heavy use of formulas, we rely a lot on rendered docs in PRs.

@thomasmarwitz
Copy link
Author

I don't know whether branch based deployments can be accomplished out of the box with mike and gh-pages, that would be interesting to keep an eye on.

I just tried out to host the mkdocs documentation with readthedocs and that worked: https://metalearners--81.org.readthedocs.build/en/81/

@thomasmarwitz
Copy link
Author

During porting the rest of the documentation I noticed:

Mkdocs does (by default) not display methods if docstring is missing. For classes that override a method but don't add a docstring, this method is not listed in the API doc:

Example of method from RLearner:
image

Mkdocs does only include 2 methods (with "own" docstring):
image

Sphinx on the other hand includes these methods because they "inherit" the docstring:
image

@thomasmarwitz
Copy link
Author

@kklein pointed out this mkdocstrings feature 1.

It adds inherited methods (w/ a docstring) to subclasses. However, it still differs slightly from the sphinx behavior:

Screenshot 2024-08-28 at 15 51 13

In the case above, SLearner overwrites evaluate w/o providing a docstring in the overwritten impl. Therefore, this method doesn't appear in contrast to sphinx's way of handling this.

But perhaps this is even desired behavior? In the sense that if you change the implementation, you should also adapt the docstring?

Footnotes

  1. https://mkdocstrings.github.io/python/usage/configuration/members/#inherited_members

@kklein
Copy link
Collaborator

kklein commented Aug 29, 2024

But perhaps this is even desired behavior? In the sense that if you change the implementation, you should also adapt the docstring?

I can totally see that one might want to ask that question. In our case we mostly described the 'contract' of a given method/function in the docstrings, i.e. what a user may provide as input and what they can expect as an output. We don't usually say much about the 'how's.

At times, changing the implementation does not change this contract.

E.g. one might think of the folllowing:

class Shape:
    contour: list[Point]
    def surface_area(self):
        """Return the surface area in square meters."""
        return numerical_integration(self.contour)
        
class Rectange(Shape)
    def surface_area(self):
        return distance(self.cotour[2], self.contour[0]) * distance(self.contour[3], self.contour[1])

In this case we used said sphinx feature in order to DRY and reduce the risk of inconsistencies due to redundancy.

@pavelzw
Copy link
Member

pavelzw commented Aug 29, 2024

Can we have branch-specific deployments with GitHub-pages? Given our somewhat heavy use of formulas, we rely a lot on rendered docs in PRs.

I don't think so, so in this case readthedocs might be more fitting.
Note though, that with mkdocs you can easily spin up a dev docs instance using pixi run docs or pixi run mkdocs serve.

mkdocs.yml Outdated
Copy link
Author

Choose a reason for hiding this comment

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

check "true" possible?

Comment on lines +2 to +3
Hides the In[ ] blocks.
Out[ ] blocks are not displayed.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Hides the In[ ] blocks.
Out[ ] blocks are not displayed.
Hides the In[ ] blocks in rendered jupyter notebooks.
Out[ ] blocks are not displayed.

Comment on lines +9 to +16
/*
Add your custom style here to highlight the target element.
This e.g. triggers when an internal link to a function in the API documentation is clicked.
The function will be the target then.
*/
:target {

}
Copy link
Member

Choose a reason for hiding this comment

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

is this still needed?

Comment on lines +76 to +81
docs = "mkdocs build"
readthedocs = "rm -rf $READTHEDOCS_OUTPUT/html && cp -r site/ $READTHEDOCS_OUTPUT/html"
serve-docs = "mkdocs serve" # postinstall task needs to be executed in 'docs' environment beforehand
Copy link
Member

Choose a reason for hiding this comment

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

how about pixi run docs for interactive docs serving and pixi run docs-build for building the HTML?

i'm also wondering whether we should add --strict to docs-build, xref https://github.com/prefix-dev/pixi/blob/e56f2e6300ba2defbd322fc13a45f224886a89c4/pixi.toml#L122

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.

3 participants