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

[CI/Build] Add sphinx/rst linter for docs #10366

Merged
merged 7 commits into from
Nov 20, 2024

Conversation

rafvasq
Copy link
Contributor

@rafvasq rafvasq commented Nov 15, 2024

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.

Copy link

👋 Hi! Thank you for contributing to the vLLM project.
Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

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:

  • Add ready label to the PR
  • Enable auto-merge.

🚀

Signed-off-by: Rafael Vasquez <[email protected]>
@rafvasq rafvasq changed the title [CI/Build] Add doc8 lint workflow for docs [CI/Build] Add sphinx/rst linter for docs Nov 15, 2024
Copy link
Collaborator

@russellb russellb left a comment

Choose a reason for hiding this comment

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

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 docs

and 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?

@rafvasq
Copy link
Contributor Author

rafvasq commented Nov 15, 2024

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.

`GPTQ <https://arxiv.org/abs/2210.17323>`_
:class:`~vllm.LLM`

Copy link
Collaborator

@russellb russellb left a comment

Choose a reason for hiding this comment

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

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

- name: Install dependencies
run: |
python -m pip install --upgrade pip
pip install sphinx-lint
Copy link
Collaborator

Choose a reason for hiding this comment

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

if you pull in my commit, I would change this to:

Suggested change
pip install sphinx-lint
pip install -r requirements-lint.txt

python -m pip install --upgrade pip
pip install sphinx-lint
- name: Linting docs
run: sphinx-lint docs
Copy link
Collaborator

Choose a reason for hiding this comment

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

and I would change this to:

Suggested change
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.

@russellb
Copy link
Collaborator

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

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!

russellb and others added 2 commits November 18, 2024 10:43
Run `sphinx-lint` from `format.sh` as a quick check for any doc
formatting mistakes.

Signed-off-by: Russell Bryant <[email protected]>
(cherry picked from commit 1100f67)
Signed-off-by: Rafael Vasquez <[email protected]>
@rafvasq rafvasq marked this pull request as ready for review November 18, 2024 15:48
Signed-off-by: Rafael Vasquez <[email protected]>
Copy link
Collaborator

@russellb russellb left a comment

Choose a reason for hiding this comment

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

thanks for the updates! one minor comment

Comment on lines +5 to +11
branches:
- main
paths:
- "docs/**"
pull_request:
branches:
- main
Copy link
Collaborator

Choose a reason for hiding this comment

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

any reason you have paths set for pushes but not PRs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason, nice catch!

Signed-off-by: Rafael Vasquez <[email protected]>
@rafvasq
Copy link
Contributor Author

rafvasq commented Nov 18, 2024

@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.

Copy link
Collaborator

@russellb russellb left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

Copy link
Member

@DarkLight1337 DarkLight1337 left a comment

Choose a reason for hiding this comment

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

Thanks for improving the quality of docs!

@DarkLight1337 DarkLight1337 enabled auto-merge (squash) November 19, 2024 03:32
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Nov 19, 2024
@DarkLight1337
Copy link
Member

Please merge from main to fix the test failures.

@simon-mo simon-mo merged commit 709c9f1 into vllm-project:main Nov 20, 2024
70 of 72 checks passed
KuntaiDu pushed a commit to KuntaiDu/vllm that referenced this pull request Nov 20, 2024
@rafvasq rafvasq deleted the add-doc-linter branch November 20, 2024 14:36
tlrmchlsmth pushed a commit to neuralmagic/vllm that referenced this pull request Nov 23, 2024
sleepwalker2017 pushed a commit to sleepwalker2017/vllm that referenced this pull request Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/build ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants