-
-
Notifications
You must be signed in to change notification settings - Fork 10.4k
[New Model] Add Seed-Oss model #23241
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
Conversation
Signed-off-by: jiabin.00 <jiabin.00@bytedance.com>
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 adds support for the Seed-Oss model, including the model implementation, a tool parser, and necessary registry updates. The implementation looks mostly solid, but I've identified a few critical issues that should be addressed before merging. These include leftover debugging print statements, a security vulnerability from using eval()
on model output, and silent error handling that could obscure bugs. I've provided specific suggestions for each of these points.
param_value = eval(param_value) | ||
except: | ||
logger.warning( | ||
f"Parsed value '{param_value}' of parameter '{param_name}' cannot be converted via Python `eval()` in tool '{func_name}', degenerating to string." | ||
) |
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.
Using eval()
on model output is a significant security vulnerability. Model output is untrusted input and can be manipulated to execute arbitrary code on the server. Please replace eval()
with ast.literal_eval
and add import ast
at the top of the file.
param_value = eval(param_value) | |
except: | |
logger.warning( | |
f"Parsed value '{param_value}' of parameter '{param_name}' cannot be converted via Python `eval()` in tool '{func_name}', degenerating to string." | |
) | |
param_value = ast.literal_eval(param_value) | |
except (ValueError, SyntaxError): | |
logger.warning( | |
f"Parsed value '{param_value}' of parameter '{param_name}' cannot be converted via Python `ast.literal_eval()` in tool '{func_name}', degenerating to string." | |
) |
print(architectures) | ||
print(model_config) |
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 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.
Makes sense
except Exception: | ||
pass # Ignore parsing errors during streaming |
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.
Silently ignoring all exceptions with except Exception: pass
is dangerous. It can hide bugs and make debugging very difficult. If an error occurs during argument parsing, it should at least be logged as a warning. This will help identify issues with model output or the parser logic itself.
except Exception:
logger.warning(
"Failed to parse tool arguments during streaming.",
exc_info=True)
👋 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 🚀 |
cc @DarkLight1337 who can help take a look? |
return loaded_params | ||
|
||
|
||
class SeedOssForCausalLM(nn.Module, SupportsLoRA, SupportsPP): |
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.
It looks like this model structure is consistent with llama. Can we directly inherit from llama, just like phi3 does?
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.
We also need to update the supported_models and test
rf"{tool_start_re}(.*?){tool_end_re}|{tool_start_re}(.*?)$", | ||
re.DOTALL) | ||
|
||
# 其余 regex 与原版一致 |
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.
We should add comment in english
@@ -0,0 +1,632 @@ | |||
# SPDX-License-Identifier: Apache-2.0 |
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.
We need to add test for this tool parser, see: https://github.com/vllm-project/vllm/tree/main/tests/tool_use
Now that Seed-OSS is released, I was wondering if there's any progress on this PR. |
thanks for review, i'll fix the comments these days |
Signed-off-by: jiabin.00 <jiabin.00@bytedance.com>
Signed-off-by: jiabin.00 <jiabin.00@bytedance.com>
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.
Already confirmed with @FoolPlayer, both the generation and tool calls are working fine, so I'm adding my stamp.
@FoolPlayer Thank you for contribuion
In addition to the documents and test files, the current PR is lacking some necessary elements, such as ReasoningParser. The current PR does not handle the xml tags such as seed:think.
|
Signed-off-by: Jee Jee Li <pandaleefree@gmail.com>
Signed-off-by: jiabin.00 <jiabin.00@bytedance.com> Signed-off-by: Jee Jee Li <pandaleefree@gmail.com> Co-authored-by: Jee Jee Li <pandaleefree@gmail.com> Signed-off-by: root <xwq391974@alibaba-inc.com>
Signed-off-by: jiabin.00 <jiabin.00@bytedance.com> Signed-off-by: Jee Jee Li <pandaleefree@gmail.com> Co-authored-by: Jee Jee Li <pandaleefree@gmail.com>
Signed-off-by: jiabin.00 <jiabin.00@bytedance.com> Signed-off-by: Jee Jee Li <pandaleefree@gmail.com> Co-authored-by: Jee Jee Li <pandaleefree@gmail.com> Signed-off-by: Xiao Yu <xiao.yu@amd.com>
Signed-off-by: jiabin.00 <jiabin.00@bytedance.com> Signed-off-by: Jee Jee Li <pandaleefree@gmail.com> Co-authored-by: Jee Jee Li <pandaleefree@gmail.com>
Signed-off-by: jiabin.00 <jiabin.00@bytedance.com> Signed-off-by: Jee Jee Li <pandaleefree@gmail.com> Co-authored-by: Jee Jee Li <pandaleefree@gmail.com>
Signed-off-by: jiabin.00 <jiabin.00@bytedance.com> Signed-off-by: Jee Jee Li <pandaleefree@gmail.com> Co-authored-by: Jee Jee Li <pandaleefree@gmail.com>
Signed-off-by: jiabin.00 <jiabin.00@bytedance.com> Signed-off-by: Jee Jee Li <pandaleefree@gmail.com> Co-authored-by: Jee Jee Li <pandaleefree@gmail.com>
Purpose
Add Seed-Oss model.
transformers see: huggingface/transformers#40272
Test Plan
Test Result
(Optional) Documentation Update
Essential Elements of an Effective PR Description Checklist
supported_models.md
andexamples
for a new model.