Migrate in-code docs to Google docstyle, document the public API, and gate CI on both - #274
Draft
kozlov721 wants to merge 18 commits into
Draft
Migrate in-code docs to Google docstyle, document the public API, and gate CI on both#274kozlov721 wants to merge 18 commits into
kozlov721 wants to merge 18 commits into
Conversation
Mirrors the luxonis-ml migration (3460597, "Enhance documentation"):
- pyproject: `docformatter` style epytext -> google; enable ruff's `D`
rules with the same ignore list luxonis-ml uses (D100-D105, D107,
D202, D203, D205, D212, D213) in place of the blanket `D1`; add
`docstring-code-format`; drop the duplicate `ICN` select entry.
- Convert every epytext docstring (`@type`/`@param`/`@rtype`/`@return`/
`@raises`) to Google `Args:`/`Returns:`/`Yields:`/`Raises:` sections,
and epytext inline markup (`C{}`, `L{}`) to ``literal`` / `reference`.
- Convert the numpydoc `Parameters`/`----------` blocks the cyclopts CLI
commands use. Cyclopts parses Google style equally, so `--help` output
is unchanged apart from the imperative summaries and a `fron` typo fix.
- Fix all resulting `D` violations, mainly D401 (imperative mood) and
D209/D413 (blank line before closing quotes).
- Correct the `layout` doctests, which showed their expected output as
another `>>>` input line; verified against the real functions.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`pydoctor --docformat=google -W` now exits 0. Link targets: single backticks are cross-references in Google/RST mode, so names pydoctor cannot resolve (stdlib `subprocess.*`, luxonis-ml's `NNArchiveConfig`, `max_files`, `</dev/null`) have to be literals in double backticks, not references. Annotation syntax errors: pydoctor `ast.parse`s every string literal inside an annotation as a forward reference, so a string that is not a valid expression fails -- `Parameter(help="some help")` (space) and `Field(alias="from")` (keyword) do, while `group="grp"` and `["image", "docker-image"]` happen to parse. These 7 warnings predate the docstyle migration. Fixed by taking those strings out of the annotations: - `launcher` had no docstring and carried its six option descriptions in `Parameter(help=...)`. Moved into a Google `Args:` section, which cyclopts reads the same way it already does for every other command -- `--help` output is unchanged apart from `'4g'`-style quoted literals now rendering as highlighted code. - `EncodingConfig.from_` declares its alias via a plain `Field()` default instead of `Annotated[..., Field(...)]`. Same pydantic semantics: alias parsing, serialization alias and the default are all unchanged. Also gitignore the generated `apidocs/` output, as luxonis-ml does. For reference, the pre-migration tree had considerably more: 16 objects with docstring syntax errors, 6 documented-but-nonexistent parameters and several paragraph-indentation warnings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds a `docs` job running `pydoctor --docformat=google -W`, and makes the `unit` job depend on it, so a docstring pydoctor cannot render stops the run before any test job starts. `integration` and `coverage` already chain off `unit`, so the whole test graph is gated transitively. pydoctor reads the sources rather than importing them, so the job installs pydoctor alone -- no project dependencies, no Docker -- and finishes in well under a minute. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds a docstring to every public module, package, class, method and function, and turns the `D1` rules on so the next one cannot be added without documentation. pyproject: `D100`-`D107` are no longer ignored; only the five style D-rules luxonis-ml also ignores (`D202`, `D203`, `D205`, `D212`, `D213`) stay. `tests/*` is exempt from `D103` alone -- a docstring on each of the 410 `def test_*` would be noise, while the module, package, class and helper docstrings around them are not, so those were written. Note that ruff derives publicness from `__all__` where a module defines one, so `D101`/`D102` never fired for `Layout`, `QuantizationMode`, `InputFileType` and `InputFileType.from_path` in `utils.types`, all of which are used across the package. They are documented too, but the rules will not catch the next such symbol. Corrects the input formats along the way. The converter takes ONNX, OpenVINO IR and TFLite -- not ONNX alone -- and the accepted set is per-target rather than global: the CLI rejects an IR for anything but RVC2/RVC3, while TFLite reaches RVC4 and Hailo through `snpe-tflite-to-dlc` and `translate_tf_model`, and RVC2/RVC3 through an ONNX conversion their exporter logs as experimental. Each sub-package now states what it accepts and the top-level defers to them, so the matrix is documented next to the code that implements it. `.pt`/`.pth` inputs are no longer relevant and are not documented. Everything here is docstring-only except one line of `rvc4/analyze.py`: a `return` inside a `try` moved into an `else` block. That TRY300 has been failing on `main`, and documenting the file brought it under the pre-commit hook for the first time, which blocked the commit. Success now returns from the `else`, the recoverable-error path still falls through to the retry, and any other exception still propagates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds a `pre-commit` job running the hooks over the whole tree, ahead of the `docs` job that was already gating the tests. `docs` now needs it and `unit` needs `docs`, so lint, formatting and the docs build all have to pass before any test job starts -- including the integration jobs, which occupy a runner with a device attached. Runs the hooks with `prek` rather than `pre-commit`, as luxonis-train and luxonis-ml's `dev/uv` branch do. Both invoke it through `uv run --locked`; modelconverter installs from requirements files, so the job pips it in directly, the same way the docs job installs pydoctor. `prek` reads `.pre-commit-config.yaml` unchanged, so no hook definitions move. `requirements-dev.txt` swaps `pre-commit` for `prek` so a local checkout gets the same tool. `SKIP: no-commit-to-branch` mirrors luxonis-train: `workflow_call` can check this workflow out on a protected branch, where that hook would fail a lint run that has nothing to do with committing. It still applies to local commits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## chore/type-gate-and-coverage #274 +/- ##
=============================================================
Coverage 99.98% 99.98%
=============================================================
Files 86 87 +1
Lines 7152 7176 +24
=============================================================
+ Hits 7151 7175 +24
Misses 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
The Google docstrings this branch introduced stayed to prose and inline markup, while `luxonis-ml` -- built by the same `pydoctor --docformat=google` invocation -- leans on `:math:`, doctests, `.. deprecated::` and admonitions throughout. Nothing stood in the way of those here; they had simply not been reached for. Adopt them where there is something to say that prose said worse: - `.. deprecated::` on the two config options that already raise a `DeprecationWarning`, versioned from the tags that first carried them. - `:math:` for the normalization the exporters bake into the graph, on the pass that inserts it and on the fields that parametrize it. - `.. code-block:: yaml` for the single-stage shorthand, which until now was described but never shown. - `.. warning::` where a call mutates a caller's object or deletes a directory, and `.. note::` for the toolchains that only exist inside a target's image. - Doctests on the pure helpers, whose examples were prose before. Two constraints found on the way, both load-bearing: Docstrings in `__main__.py` are read twice -- by pydoctor, and by cyclopts to build `--help`, which renders them as plain text. A directive there reaches the terminal verbatim, so the CLI commands are deliberately left in prose. Doctests nobody runs are just comments that look tested, so they are executed as part of the unit suite. `--doctest-modules` cannot be turned on wholesale the way `luxonis-ml` does -- importing every module would pull in the vendor toolchains -- so the package is walked and the modules that import on the host contribute their doctests, the rest reported as skipped. A guard test fails if that skip path ever swallows them all. `:math:` brings LaTeX backslashes, and so `r"""`, matching what `luxonis-ml` already does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rebase this branch onto the type gate work. The type gate branch is the base: it supplies the code, the renames and the type fixes. This branch adds the Google docstrings on top of them. The two branches touch the same lines for three reasons: - The type gate branch renamed `packages/` to `platforms/` and `Target` to `Platform`. - It made many members private. - It moved the public methods above the private ones. The resolution keeps all three changes and moves each docstring to the member it belongs to. The docstrings now use the new names, the `Args` keys included. New code from the type gate branch gets a Google docstring, because this branch turns on the `D1` rules and gates CI on a warning-free `pydoctor` build. The CI chain is now `pre-commit`, `docs`, `typecheck`, `unit`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UhTizdLYx2W6YzR3p1mx6o
`get_optional_option` kept two Epytext fields below its new `Args:` block. Under `--docformat=google` pydoctor reads them as plain text, so the docs showed the raw `@return:` line and marked the return value as undocumented. The docs build does not warn about this, so CI stayed green. These were the last two Epytext lines in the package. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UhTizdLYx2W6YzR3p1mx6o
The skip caught every `ImportError` and reported each one as a missing vendor toolchain. A broken import anywhere in the package therefore turned the doctests green while they ran nothing, and the skip reason named a cause the test never checked. Only the three toolchains that live in the target Docker images are an expected failure now. Every other import error fails the test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UhTizdLYx2W6YzR3p1mx6o
The `Attributes:` blocks named seven attributes that the classes do not have. Each value is stored under a leading underscore, so pydoctor listed the documented names beside the real ones, which it marked as undocumented. A reader who followed the API docs got an `AttributeError`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UhTizdLYx2W6YzR3p1mx6o
The launcher docstring becomes the description of the whole application, so `modelconverter --help` told the user that every command runs in a docker container. Only `convert`, `infer` and `shell` do. The other commands are meta commands, and they always run on the host. The summary now states what the tool does, and the text below it names the three commands that use a container. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UhTizdLYx2W6YzR3p1mx6o
`Config.get_stage_config` raises a `KeyError` for an unknown stage name, and the docstring listed only the `ValueError`. A caller that follows the docs and catches `ValueError` misses it. `process_nn_archive` also accepts a directory that already holds an unpacked archive, which the docstring denied, and it raises two `RuntimeError`s that the docstring did not mention. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UhTizdLYx2W6YzR3p1mx6o
The `D` rules were selected without a convention, so ruff kept the rules that the Google convention switches off. `D413` is one of them. It belongs to the numpy style, and it demanded a blank line after the last section of every docstring. The convention now matches the style the project writes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UhTizdLYx2W6YzR3p1mx6o
The merge commit added this file, and neither of its parents holds it. It has no content, and nothing refers to it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UhTizdLYx2W6YzR3p1mx6o
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Our in-code documentation is mid-migration to Google docstyle.
luxonis-mlhasmoved;
modelconverterwas still on Epytext, and its CLI commands used a thirdstyle (numpydoc) on top of that. This brings
modelconverterin line withluxonis-ml's3460597("Enhance documentation"), documents the public API,and puts both under CI so neither can regress.
The docs also could not be built:
pydoctoremitted 14 warnings, so awarnings-as-errors build was impossible.
Specification
Docstyle migration.
docformatterstyleepytext→google, ruff'sDrules enabled with the same ignore list
luxonis-mluses. Every Epytextdocstring converted (
@type/@param/@rtype/@return/@raises→Args:/Returns:/Yields:/Raises:;C{}/L{}→ literals/references). Thecyclopts CLI commands' numpydoc
Parametersblocks are converted too — theserender into
modelconverter --help.Warning-free docs build.
pydoctor --docformat=google -Wnow exits 0. Twocauses: single backticks are cross-references in Google/RST mode, so
unresolvable names had to become literals; and pydoctor
ast.parses everystring literal inside an annotation, so
Parameter(help="some help")andField(alias="from")failed to parse.launcher's six option descriptionsmoved from
Parameter(help=...)into its (previously absent) docstring, whichcyclopts reads identically;
EncodingConfig.from_now uses a plainField()default. Those 7 warnings predate this PR.
Public API documented and enforced. Every public module, package, class,
method and function now has a docstring, and
D100–D107are enforced.tests/*is exempt fromD103alone — a docstring on each of the 410def test_*would be noise, while the module, class and helper docstringsaround them are not.
Input formats corrected. The converter takes ONNX, OpenVINO IR and TFLite,
and the accepted set is per-target: the CLI rejects an IR for anything but
RVC2/RVC3, TFLite reaches RVC4/Hailo via
snpe-tflite-to-dlcandtranslate_tf_model, and RVC2/RVC3 via an ONNX conversion their exporter logsas experimental. Each sub-package states what it accepts; the top-level defers
to them.
.pt/.pthare not documented, being no longer relevant.CI. A
pre-commitjob (runningprek, asluxonis-trainandluxonis-ml'sdev/uvbranch do) and adocsjob (pydoctor -W) gate everytest job:
pre-commit → docs → unit → integration → coverage.Dependencies & Potential Impact
No runtime dependency changes.
requirements-dev.txtswapspre-commit~=4.6for
prek~=0.4.13;.pre-commit-config.yamlis unchanged, as prek reads itas-is. CI gains a
pydoctor~=25.10install in the docs job (no project depsneeded — pydoctor reads sources rather than importing them).
No behaviour change. Every docstring commit is AST-proven docstring-only
against its parent: no signature, logic, import or behaviour changes. Two
deliberate exceptions, both called out in their commit messages:
EncodingConfig.from_declares its alias via a plainField()defaultinstead of
Annotated[..., Field(...)]. Identical pydantic semantics —alias parsing, serialization alias and default all verified unchanged.
rvc4/analyze.py: areturninside atrymoved into anelseblock. That
TRY300has been failing onmain; documenting the file broughtit under the hook and blocked the commit. All three paths are unchanged.
Reviewers should note that ruff derives publicness from
__all__, soD101/D102never fired forLayout,QuantizationMode,InputFileTypeandInputFileType.from_pathinutils.types. They are documented here, but thenewly enabled rules will not catch the next such symbol.
Testing & Validation
pydoctor --docformat=google -Wexits 0 (verified in a venv containing onlypydoctor, i.e. exactly what the CI job does), and verified to exit non-zero
on a deliberately broken reference.
ruff check .— all checks passed, no exclusions.ruff format --check .—116 files clean.
prek run --all-files— 16 hooks pass; verified to exit 1 on a bad file.modelconverter --helpdiffed before/after for all seven commands: everyparameter still renders; the only changes are imperative summaries, a
frontypo fix, and quoted literals now rendering as code.
adversarially verified against the implementation, which corrected 52 factual
errors — including
EncodingConfig'sfrom_/todocumented backwards, aRaises:that cannot fire, and an undocumented in-place mutation of thecaller's config.
tests/conversion) were not run — they need the per-backendDocker images and, for some, an attached device.
Deployment Plan
None / not applicable — documentation, lint configuration and CI only. No
release or rollout step; reverting the branch fully restores the previous state.
AI Usage
Assisted-by: Claude Code:claude-opus-5 ruff pydoctor prek pytest
Submitted code was reviewed by a human: NO
The author is taking the responsibility for the contribution: YES/NO