Cherry-pick for Profiler bug #5
Conversation
Add dev container Dockerfile and devcontainer.json
Signed-off-by: Alexandre Milesi <[email protected]> Co-authored-by: Alexandre Milesi <[email protected]>
Signed-off-by: Varun Shenoy <[email protected]>
shenoyvvarun
requested review from
CatherineSue,
TJ5,
YouNeedCryDear,
jingqiao,
key4ng,
slin1237 and
upfixer
|
👋 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 🚀 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request cherry-picks two bug fixes: one for multimodal profiler token calculation and another for Llama 3.1 tool call parsing. The changes look correct and align with the goals. However, I've identified a critical security vulnerability and another high-severity issue in the new development Dockerfile. Additionally, there is an unresolved merge conflict in one of the Python files that needs to be addressed.
| && git config --global color.diff.new "green bold" \ | ||
| && git config --global color.diff.whitespace "red reverse" \ | ||
| && git config --global alias.lg "log --color --graph --pretty=format:'%Cred%h%Creset - %s %Cgreen(%cr) %C(bold blue)<%an>%Creset%C(auto)%d%Creset' --abbrev-commit --" \ | ||
| && git config --global http.sslVerify false \ |
There was a problem hiding this comment.
Disabling SSL verification for git globally with http.sslVerify false is a significant security vulnerability. It exposes the container to Man-in-the-Middle (MITM) attacks during git operations, as it will trust any SSL certificate.
This setting should be removed. If there's a specific need to connect to a git repository with a self-signed certificate, it should be configured on a per-repository basis, not globally.
| return self._get_mm_max_tokens(seq_len, | ||
| mm_counts, | ||
| mm_embeddings_only=False) | ||
| >>>>>>> 0e36abf99 ([Bugfix] Correct max tokens for non-contiguous embeds (#21798)) |
There was a problem hiding this comment.
This file contains an unresolved merge conflict marker. This must be resolved before the pull request can be merged.
| RUN apt update -y \ | ||
| && apt install -y --no-install-recommends gnupg \ | ||
| && echo "deb http://developer.download.nvidia.com/devtools/repos/ubuntu2004/amd64 /" | tee /etc/apt/sources.list.d/nvidia-devtools.list \ | ||
| && apt-key adv --fetch-keys http://developer.download.nvidia.com/compute/cuda/repos/ubuntu1804/x86_64/7fa2af80.pub \ |
There was a problem hiding this comment.
Fetching GPG keys over an unencrypted http connection is insecure and vulnerable to Man-in-the-Middle attacks. Please use https instead. Additionally, apt-key is deprecated and its use should be avoided in favor of storing keys in /usr/share/keyrings/.
&& apt-key adv --fetch-keys https://developer.download.nvidia.com/compute/cuda/repos/ubuntu1804/x86_64/7fa2af80.pub \
Cherry pick
vllm-project/vllm#21798
vllm-project/vllm#21842