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

Add python-only development tests #47

Closed
wants to merge 1 commit into from

Conversation

cermeng
Copy link

@cermeng cermeng commented Oct 15, 2024

Add trigger for the test introduced in vllm-project/vllm#9370.

@khluu
Copy link
Collaborator

khluu commented Oct 16, 2024

Does this test only run on-demand?

@cermeng
Copy link
Author

cermeng commented Oct 16, 2024

Does this test only run on-demand?

You mean not to test it in fastcheck? It takes less than 1 minute on my machine if vllm docker cache exists, so I put it in fastcheck. However, this test is not important and I think related scripts will not change frequently, so it's also reasonable not to include it in fast check

@khluu
Copy link
Collaborator

khluu commented Oct 16, 2024

Does this test only run on-demand?

You mean not to test it in fastcheck? It takes less than 1 minute on my machine if vllm docker cache exists, so I put it in fastcheck. However, this test is not important and I think related scripts will not change frequently, so it's also reasonable not to include it in fast check

Oh what I meant is does it need to run for every commit or just whenever the PR author needs & unblocks it. By the way the test should be added here: https://github.com/vllm-project/vllm/blob/main/.buildkite/test-pipeline.yaml (you can add fast_check: true if you want it to run on fastcheck or fast_check_only: true if it only runs on fastcheck )

@cermeng
Copy link
Author

cermeng commented Oct 16, 2024

Does this test only run on-demand?

You mean not to test it in fastcheck? It takes less than 1 minute on my machine if vllm docker cache exists, so I put it in fastcheck. However, this test is not important and I think related scripts will not change frequently, so it's also reasonable not to include it in fast check

Oh what I meant is does it need to run for every commit or just whenever the PR author needs & unblocks it. By the way the test should be added here: https://github.com/vllm-project/vllm/blob/main/.buildkite/test-pipeline.yaml (you can add fast_check: true if you want it to run on fastcheck or fast_check_only: true if it only runs on fastcheck )

My previous reply can still answer this question: it is just a less important and time-consuming test, and I think there is no need to run for every commit

this test is not important and I think related scripts will not change frequently

Why I add the test here is that I found all fast check are based on docker that has already installed vllm(dockerfile test target). If you can take a quick look, this test in the pr runs in a clean docker( I choose the base target in the dockerfile). I'm not so familiar with cool CI/CD, correct me if I'm wrong

@cermeng
Copy link
Author

cermeng commented Oct 17, 2024

Refer to vllm-project/vllm#9370 (comment) We won’t need to add task here after reconstruction. Close it.

@cermeng cermeng closed this Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants