-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Fix unit tests #11503
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
Fix unit tests #11503
Conversation
Summary of ChangesHello @merrymercy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the project's testing infrastructure and documentation. It refines CI dependency handling, updates test execution instructions and best practices, and reorganizes test suites for improved clarity and efficiency. Additionally, a small configuration error in a specific test file has been resolved, contributing to overall test stability. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request aims to fix unit tests by updating CI dependency installation, correcting test command arguments, and refining test documentation and suite configurations. The changes are generally good and improve the testing infrastructure. I've provided a couple of suggestions: one important fix for a CI script to prevent it from failing in non-interactive environments, and a minor grammatical correction in the README for better clarity.
|
|
||
| # Clean up existing installations | ||
| $PIP_CMD uninstall flashinfer_python sgl-kernel sglang vllm torch torchaudio || true | ||
| $PIP_CMD uninstall flashinfer_python sgl-kernel sglang vllm || true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The uv pip uninstall command requires a -y flag to run non-interactively in CI environments. Without it, the command will prompt for confirmation and likely fail. A similar command in this script on line 27 correctly uses the -y flag.
| $PIP_CMD uninstall flashinfer_python sgl-kernel sglang vllm || true | |
| $PIP_CMD uninstall -y flashinfer_python sgl-kernel sglang vllm || true |
| - The CI will run the `per-commit` and `nightly` automatically. If you need special setup or custom test groups, you may modify the workflows in [`.github/workflows/`](https://github.com/sgl-project/sglang/tree/main/.github/workflows). | ||
|
|
||
| - Ensure they are referenced in the respective `run_suite.py` (e.g., `test/srt/run_suite.py`) so they are picked up in CI. For most small test cases, they can be added to the `per-commit-1-gpu` suite. Sort the test cases alphabetically by name. | ||
| - Ensure you added `unittest.main()` for unittest and `pytest.main([__file__])` for pytest in the scripts. The CI run them via `python3 test_file.py`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a minor grammatical error here. For improved clarity, "run" should be "runs".
| - Ensure you added `unittest.main()` for unittest and `pytest.main([__file__])` for pytest in the scripts. The CI run them via `python3 test_file.py`. | |
| - Ensure you added `unittest.main()` for unittest and `pytest.main([__file__])` for pytest in the scripts. The CI runs them via `python3 test_file.py`. |
No description provided.