-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
[CI] Make JSON output tests less likely to fail #17859
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
[CI] Make JSON output tests less likely to fail #17859
Conversation
👋 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 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 🚀 |
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.
LGTM thanks
Hmm the test is still failing |
The part that failed is a spot I missed in my changes. I pushed an update. Hopefully this time it's green ... |
There seems to be yet another error |
This error in the last failure is:
This was the test:
I'll dig into it ... |
I ran the failed test in a loop for about an hour and never hit the same failure. I did hit a different one, but it turned out to be a known issue in xgrammar: mlc-ai/xgrammar#286. It's allowing invalid characters inside of a JSON string, so that's probably causing CI failures on occasion, as well. I think the current PR is probably a net improvement, though there is still work to do. |
FWIW, from the Schemathesis side, you can safely ignore that specific warning, the |
We occasionally see the JSON format structured output tests fail in CI. PR vllm-project#17490 included a change to the prompts asking to make the response as short as possible. This change includes a couple more things to help: - Increase the output length limit. The failures occur when we cut off the output before a JSON object is properly terminated. - Set `additionalProperties` to `False` in each JSON schema used. This should restrict the model from adding properties not specified in the schemas, unnecessarily increasing the size of the JSON object output and making it more likely to hit the length limit before it finishes. Signed-off-by: Russell Bryant <rbryant@redhat.com>
…roperties Signed-off-by: Russell Bryant <rbryant@redhat.com>
workaround for mlc-ai/xgrammar#286 to avoid CI failures Signed-off-by: Russell Bryant <rbryant@redhat.com>
a8da442
to
57337db
Compare
Signed-off-by: Russell Bryant <rbryant@redhat.com> Signed-off-by: Ronald Xu <ronaldxu@amazon.com>
Signed-off-by: Russell Bryant <rbryant@redhat.com>
We occasionally see the JSON format structured output tests fail in CI.
PR #17490 included a change to the prompts asking to make the response
as short as possible. This change includes a couple more things to help:
Increase the output length limit. The failures occur when we cut off
the output before a JSON object is properly terminated.
Set
additionalProperties
toFalse
in each JSON schema used. Thisshould restrict the model from adding properties not specified in the
schemas, unnecessarily increasing the size of the JSON object output
and making it more likely to hit the length limit before it finishes.
Signed-off-by: Russell Bryant rbryant@redhat.com