Fix vllm ipykernel patch#3907
Conversation
Add patch for vLLM compatibility in notebook environments.
Patch sys.stdout.fileno for vLLM compatibility in notebooks.
for more information, see https://pre-commit.ci
Summary of ChangesHello @pluesclues, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical compatibility issue preventing Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a patch to fix an issue with vllm in Jupyter and Colab notebook environments by monkey-patching sys.stdout.fileno. The changes in unsloth/__init__.py correctly integrate the new patch function. My review focuses on improving the implementation of the new patch_vllm_for_notebooks function in unsloth/import_fixes.py for better code quality, readability, and robustness. I've suggested a refactoring to simplify the logic and fix style issues like inconsistent indentation.
|
|
||
| def patch_vllm_for_notebooks(): | ||
| import sys | ||
|
|
||
| if "get_ipython" in globals(): | ||
| try: | ||
| shell = get_ipython().__class__.__name__ | ||
| is_notebook = shell == "ZMQInteractiveShell" or "google.colab" in str( | ||
| type(get_ipython()) | ||
| ) | ||
| except NameError: | ||
| is_notebook = False | ||
| else: | ||
| is_notebook = False | ||
|
|
||
| if is_notebook: | ||
| print("Notebook detected: Patching sys.stdout.fileno for vLLM compatibility...") |
There was a problem hiding this comment.
The implementation of this new function can be improved for better readability, consistency, and robustness.
- Indentation: The indentation is inconsistent and does not follow the PEP 8 standard of 4 spaces per level, which is used in the rest of the file.
- Logic Simplification: The notebook detection logic is a bit convoluted. It can be simplified by initializing
is_notebooktoFalseand removing the redundantelseblock. This makes the code cleaner and easier to follow. - Redundant Call:
get_ipython()is called twice. It's more efficient to call it once and store the result in a variable. - Error Handling: The
except NameErrormight not be sufficient. Ifget_ipython()returnsNone, anAttributeErrorwould be raised. It's better to handle both. - Trailing Whitespace: Line 222 contains only whitespace, which should be removed.
I suggest the following refactoring to address these points.
def patch_vllm_for_notebooks():
import sys
is_notebook = False
if "get_ipython" in globals():
try:
ipython_shell = get_ipython()
if ipython_shell:
shell_name = ipython_shell.__class__.__name__
is_notebook = (
shell_name == 'ZMQInteractiveShell' or
'google.colab' in str(type(ipython_shell))
)
except (NameError, AttributeError):
# Fallback in case of unexpected environment.
pass
if is_notebook:
print("Notebook detected: Patching sys.stdout.fileno for vLLM compatibility...")
if hasattr(sys.stdout, 'fileno'):
# Force fileno to return 1 (standard output) to prevent vLLM crashes
sys.stdout.fileno = lambda: 1There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86605dab2e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if "get_ipython" in globals(): | ||
| try: | ||
| shell = get_ipython().__class__.__name__ | ||
| is_notebook = shell == "ZMQInteractiveShell" or "google.colab" in str( | ||
| type(get_ipython()) |
There was a problem hiding this comment.
Detect notebooks via builtins or IPython import
The notebook detection is effectively dead code in normal Jupyter/Colab sessions because get_ipython is injected into builtins, not this module’s globals(). As written, "get_ipython" in globals() is false in notebooks, so is_notebook stays false and the stdout patch never runs—leaving the vLLM crash unmitigated in the exact environment this patch targets. Consider importing get_ipython from IPython or checking hasattr(builtins, "get_ipython") so the detection actually triggers in notebooks.
Useful? React with 👍 / 👎.
* Implement vLLM patch for notebook detection Add patch for vLLM compatibility in notebook environments. * Fix sys.stdout.fileno for vLLM compatibility Patch sys.stdout.fileno for vLLM compatibility in notebooks. * Add patch_vllm_for_notebooks to initialization * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Harden vLLM notebook stdout patch * Use logger for vLLM notebook patch * Clarify vLLM notebook patch log message --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen <danielhanchen@users.noreply.github.com>
Fixing vllm > 0.11.2 imports on collab and ipykernel jupyter notebooks in general.