Conversation
Signed-off-by: weedge <weege007@gmail.com>
Signed-off-by: weedge <weege007@gmail.com>
…r on the same session ctx Signed-off-by: weedge <weege007@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @weedge, 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!
Summary of Changes
This pull request introduces concurrency testing for the TTS (Text-to-Speech) generator, and fixes an issue with gender control in the spark_generator_tts module. The concurrency testing is implemented in run_generator_tts.py, allowing for multiple TTS requests to be processed simultaneously. The fix in spark_generator_tts.py ensures that gen_global_token_ids are correctly used for gender control within the same session context. Additionally, the PR updates dependencies and adds support for flash-attn in the transformers generator engine.
Highlights
- Concurrency TTS Test: Introduces concurrency testing for TTS generation using asyncio to simulate multiple concurrent requests. This allows for performance evaluation and identification of potential bottlenecks.
- Gender Control Fix: Fixes an issue in
spark_generator_ttswhere gender control was not working correctly within the same session context. The fix ensures thatgen_global_token_idsare properly used for consistent gender control. - Dependency Updates and Flash Attention Support: Updates dependencies in
run_generator_tts.pyand adds support forflash-attnwhen using the transformers generator engine. This includes installingflash-attnand setting the appropriate environment variables.
Changelog
Click here to see the changelog
- deploy/modal/src/tts/run_generator_tts.py
- Added asyncio and math imports for concurrency support.
- Added conditional installation of
flash-attnfor transformers generator engine. - Added
accelerateas a dependency for vllm. - Introduced
WARMUP_TEXTenvironment variable for warming up the TTS engine. - Implemented concurrent TTS generation using asyncio.gather.
- Added logic to handle
gen_global_token_idsfor gender control across concurrent requests. - Added
nvidia-smicommand execution for debugging GPU memory usage. - Updated default TTS text to a news excerpt.
- Added CONCURRENCY_CN environment variable to control the number of concurrent requests.
- pyproject.toml
- Updated the version of the
achatbotpackage to 0.0.9.7.2.
- Updated the version of the
- src/core/llm/init.py
- Added
lm_device_mapto the arguments passed to the transformers generator.
- Added
- src/core/llm/transformers/generator.py
- Modified the transformers generator to use
device_mapwhen loading the model iflm_device_mapis set, enabling multi-GPU support with accelerate.
- Modified the transformers generator to use
- src/modules/speech/tts/spark_generator_tts.py
- Fixed an issue where
gen_global_token_idswas not being correctly used for gender control within the same session context. Now thegen_global_token_idsare stored in the session context if they are not already present. - Modified the _inference method to use session.ctx.state['gen_global_token_ids'] instead of controll_gen_global_token_ids for token2wav calls.
- Added return statements when sub_tts_speech.size == 0 or pre_sub_tts_speech_size > sub_tts_speech.size to prevent further processing of empty speech chunks.
- Fixed an issue where
- test/modules/speech/tts/test_spark.py
- Added a warmup phase to the test to ensure that
gen_global_token_idsis initialized before the main synthesis. - Increased the temperature setting to 0.9.
- Added assertions to check for the presence of
gen_global_token_idsin the session context after synthesis.
- Added a warmup phase to the test to ensure that
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 is currently in preview and 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 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.
Async calls in the night,
Voices weaving, taking flight,
Concurrency's hum,
A chorus to come,
TTS tests shining bright.
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
The pull request introduces concurrency to the TTS test and fixes an issue with gender control in spark_generator_tts.py. The changes involve adding asyncio, updating dependencies, and modifying the logic for text generation and audio synthesis. Overall, the changes seem well-structured, but there are a few areas that could be improved for robustness and maintainability.
Summary of Findings
- Flash Attention Installation: The code installs
flash-attnin multiple places. It's important to ensure that these installations are necessary and don't conflict with each other, potentially leading to unexpected behavior or performance issues. - Inconsistent ACHATBOT_VERSION Updates: The
ACHATBOT_VERSIONis updated in some places but not others. It's crucial to maintain consistency across all relevant configurations to avoid compatibility issues. - Potential Race Condition in gen_global_token_ids: The logic for handling
gen_global_token_idsinspark_generator_tts.pymight have a race condition if multiple concurrent requests are processed within the same session context. This could lead to incorrect gender control. - Missing Error Handling: The code lacks proper error handling for various operations, such as environment variable retrieval and file operations. This could make it difficult to diagnose and resolve issues in production.
Merge Readiness
The pull request introduces important features and fixes, but there are some potential issues that need to be addressed before merging. Specifically, the flash attention installation, ACHATBOT_VERSION updates, potential race condition in gen_global_token_ids, and missing error handling should be carefully reviewed and resolved. I am unable to approve this pull request, and recommend that the pull request not be merged until these issues are addressed (at a minimum), and that others review and approve this code before merging.
| if session.ctx.state.get("gen_global_token_ids") is None: | ||
| session.ctx.state["gen_global_token_ids"] = controll_gen_global_token_ids |
There was a problem hiding this comment.
This logic for setting gen_global_token_ids might have a race condition if multiple concurrent requests are processed within the same session context. Consider using a thread-safe mechanism to ensure that the gen_global_token_ids is correctly initialized and updated.
if "gen_global_token_ids" not in session.ctx.state:
session.ctx.state["gen_global_token_ids"] = controll_gen_global_token_ids| "find /usr/ -name 'libcuda.so.*'", | ||
| "echo $LD_LIBRARY_PATH", | ||
| f"LD_LIBRARY_PATH=/usr/local/cuda-12.5/compat:$LD_LIBRARY_PATH CMAKE_ARGS='-DGGML_CUDA=on' pip install llama-cpp-python --extra-index-url='https://abetlen.github.io/llama-cpp-python/whl/cu125'", | ||
| "pip install flash-attn --no-build-isolation", |
There was a problem hiding this comment.
Consider whether this installation of flash-attn is necessary here, as it's also installed in the transformers block. Installing it in multiple places might lead to conflicts or unexpected behavior.
| if sub_tts_speech.size == 0: | ||
| break | ||
| return |
There was a problem hiding this comment.
Consider adding error handling here to gracefully handle the case where sub_tts_speech.size is 0. This could involve logging an error message and returning a default value or raising an exception.
if sub_tts_speech.size == 0:
logging.error("sub_tts_speech size is 0")
returnSigned-off-by: weedge <weege007@gmail.com>
1 participant
feat:
cases:
# CONCURRENCY_CN=4 CONCURRENCY_CN=4 GENERATOR_ENGINE=vllm TTS_TAG=tts_generator_spark ACHATBOT_VERSION=0.0.9.post5 IMAGE_GPU=L40S modal run src/tts/run_generator_tts.py CONCURRENCY_CN=4 GENERATOR_ENGINE=sglang TTS_TAG=tts_generator_spark ACHATBOT_VERSION=0.0.9.post5 IMAGE_GPU=L40S modal run src/tts/run_generator_tts.py CONCURRENCY_CN=4 GENERATOR_ENGINE=trtllm TTS_TAG=tts_generator_spark ACHATBOT_VERSION=0.0.9.post5 IMAGE_GPU=L40S modal run src/tts/run_generator_tts.py CONCURRENCY_CN=4 GENERATOR_ENGINE=trtllm_runner TTS_TAG=tts_generator_spark ACHATBOT_VERSION=0.0.9.post5 IMAGE_GPU=L40S modal run src/tts/run_generator_tts.pybash run_generator_tts.sh -s run -t 'hello,你好，我是机器人。|万物之始,大道至简,衍化至繁。|君不见黄河之水天上来，奔流到海不复回。君不见高堂明镜悲白发，朝如青丝暮成雪。人生得意须尽欢，莫使金樽空对月。天生我材必有用，千金散尽还复来。' -g trtllm -d L40s -a tts_generator_spark -c 2 bash run_generator_tts.sh -s run -g vllm -d L40s -a tts_generator_spark -c 4 bash run_generator_tts.sh -s run -g sglang -d L40s -a tts_generator_spark -c 4 bash run_generator_tts.sh -s run -g trtllm -d L40s -a tts_generator_spark -c 4 bash run_generator_tts.sh -s run -g vllm -d L40s -a tts_generator_spark -c 8 bash run_generator_tts.sh -s run -g sglang -d L40s -a tts_generator_spark -c 8 bash run_generator_tts.sh -s run -g trtllm -d L40s -a tts_generator_spark -c 8 bash run_generator_tts.sh -s run -g vllm -d L40s -a tts_generator_spark -c 12 bash run_generator_tts.sh -s run -g sglang -d L40s -a tts_generator_spark -c 12 bash run_generator_tts.sh -s run -g trtllm -d L40s -a tts_generator_spark -c 12fix:
Tip