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

Documentation build #22

Merged
merged 19 commits into from
May 31, 2023
Merged

Documentation build #22

merged 19 commits into from
May 31, 2023

Conversation

mscheltienne
Copy link

@mscheltienne mscheltienne commented Mar 17, 2023

Beginning of the documentation build PR.

The documentation is build from the 2 sources:

  • RST (or markdown) files in LaPy/doc
  • The LaPy code and its docstrings
    Which is why it was important to first sort that out.

For the docstrings, I ran pydocstlye LaPy/lapy and fixed all the remaining issues. Mostly the 3 following convention were not followed:

  • D202: No blank line allowed after function docstring.
  • D205: 1 blank line required between summary line and description.
  • D401: First line should be in imperative mood.

And I also enabled it back in the code-style workflow.
I did not however rephrase, add x-ref, or add syntax highlights.


To install the documentation dependencies, you can run pip install LaPy/lapy[doc] from where LaPy is the cloned repository. To build the documentation:

    $ cd LaPy/doc
    $ make html

The documentation is then in LaPy/doc/_build/html. You can open index.html.


For the theme, I've used Furo which I like. For instance, it's used by nilearn. Another popular choice is pydata-sphinx-theme. For instance, it's used by scipy.


WIP.

@mscheltienne mscheltienne marked this pull request as draft March 17, 2023 15:54
@codecov-commenter
Copy link

codecov-commenter commented Mar 17, 2023

Codecov Report

❗ No coverage uploaded for pull request base (master@f756280). Click here to learn what that means.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff            @@
##             master     #22   +/-   ##
========================================
  Coverage          ?   5.29%           
========================================
  Files             ?      14           
  Lines             ?    2359           
  Branches          ?     306           
========================================
  Hits              ?     125           
  Misses            ?    2231           
  Partials          ?       3           
Flag Coverage Δ
unittests 5.29% <0.00%> (?)

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

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

@mscheltienne
Copy link
Author

Short update, not forgetting about this PR; but I'm lacking the bandwidth at the moment to finish it.. we'll get back to it when I can. IIRC, the doc build is complete but there is a number of warnings to resolve.

@m-reuter
Copy link
Member

Hi @mscheltienne , looking at this again. I fixed the typo in the README (in master), so you can pull that into your branch. About the failing doc build I wonder if sphinx is missing a correct path setup in doc/conf.py and therefore does not find the modules?
See eg. https://stackoverflow.com/questions/10324393/sphinx-build-fail-autodoc-cant-import-find-module

@mscheltienne
Copy link
Author

It should not require this sys.path.append anymore, it's part of legacy at this point. IIRC, it's an issue with numpydoc, a simple solution is to switch to sphinx.ext.napoleon instead and drop the numpydoc tables and validation. I'll try to pick this up in the coming weeks, would be nice to finalize this ;)

@mscheltienne
Copy link
Author

mscheltienne commented May 26, 2023

I had a look today, and the issue I faced last time became obvious. The naming convention ;)
In Python, PEP8 says:

Screenshot from 2023-05-26 12-01-13

Now if we look at LaPy, the classes are correctly named, e.g. TriaMesh but the modules are not. They have the same name as the class! Thus, when numpydoc tries to document the class lapy.TriaMesh it fails because it sees the module first.

Several option to resolve this:

  • (1) By-pass this issue by documenting from a different level. For instance, I would document TriaMesh from lapy.TriaMesh instead of from lapy.
  • (2) Fix the module naming to respect PEP8.

Option (1) has the advantage of not impacting the codebase. It is fully backward compatible.
Option (2) is my preferred option, but it is not backward compatible. For instance with TriaMesh, the module would become tria_mesh.py thus:

from lapy import TriaMesh  # would still work (because defined in `__init__.py`
from lapy.TriaMesh import TriaMesh  # would not work anymore

In lapy/__init__.py, you have only 3 classes imported (and thus accessible at the top-level of the package).

from .Solver import Solver  # noqa: F401
from .TetMesh import TetMesh  # noqa: F401
from .TriaMesh import TriaMesh  # noqa: F401

Those would be the 3 only function/classes that would retain backward compatibility in the import if we go with option (2). All other function would fail the import if we change the file name.

Up to you @m-reuter to decide, it depends on the user base and on how you use this package. If 99% of the time you import only those 3 classes defined at the package top-level, backward compatibility is not an issue. If you also import often functions or classes in the modules, then option (1) would be better for this project.

@m-reuter
Copy link
Member

Thanks for the explanation! I also think we should go to option 2 and fix the naming. We will then do a new major release to highlight the broken compatibility. We can easily adjust this in our own dependent projects like brainprint , qc and fastsurfer in case we import it in the old way.

@mscheltienne
Copy link
Author

mscheltienne commented May 26, 2023

Option (2) done, documentation build is green locally and should comeback green here too.
But, before we merge this PR and deploy the documentation, we need to create the gh-pages branch and set the deployment in the settings.

  • Create a branch called gh-pages
  • Copy the content of the gh-pages on my template: https://github.com/mscheltienne/template-python/tree/gh-pages
  • Edit index.html and replace the 3 occurences of https://mscheltienne.github.io/template-python/dev/index.html with https://Deep-MI.github.io/LaPy/dev/index.html
  • Then go to the Settings of the repository (tab on the far right after Code, Issues, Pull request, ...)
  • In the section "Code and automation", click on Pages
  • In the section "Build and deployment", select:
    • Source: Deploy from a branch
    • Branch "gh-pages" and folder "/ (root)"

Once done, please ping me here, and I'll check that the automation is setup. Then we can merge this PR. The workflow is setup so that:

  • pushes on a PR run the documentation build (to make sure it works)
  • pushes on main (e.g. by merging a PR) run the documentation build and deploy it on the gh-pages branch.

Finally, you can find here: https://github.com/Deep-MI/LaPy/suites/13170914153/artifacts/716604501
the current documentation website (you can open index.html). I will detail here or in an issue where you can find the different items documented, how you can customize it, ..

@mscheltienne mscheltienne marked this pull request as ready for review May 26, 2023 11:32
@m-reuter
Copy link
Member

Thanks, this looks really great. I created a gh-pages branch, but am not sure what you mean with copying the content onto your template?

@mscheltienne
Copy link
Author

If you created the branch, I can create a PR to fill it. It will become clear, give me a minute.

@mscheltienne
Copy link
Author

The gh-pages branch doesn't have at all the same content as the main branch. It does not contain the Python code, package, structure, ... instead it contanis the HTML of the website. Once you merge #25, you'll see that the content of gh-pages will be similar to https://github.com/mscheltienne/template-python/tree/gh-pages

@m-reuter
Copy link
Member

thanks, this is done, also the Build and deployment in settings was already setup as you specified (probably the default).

@mscheltienne
Copy link
Author

Unsurprising, when GitHub detects a gh-pages branch with an index.html file, it sets it up automatically.
You can now merge this PR. Any push to main (either direct or by merging a PR) will build the documentation and deploy it on the gh-pages branch in the folder dev. Then the GitHub infrastructure picks it up and deploys the website.

@m-reuter m-reuter merged commit 763251c into Deep-MI:master May 31, 2023
@mscheltienne mscheltienne deleted the doc-build branch May 31, 2023 17:19
@mscheltienne
Copy link
Author

mscheltienne commented May 31, 2023

So.. it did nothing 😅
The branch on LaPy is called master (legacy names) instead of main and the workflows are defined to trigger on main, e.g.

on:
pull_request:
push:
branches: [main]
workflow_dispatch:

I'll open a PR today or tomorrow to fix the trigger from main to master.

@m-reuter
Copy link
Member

We are also planning to rename the branch to main as master is outdated. Probably a good time to do that now.

@mscheltienne
Copy link
Author

Works too. Once done, an empty commit to main will trigger the workflows. Alternatively, you could also trigger them manually from the Actions tab.

@m-reuter
Copy link
Member

worked. would be great if you can at some point explain more on the structure and how to customise. Also why this is under ..dev.. in the path. Thanks.

@mscheltienne
Copy link
Author

Will do, probably Friday! I'm not going to leave you with the infrastructure and no manual ;)

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