-
Notifications
You must be signed in to change notification settings - Fork 63
Fix: fix detached actors #132
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
Conversation
Summary of ChangesHello @ChenZiHong-Gavin, 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 focuses on enhancing the stability and reliability of the GraphGen framework by addressing critical issues related to Ray actor lifecycle management and environment variable propagation. It refines how Ray actors are created and interacted with, centralizes environment configuration for distributed tasks, and integrates LLM actor management directly into the core Highlights
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 introduces several valuable improvements. It addresses Ray actor lifecycle management by removing the lifetime="detached" option for LLM actors, which is a good move for ensuring they are properly cleaned up with the driver process. The changes to LLMServiceProxy to accept an actor handle directly is a nice application of dependency injection. The environment variable handling is also made more robust by explicitly passing them to the Ray runtime environment, ensuring workers have the correct context. I've added a couple of comments regarding logging consistency and a potentially unused/incomplete feature for LLM actor initialization. Overall, these are solid changes that improve the stability and maintainability of the framework.
| def _init_llms(self): | ||
| self.llm_actors["synthesizer"] = init_llm("synthesizer") | ||
| self.llm_actors["trainee"] = init_llm("trainee") |
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.
The _init_llms method initializes self.llm_actors, but this attribute is not used anywhere else in the Engine class. This appears to be either dead code or part of an incomplete feature. If it's not used, it should be removed to avoid confusion and unnecessary initialization of LLM actors, which can be resource-intensive. If it is intended for use by operators, it should be passed to them, for example via a shared context.
Additionally, the LLM types synthesizer and trainee are hardcoded. This is inflexible. A better approach would be to derive the required LLM types from the configuration, for instance by inspecting the nodes in the graph for LLM requirements.
| print(f"Using existing Ray actor: {actor_name}") | ||
| except ValueError: | ||
| print(f"Creating Ray actor for LLM {model_type} with backend {backend}.") |
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 PR fixes detached Ray actor lifecycle issues and improves environment variable handling in the GraphGen framework.