Conversation
Summary of ChangesHello, 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 refactors the core AgentJet job system to enhance its configurability and stability. It streamlines the naming of a key interchange server feature and overhauls 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. Changelog
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable refactoring of the AgentJetJob class, making it more flexible by loading configurations from a base YAML file and allowing overrides via keyword arguments. This is a great improvement for usability. The consistent renaming of enable_experimental_interchange_server to enable_interchange_server across the codebase improves clarity. The removal of the tune method from AgentJetJob is a good design choice, separating configuration from execution. The addition of the new werewolves_swarm example is a great showcase of the system's capabilities. However, the refactoring of AgentJetJob has introduced a critical bug in the initialization logic that needs to be addressed. I've also noted a few minor issues in the new example code.
| self.config_as_dict: dict = self.build_job_from_yaml(base_yaml_config) | ||
| self.config = Config.update_from_dict_recursive(Config(), self.config_as_dict) |
There was a problem hiding this comment.
There's a critical bug in the __init__ method's logic. The call to self.build_job_from_yaml(base_yaml_config) on line 79 happens before the instance attributes it depends on (like self.experiment_name, self.backbone, and self.experiment_dir) are initialized. These attributes are only assigned values later in the method.
This will lead to an AttributeError when build_job_from_yaml is called, as it tries to access these uninitialized attributes. The logic has a circular dependency:
build_job_from_yamlis called to createself.config.build_job_from_yamlneeds attributes likeself.experiment_dir.- The final value of
self.experiment_diris determined by the override logic (lines 112-128). - The override logic needs
self.configto get the base value from the YAML.
This initialization flow needs to be re-architected to resolve this circular dependency. A possible approach is to first determine the values for parameters required for loading the configuration (like experiment_name, backbone, experiment_dir) by checking the __init__ arguments, and then use them to call read_ajet_hierarchical_config. After the config is loaded, the rest of the parameters can be overridden.
| except: | ||
| pass |
There was a problem hiding this comment.
Using a bare except: is dangerous as it catches all exceptions, including system-exiting ones like SystemExit and KeyboardInterrupt, making it hard to debug and stop the program. It's better to catch Exception at the very least and log the error for debugging purposes.
| except: | |
| pass | |
| except Exception as e: | |
| print(f"Error during rollout for task {task.task_id}: {e}") |
| default_yaml = None | ||
| self.config_as_dict: dict = self.build_job_from_yaml(default_yaml) | ||
| logger.warning(f"Reading config from {base_yaml_config}.") | ||
| time.sleep(1) |
There was a problem hiding this comment.
The time.sleep(1) here is a code smell. Sleeps like this are often used to work around race conditions or to ensure a log message is visible before a potential crash. This can hide underlying issues and make the code's behavior dependent on timing. It would be better to identify and fix the root cause rather than using a sleep.
| default_model = OpenAIChatModel( | ||
| model_name="/mnt/data_cpfs/model_cache/modelscope/hub/Qwen/Qwen/Qwen3-235B-A22B-Instruct-2507/", | ||
| stream=False, | ||
| client_args={"base_url": "http://22.17.52.4:2888/v1"}, | ||
| api_key="no_api_key", | ||
| generate_kwargs={"temperature": 0.01}, | ||
| ) |
There was a problem hiding this comment.
The default_model configuration contains a hardcoded model path and a hardcoded URL. This makes the example less portable and harder to run in different environments. It would be better to fetch these values from environment variables or a configuration file. You'll need to add import os at the top of the file.
| default_model = OpenAIChatModel( | |
| model_name="/mnt/data_cpfs/model_cache/modelscope/hub/Qwen/Qwen/Qwen3-235B-A22B-Instruct-2507/", | |
| stream=False, | |
| client_args={"base_url": "http://22.17.52.4:2888/v1"}, | |
| api_key="no_api_key", | |
| generate_kwargs={"temperature": 0.01}, | |
| ) | |
| default_model = OpenAIChatModel( | |
| model_name=os.getenv("AJET_DEFAULT_MODEL_PATH", "/mnt/data_cpfs/model_cache/modelscope/hub/Qwen/Qwen/Qwen3-235B-A22B-Instruct-2507/"), | |
| stream=False, | |
| client_args={"base_url": os.getenv("AJET_DEFAULT_MODEL_URL", "http://22.17.52.4:2888/v1")}, | |
| api_key="no_api_key", | |
| generate_kwargs={"temperature": 0.01}, | |
| ) |
binary-husky
force-pushed
the
refactor-agentjet-job
branch
from
5a55295 to
76d0e3f
Compare
1 participant
No description provided.