Skip to content

Conversation

@jovsa
Copy link
Contributor

@jovsa jovsa commented Mar 13, 2025

Fixed some of the broken tests when running pytest tests/

  • fix broken test_pass_manager tests
  • added imports for test_flashinfer so you can run test_fash_attn.py

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

🚀

@mergify mergify bot added the ci/build label Mar 13, 2025
@jovsa jovsa marked this pull request as ready for review March 13, 2025 00:26
Copy link
Member

Choose a reason for hiding this comment

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

this is installed in the docker. we need to build it. the pypi version is just source code. we cannot install it directly from pypi.

Copy link
Member

Choose a reason for hiding this comment

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

@ProExpertProg can you help take a look at this change to see if it is necessary?

Copy link
Collaborator

@houseroad houseroad left a comment

Choose a reason for hiding this comment

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

@zou3519 may give a pass as well.

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.

Thanks for fixing these tests! Been on my TODO list for a while. Could you also add the test to CI?

Comment on lines 34 to 39
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is more complex than it needs to be. The outer if is not necessary, just update the works boolean if necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback!
Made the changes in this commit: 06b6d03

@jovsa
Copy link
Contributor Author

jovsa commented Mar 15, 2025

Thanks for fixing these tests! Been on my TODO list for a while. Could you also add the test to CI?

Done here: b8d0be0

@jovsa jovsa requested a review from ProExpertProg March 15, 2025 06:23
Copy link
Collaborator

@zou3519 zou3519 left a comment

Choose a reason for hiding this comment

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

seems reasonable to me but I'll defer to @ProExpertProg and @youkaichao

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.

Upon closer inspection, I realized the test is very outdated. After I added it in #10273, we ended up drastically changing the caching logic. There is no more bypassing the code cache and throwing inside pickle.dumps. But there is an assertion in pass_manager.add. I added some suggestions to make it easier to make the change, and thanks again for doing this.

Comment on lines 18 to 19
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I would call this callable_uuid as the uuid is provided manually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done here: c325dfa

jovsa and others added 9 commits March 18, 2025 01:06
* fix test_pass_manager tests
* added imports for test_flashinfer

Signed-off-by: JovanSardinha <jovan.sardinha@gmail.com>
Signed-off-by: JovanSardinha <jovan.sardinha@gmail.com>
Signed-off-by: JovanSardinha <jovan.sardinha@gmail.com>
Signed-off-by: JovanSardinha <jovan.sardinha@gmail.com>
Signed-off-by: JovanSardinha <jovan.sardinha@gmail.com>
Signed-off-by: JovanSardinha <jovan.sardinha@gmail.com>
Co-authored-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
Signed-off-by: JovanSardinha <jovan.sardinha@gmail.com>
Co-authored-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
Signed-off-by: JovanSardinha <jovan.sardinha@gmail.com>
Signed-off-by: JovanSardinha <jovan.sardinha@gmail.com>
@jovsa jovsa force-pushed the jovsa/fixing-tests branch from c325dfa to 684175d Compare March 18, 2025 01:07
@jovsa jovsa requested a review from ProExpertProg March 18, 2025 01:23
Signed-off-by: JovanSardinha <jovan.sardinha@gmail.com>
@youkaichao youkaichao enabled auto-merge (squash) March 19, 2025 17:31
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Mar 19, 2025
@ProExpertProg
Copy link
Collaborator

@youkaichao I think the CI is failing because it's using torch 2.5 but the precompiled kernels it downloads use 2.6. Why is torch==2.5.1 being used?

We could also just force-merge and address later

@ProExpertProg
Copy link
Collaborator

@jovsa culd you rebase/merge main for the CI tests?

@youkaichao youkaichao merged commit 70e500c into vllm-project:main Mar 20, 2025
59 checks passed
erictang000 pushed a commit to erictang000/vllm that referenced this pull request Mar 25, 2025
Signed-off-by: JovanSardinha <jovan.sardinha@gmail.com>
Co-authored-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
lulmer pushed a commit to lulmer/vllm that referenced this pull request Apr 7, 2025
Signed-off-by: JovanSardinha <jovan.sardinha@gmail.com>
Co-authored-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
Signed-off-by: Louis Ulmer <ulmerlouis@gmail.com>
shreyankg pushed a commit to shreyankg/vllm that referenced this pull request May 3, 2025
Signed-off-by: JovanSardinha <jovan.sardinha@gmail.com>
Co-authored-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
RichardoMrMu pushed a commit to RichardoMrMu/vllm that referenced this pull request May 12, 2025
Signed-off-by: JovanSardinha <jovan.sardinha@gmail.com>
Co-authored-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
Signed-off-by: Mu Huai <tianbowen.tbw@antgroup.com>
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