[CI/Build] Add sphinx/rst linter for docs#10366
Conversation
|
👋 Hi! Thank you for contributing to the vLLM project. Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can do one of these:
🚀 |
7aa32c2 to
430a242
Compare
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
bed41b3 to
9b553ac
Compare
russellb
left a comment
There was a problem hiding this comment.
I took a look at this same linter earlier this week. I didn't post it because it didn't seem to find anything interesting. All I had to disable to get it to run cleanly was:
sphinx-lint --disable trailing-whitespace,missing-final-newline docsand neither of those warnings seem worth enforcing. My conclusion was that the most important issues are already caught as part of the docs build, which already happens on every PR.
In other words, this didn't seem worth it to me. Did you find anything I missed?
|
That's fair, I totally understand if those particular warnings aren't worth enforcing. A few things I'd advocate as helpful for future documentation changes though are the specific the backticks, spaces, underscores needed by things like hyperlinks and roles which don't break the doc build. You'd have to wait for the doc build to complete and only catch these during review. |
russellb
left a comment
There was a problem hiding this comment.
In other words, it's quicker to find issues using this during the dev (writing) phase than doing full doc builds? I'd agree with that. To make that a little easier, I'd include it in format.sh, as well. I have that in a branch if you want to cherry-pick it to your branch.
https://github.com/vllm-project/vllm/compare/main...russellb:vllm:rst-linting?expand=1
.github/workflows/sphinx-lint.yml
Outdated
| - name: Install dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install sphinx-lint |
There was a problem hiding this comment.
if you pull in my commit, I would change this to:
| pip install sphinx-lint | |
| pip install -r requirements-lint.txt |
.github/workflows/sphinx-lint.yml
Outdated
| python -m pip install --upgrade pip | ||
| pip install sphinx-lint | ||
| - name: Linting docs | ||
| run: sphinx-lint docs |
There was a problem hiding this comment.
and I would change this to:
| run: sphinx-lint docs | |
| run: tools/sphinx-lint.sh |
This ensures that we use the same arguments to the tool for both CI and local usage.
btw, I'm also happy to do the opposite - pull your commit into my branch and make the suggested updates. You will still get co-author credit either way. Just let me know how I can help! |
Run `sphinx-lint` from `format.sh` as a quick check for any doc formatting mistakes. Signed-off-by: Russell Bryant <rbryant@redhat.com> (cherry picked from commit 1100f67)
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
russellb
left a comment
There was a problem hiding this comment.
thanks for the updates! one minor comment
| branches: | ||
| - main | ||
| paths: | ||
| - "docs/**" | ||
| pull_request: | ||
| branches: | ||
| - main |
There was a problem hiding this comment.
any reason you have paths set for pushes but not PRs?
There was a problem hiding this comment.
No reason, nice catch!
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
|
@russellb, thanks for the offer to help (and the reviews), I have to get my git practice in whenever I can 😅 I think I got this one right. |
DarkLight1337
left a comment
There was a problem hiding this comment.
Thanks for improving the quality of docs!
|
Please merge from main to fix the test failures. |
Currently there's no linter/style checker for the docs, this PR offers sphinx-lint to clean up stylistic and formatting errors that don't show up in the build.