-
Notifications
You must be signed in to change notification settings - Fork 923
[WIP]Support ray #6245
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?
[WIP]Support ray #6245
Conversation
Summary of ChangesHello @tastelikefeet, 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 significantly enhances the system's distributed computing capabilities by integrating Ray. The core purpose is to enable scalable execution of tasks like model sampling and reward model evaluation across multiple devices and nodes. This is achieved through new configuration options, a dedicated argument class for Ray, and the strategic use of Ray worker and function decorators within the sampling pipeline, allowing for efficient resource allocation and parallel processing. 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 Ray support to the Swift framework, enabling distributed execution for sampling tasks. The changes include adding new arguments for Ray configuration, modifying the CLI to handle these arguments, and integrating Ray into the sampling pipeline. The review focuses on correctness and potential issues arising from the integration of Ray.
| for i in range(0, len(argv[1:]), 2): | ||
| arg_name = argv[i] | ||
| arg_value = argv[i + 1] | ||
| if arg_name == '--config': |
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 loop iterates through argv[1:], but accesses elements using argv[i] and argv[i + 1]. This will cause an IndexError when i is the last index in the loop, as argv[i + 1] will be out of bounds. The loop should iterate through the indices of argv directly.
To fix this, iterate through range(1, len(argv), 2) and adjust the indexing accordingly.
| for i in range(0, len(argv[1:]), 2): | |
| arg_name = argv[i] | |
| arg_value = argv[i + 1] | |
| if arg_name == '--config': | |
| for i in range(1, len(argv), 2): | |
| arg_name = argv[i] | |
| if i + 1 < len(argv): | |
| arg_value = argv[i + 1] | |
| else: | |
| break # Handle the case where there is no value for the last argument |
| if isinstance(self.device_groups, str): | ||
| self.device_groups = json.loads(self.device_groups) No newline at end of file |
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 device_groups attribute is loaded as a JSON string, but there's no error handling if the string is malformed. If the string is not a valid JSON, json.loads will throw an exception, crashing the program. Add a try-except block to handle potential json.JSONDecodeError exceptions.
| if isinstance(self.device_groups, str): | |
| self.device_groups = json.loads(self.device_groups) | |
| try: | |
| self.device_groups = json.loads(self.device_groups) | |
| except json.JSONDecodeError: | |
| print("Error decoding device_groups JSON string.") | |
| self.device_groups = None # or some default value |
| self.prepare_sampler() | ||
| self.caches = self.read_cache() |
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 prepare_sampler and read_cache methods are called immediately after the __init__ method, but they are also decorated with @RayHelper.function. This means that when running in a Ray worker, these methods will not be executed, as the RayHelper.function decorator prevents execution within the worker. This could lead to unexpected behavior, as the sampler might not be properly initialized in the Ray workers.
Consider removing the @RayHelper.function decorator from these methods, or ensure that the initialization logic is correctly handled within the Ray worker context.
| self.prepare_sampler() | |
| self.caches = self.read_cache() | |
| def __init__(self, *args, **kwargs): | |
| super().__init__(*args, **kwargs) | |
| self.prepare_sampler() | |
| self.caches = self.read_cache() |
PR type
PR information
Experiment results
Paste your experiment result here(if needed).