-
Notifications
You must be signed in to change notification settings - Fork 143
[Bug Fix] add requirements and add tensorboard dependency #433
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
base: main
Are you sure you want to change the base?
Conversation
fix lint
Summary of ChangesHello @jiapingW, 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 addresses a reported bug by updating and consolidating the project's dependency management. It ensures that the 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.
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 tensorboard as a dependency and introduces a new requirements.txt file. While adding the dependency is the goal, creating a new requirements.txt file introduces a second source of truth for dependencies, which is already managed in pyproject.toml. This can lead to inconsistencies and maintenance problems. The new tensorboard dependency is also missing from pyproject.toml, which will cause installation failures for users following the standard installation procedure. My recommendation is to remove the newly created requirements.txt and instead add tensorboard to the list of dependencies in pyproject.toml to maintain a single, consistent source for dependency management.
requirements.txt
Outdated
| pre-commit | ||
| torch==2.9.1 | ||
| torchaudio==2.9.1 | ||
| torchvision==0.24.1 | ||
| transformers==4.57.1 | ||
| qwen-vl-utils==0.0.11 | ||
| datasets | ||
| setuptools | ||
| tqdm | ||
| wandb | ||
| psutil | ||
| numpy | ||
| tensorboard |
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.
This new requirements.txt file duplicates dependencies that are already managed in pyproject.toml. This creates two sources of truth for dependencies, which can easily lead to inconsistencies and maintenance issues.
For example:
- This file is missing several dependencies listed in
pyproject.toml(e.g.,accelerate,pydantic,sglang). - It includes
tensorboard, which is not present inpyproject.toml.
The standard practice for modern Python projects is to define dependencies in pyproject.toml. The installation instructions in docs/get_started/installation.md also rely on this file (uv pip install .).
To resolve this, I recommend removing this requirements.txt file and adding tensorboard to the dependencies list in pyproject.toml. This will ensure all dependencies are managed in a single, consistent location.
| pydantic | ||
| sglang[all]==0.5.4 | ||
| openai-harmony | ||
| tensorboard |
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.
While adding tensorboard here is correct for the ROCm-specific environment, this dependency should also be added to the main dependency list in pyproject.toml. Currently, it's missing from pyproject.toml, which means it won't be installed in non-ROCm environments that use pip install . as per the documentation, leading to potential ModuleNotFoundError errors.
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.
I think we can move all requirements-*.txt files to under pyproject.toml as optional dependencies to make everything consistent.
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.
Great! I'll handle it soon.
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.
But now the ROCm env is not updated. Furthermore, its dependencies, such as torch, differ significantly from CUDA. They will be merged again after any AMD-related updates.
|
/gemini review |
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 refactors the project's dependencies by moving ROCm-specific requirements from requirements-rocm.txt to an optional dependency group in pyproject.toml and adding tensorboard as a core dependency. The changes centralize dependency management, which is a good improvement. My review focuses on two points: first, ensuring that the installation instructions for ROCm users are not lost in this transition, as the required --extra-index-url is no longer specified. Second, I suggest sorting the main dependencies list to improve maintainability.
pyproject.toml
Outdated
| rocm = [ | ||
| "torch==2.8.0+rocm6.3", | ||
| "torchaudio==2.8.0+rocm6.3", | ||
| "torchvision==0.23.0+rocm6.3", | ||
| "sglang[all]==0.5.4", | ||
| ] |
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.
By moving the ROCm dependencies from requirements-rocm.txt to this optional dependency group, the instruction to use --extra-index-url https://download.pytorch.org/whl/rocm6.3 is lost. This is crucial for users to install the ROCm-compatible PyTorch wheels. Please add this instruction to the project's installation documentation or add a comment in this file to guide users.
For example, you could add a comment above this section:
# To install with ROCm support, run:
# pip install .[rocm] --extra-index-url https://download.pytorch.org/whl/rocm6.3
rocm = [
# ...
]
pyproject.toml
Outdated
| "tensorboard", | ||
| "torch==2.9.1", | ||
| "torchaudio==2.9.1", | ||
| "torchvision==0.24.1", | ||
| "sglang==0.5.6", |
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.
715cb04 to
a999434
Compare
|
/gemini review |
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 aims to improve dependency management by moving ROCm-specific requirements into pyproject.toml as an optional dependency group and adding tensorboard. While centralizing dependencies is a good step, the current implementation introduces a critical dependency conflict that will break installation for ROCm users. I've provided a detailed comment on how to resolve this. Additionally, I've made a couple of suggestions to sort the dependency lists for better maintainability. Please review the comments for details.
a999434 to
d7ed845
Compare
Motivation
Fix issue. #432
Modifications
Related Issues
Accuracy Test
Benchmark & Profiling
Checklist