Skip to content

[Doc] Add missing mock import to docs conf.py #6834

New issue

Have a question about this project? No Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “No 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? No Sign in to your account

Merged
merged 8 commits into from
Jul 27, 2024

Conversation

hmellor
Copy link
Member

@hmellor hmellor commented Jul 26, 2024

Fixes the issue identified in #6600 (comment)

FIX #6853

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 consists a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of default ones by unblocking the steps in your fast-check build on Buildkite UI.

Once the PR is approved and ready to go, please make sure to run full CI as it is required to merge (or just use auto-merge).

To run full CI, you can do one of these:

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

🚀

@DarkLight1337
Copy link
Member

DarkLight1337 commented Jul 26, 2024

Thanks for fixing this! Is there a way to cause failures like https://readthedocs.org/projects/vllm/builds/25082568/ to fail the CI so we won't run into such issues again? (This is far from the first time the API reference has been broken by a missing mock import)

@DarkLight1337 DarkLight1337 requested a review from simon-mo July 26, 2024 13:28
@DarkLight1337 DarkLight1337 added the ready ONLY add when PR is ready to merge/full CI is needed label Jul 26, 2024
@DarkLight1337 DarkLight1337 enabled auto-merge (squash) July 26, 2024 13:29
@hmellor
Copy link
Member Author

hmellor commented Jul 26, 2024

We could use some combination of nitpicky mode and fail on warning?

This is probably something a RTD admin would have to do though

@DarkLight1337
Copy link
Member

DarkLight1337 commented Jul 26, 2024

Seems like we can update .readthedocs.yaml with those options.

https://docs.readthedocs.io/en/stable/config-file/v2.html

Can you test it out in this PR? (It should fail without the mock import)

auto-merge was automatically disabled July 26, 2024 13:50

Head branch was pushed to by a user without write access

@hmellor
Copy link
Member Author

hmellor commented Jul 26, 2024

@DarkLight1337 I can't seem to get it to fail...

@DarkLight1337
Copy link
Member

python -m sphinx -T -W --keep-going -b html -d _build/doctrees -D language=en . $READTHEDOCS_OUTPUT/html

I think it's because of -W --keep-going. Where did that come from?

@hmellor
Copy link
Member Author

hmellor commented Jul 26, 2024

That is what fail_on_warning adds. It turns warnings into errors and adds keep going so that it doesn't crash at the first warning.

@DarkLight1337
Copy link
Member

Oh, I didn't read the details of the CLI flags and thought it meant to skip the warnings. Hmm, in that case the build should have failed...

@hmellor
Copy link
Member Author

hmellor commented Jul 26, 2024

Oh it did fail in AWS https://buildkite.com/vllm/ci-aws/builds/5709#0190ef62-e04f-4fb9-bfb2-bd29b30540e0

@DarkLight1337
Copy link
Member

Nice! Let's put the fix back in this PR and get it merged then.

@DarkLight1337 DarkLight1337 enabled auto-merge (squash) July 26, 2024 15:57
@DarkLight1337
Copy link
Member

Ok so it seems that we need nitpicky mode to catch missing mock imports, but doing so would cause a bunch of other errors. Let's fix the docs first and revisit the issue later.

@DarkLight1337 DarkLight1337 merged commit c53041a into vllm-project:main Jul 27, 2024
72 checks passed
@hmellor hmellor deleted the fix-docs-1 branch July 29, 2024 09:04
Alvant pushed a commit to compressa-ai/vllm that referenced this pull request Oct 26, 2024
Signed-off-by: Alvant <alvasian@yandex.ru>
LeiWang1999 pushed a commit to LeiWang1999/vllm-bitblas that referenced this pull request Mar 26, 2025
Signed-off-by: LeiWang1999 <leiwang1999@outlook.com>
No Sign up for free to join this conversation on GitHub. Already have an account? No Sign in to comment
Labels
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.

[Doc]: Sampling page is no longer showing up
2 participants