[FIX] Fix the nightly CI by disabling swa mem pool for gemma2 #8693
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @merrymercy, 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!
I've made a crucial change to stabilize our nightly CI. This pull request addresses a recurring failure when testing the google/gemma-2-27b-it model. The core of the fix involves disabling a specific memory pool feature for this model during its test execution, which should prevent the CI from failing due to memory-related issues.
Highlights
- CI Fix: I've implemented a targeted fix to address issues in the nightly CI pipeline.
- Gemma2 Specific Fix: The fix specifically applies to the
google/gemma-2-27b-itmodel. - Memory Pool Disablement: The solution involves adding the
--disable-hybrid-swa-memoryargument to the server launch process when thegoogle/gemma-2-27b-itmodel is being tested.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request fixes a nightly CI failure by disabling the hybrid SWA memory pool for the google/gemma-2-27b-it model. The change is a targeted fix within a test file. While this resolves the immediate issue, I've provided a suggestion to make the implementation safer and more robust. The suggestion includes using an exact match for the model name and discusses moving this model-specific logic into the server's argument parsing (server_args.py) for a more permanent and generalized solution.
test/srt/test_nightly_gsm8k_eval.py
Outdated
| if "google/gemma-2-27b-it" in model: | ||
| other_args.extend(["--disable-hybrid-swa-memory"]) |
There was a problem hiding this comment.
Using in for checking the model name can be brittle. It's safer to use an exact match (==) to avoid unintended matches with other model paths that might contain this string as a substring.
Furthermore, for a more robust and maintainable solution, this model-specific logic should ideally reside in the server configuration rather than a test file. Consider moving this check into python/sglang/srt/server_args.py inside the ServerArgs.__post_init__ method. There, you can detect the model architecture and automatically disable this feature for all Gemma2 models.
Here is an example of how you could implement this in server_args.py:
# In ServerArgs.__post_init__
hf_config = self.get_hf_config()
if "Gemma2ForCausalLM" in hf_config.architectures[0]:
if not self.disable_hybrid_swa_memory:
logger.warning("Disabling hybrid SWA memory for Gemma2 model due to compatibility issues.")
self.disable_hybrid_swa_memory = TrueThis would make the fix apply globally and not just for this test case.
| if "google/gemma-2-27b-it" in model: | |
| other_args.extend(["--disable-hybrid-swa-memory"]) | |
| if model == "google/gemma-2-27b-it": | |
| other_args.extend(["--disable-hybrid-swa-memory"]) |
5b1a7b3 to
b34aabe
Compare
7d86cfb to
8856686
Compare
No description provided.