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

Add automatic docs #2644

Merged
merged 11 commits into from
Dec 18, 2023
Merged

Add automatic docs #2644

merged 11 commits into from
Dec 18, 2023

Conversation

adam-narozniak
Copy link
Contributor

@adam-narozniak adam-narozniak commented Nov 28, 2023

Issue

A single-page API documentation (according to me) is too long and hard to navigate.

Proposal

Introduce automatic docs creation using autosummary extension from sphinx where each class and function has a separate page.

Description

  • Update index.rst for the new docs generation
  • Remove old docs ref-api-flwr (single docs file)
  • Add autosummary templates (they dictate how each file looks like)
  • (Keep the CLI docs; note that the functions are double - the reference is also present in the newly created docs but there are no argaparse arguments)
  • Configure autosummary in conf.py

Questions

Do we need to store the .rst files with documentation (I see some git diffs in the scripts for building the docs but I'm unsure if that affect this part)

Diff between flwr and flwr-datasets

  1. __init __ problem
    In flwr, proper docs of SOME classes are present in the __init__ and right after the class ClassName (I find it especially true in Strategies). In flwr-datasets added docs just after the class statement (no docs in __init __).
    That implies that the parameters to the constructor are doubled in documentation (that was also the case in old docs); see the image below.
    To account for that, I changed slightly the class template (to have the __init __ docs added).

PEP 257 says that the class Docstrings should be (immediately following the class definition) - the most common option in our docs. The constructor (__init __) Docstrings is optional. I think it should be removed in our case form all the classes subclassing Strategy and potentially others (if present).

image
  1. new base template
    the base template will need to be added to flwr-datasets. The need for it was not apparent because there was none standalone function documentation. It is required for consistent naming conventions.

@danieljanes
Copy link
Member

@adam-narozniak thanks for the description.

Re Q1: I agree with your proposal. Let's use the class docstring as the main docstring going forward (instead of the init docstring). This includes fixing it for classes where this isn't the case yet.

Re Q2: Also yes. Let's keep the approach and templates exactly the same across flwr and flwr-datasets.

@adam-narozniak
Copy link
Contributor Author

@danieljanes Great!

  1. Tomorrow I'll find all the classes with docstrings in __init__ and move and merge the docstrings so that they are immediately following the class definition.
  2. I'll also add the base template to the flwr-datasets to make it consistent.

After that I'll also the templates in this PR consistent with the ones in flwr-datasets.

adam-narozniak and others added 4 commits December 7, 2023 15:28
* Update versioned-docs script

* Remove ref-api copy

* Update datasets script

* Remove auto generated files at the end of building
@danieljanes danieljanes enabled auto-merge (squash) December 18, 2023 20:36
@danieljanes danieljanes merged commit fa3745c into main Dec 18, 2023
18 of 30 checks passed
@danieljanes danieljanes deleted the add-automatic-docs branch December 18, 2023 20:45
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