Skip to content

Conversation

@zou3519
Copy link
Collaborator

@zou3519 zou3519 commented Aug 11, 2025

Purpose

This test wasn't being run in CI

Test Plan

Run test locally.
Wait for CI, read logs

Test Result

Test passed locally

Signed-off-by: Richard Zou <zou3519@gmail.com>
@github-actions
Copy link

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

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 either: Add ready label to the PR or enable auto-merge.

🚀

@zou3519 zou3519 marked this pull request as ready for review August 11, 2025 22:39
@mergify mergify bot added the ci/build label Aug 11, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 correctly adds a missing test, tests/compile/test_config.py, to the CI pipeline. My review focuses on improving the maintainability of this CI step. I've suggested replacing the explicit list of test files with a single command to run all tests in the directory. This will prevent similar omissions in the future and make the CI configuration more robust.

Copy link
Collaborator

@ProExpertProg ProExpertProg left a comment

Choose a reason for hiding this comment

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

Let's merge this as-is and we can organize better later

@ProExpertProg ProExpertProg enabled auto-merge (squash) August 15, 2025 17:30
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Aug 15, 2025
@ProExpertProg ProExpertProg disabled auto-merge August 15, 2025 20:05
Copy link
Member

@yewentao256 yewentao256 left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for the work!

@mgoin mgoin enabled auto-merge (squash) August 19, 2025 23:28
Copy link
Member

@yewentao256 yewentao256 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 for the work!

@mergify
Copy link

mergify bot commented Sep 22, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @zou3519.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Sep 22, 2025
Signed-off-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
@mergify mergify bot removed the needs-rebase label Sep 22, 2025
@ZJY0516
Copy link
Contributor

ZJY0516 commented Sep 25, 2025

This failed again due to

[2025-09-23T15:25:35Z] FAILED compile/test_silu_mul_quant_fusion.py::test_fusion_silu_and_mul_quant[True-TestSiluMulFp8QuantModel-dtype0-128-32] - AssertionError: Tensor-likes are not close!
--
  | [2025-09-23T15:25:35Z]
  | [2025-09-23T15:25:35Z] Mismatched elements: 65 / 128 (50.8%)
  | [2025-09-23T15:25:35Z] Greatest absolute difference: 0.0625 at index (0,) (up to 0.001 allowed)
  | [2025-09-23T15:25:35Z] Greatest relative difference: 0.007049560546875 at index (0,) (up to 0.001 allowed)

@ProExpertProg
Copy link
Collaborator

Yea not sure why it fails here but not on main or locally... Perhaps we just update the tolerance. Also the rmsnorm fusion test succeeds locally for me

@ZJY0516
Copy link
Contributor

ZJY0516 commented Sep 25, 2025

I don't believe adjusting the tolerance is a good idea. While it might hide the numerical issues in the short term, it's not a sustainable solution as it avoids addressing the root cause.

@ProExpertProg
Copy link
Collaborator

@ZJY0516 these tolerances are pretty arbitrary, we don't have a good reason for it to be what it currently is. That's why we rely on E2E lm_eval testing

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.

6 participants