Implement lazy loading of datasets and agents & add EarthLink agent #6
Conversation
Summary of ChangesHello @JayKuo1, 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 introduces a foundational architectural improvement by implementing lazy loading for agents and datasets, which enhances the system's flexibility and performance. A major new component, the EarthLink agent, is integrated to streamline geoscience experiment planning. This agent intelligently assesses data availability, formulates detailed experimental plans, and refines them based on data checks, utilizing a suite of specialized tools for climate data interaction and web research. 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
|
![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 lazy loading for datasets and agents, which is a significant improvement for managing dependencies and reducing initial load times. It also adds a new EarthLink agent and its associated tools, configuration, and data handling. The lazy loading mechanism is well-implemented using __getattr__ and a registry. Several new files are added for the EarthLink agent, including data information, tools for CMIP6 and observational data, and utility functions. The .gitignore file is updated to include variables_embedding.jsonl for the new agent. Overall, the changes enhance modularity and extensibility.
| tiktoken_enabled=False, | ||
| tiktoken_model_name="Qwen/Qwen3-Embedding-8B", |
There was a problem hiding this comment.
The tiktoken_model_name is hardcoded to "Qwen/Qwen3-Embedding-8B" when tiktoken_enabled is false. This overrides the CFG.EMBEDDING_MODEL if it's different and tiktoken_enabled is false. This could lead to unexpected behavior if the user intends to use a different HuggingFace model for embeddings while tiktoken_enabled is false. It might be better to use CFG.EMBEDDING_MODEL directly here or clarify the interaction.
| tiktoken_enabled=False, | |
| tiktoken_model_name="Qwen/Qwen3-Embedding-8B", | |
| tiktoken_model_name=CFG.EMBEDDING_MODEL, |
| elif self.mode == "full": | ||
| raise NotImplementedError("Full mode is not implemented yet.") |
There was a problem hiding this comment.
The NotImplementedError for full mode is good for clarity. However, if full mode is not intended to be implemented in the near future, consider removing the full option from the Literal type hint and the mode parameter's default value to prevent users from attempting to use it.
| if len(msg) > 0: | ||
| return "Error in parameters:\n" + "\n".join(msg) + "\nPlease check the parameters and try again." # You can use the file search tool or web search tool to find the related information." | ||
|
|
||
| # assert activity != "DCPP" |
There was a problem hiding this comment.
The commented-out assertion # assert activity != "DCPP" indicates a potential constraint or a known issue. If DCPP activity is truly unsupported or problematic, it should be handled explicitly with an error message rather than a commented-out assertion, or the reason for commenting it out should be documented.
| if ("pass" not in output): | ||
| raise ValueError("Output JSON must contain 'pass' and 'reason' keys.") |
There was a problem hiding this comment.
The ValueError message Output JSON must contain 'pass' and 'reason' keys. is slightly misleading. The reason key is only required if pass is false. The current check if ("pass" not in output) covers the first part, but the second if (not output['pass']) and ("reason" not in output) is more specific. Consider rephrasing the first error message to reflect that pass is always required, and reason is conditional.
| if ("pass" not in output): | |
| raise ValueError("Output JSON must contain 'pass' and 'reason' keys.") | |
| if "pass" not in output: | |
| raise ValueError("Output JSON must contain 'pass' key.") |
| vlmeval/dataset/SciCode/eval/data/*.h5 | ||
|
|
||
| # EarthLink file | ||
| variables_embedding.jsonl No newline at end of file |
There was a problem hiding this comment.
The .gitignore entry variables_embedding.jsonl is added without a preceding newline, which can sometimes lead to issues with the last line not being properly ignored by some git clients or tools. It's a good practice to ensure a newline at the end of the file.
variables_embedding.jsonl
| _cache = LRUCache(maxsize=512) | ||
| _lock = threading.Lock() |
There was a problem hiding this comment.
The _cache and _lock are defined outside the hashable_cache decorator, making them global to the module. This means all functions decorated with hashable_cache will share the same cache and lock. If different decorated functions need independent caches, these should be defined within the decorator closure for each function.
|
|
||
| from scieval.agents import create_agent, get_available_agents | ||
| from scieval.agents.records import EvalRecord, TrajectoryStore | ||
| from scieval.agents.smolagents import SmolAgentsAgent |
There was a problem hiding this comment.
The import from scieval.agents.smolagents import SmolAgentsAgent is removed, but SmolAgentsAgent is still referenced as a default class name in _build_agent_from_config. While the new create_agent factory handles this, it might be cleaner to explicitly import SmolAgentsAgent if it's intended to be a default, or ensure the default is a string that create_agent can resolve without needing a direct import here.
| assert np.all([x == TYPES[0] for x in TYPES]), (dataset_list, TYPES) | ||
| return TYPES[0] |
There was a problem hiding this comment.
The assert np.all([x == TYPES[0] for x in TYPES]) check relies on numpy which is imported but not explicitly used elsewhere in this function. If numpy is only used for this assertion, consider replacing it with a pure Python check (e.g., len(set(TYPES)) == 1) to reduce dependencies or clarify its usage.
| MODALITIES = [DATASET_MODALITY(dname) for dname in dataset_list] | ||
| assert np.all([x == MODALITIES[0] for x in MODALITIES]), (dataset_list, MODALITIES) |
There was a problem hiding this comment.
The assert np.all([x == MODALITIES[0] for x in MODALITIES]) check relies on numpy which is imported but not explicitly used elsewhere in this function. If numpy is only used for this assertion, consider replacing it with a pure Python check (e.g., len(set(MODALITIES)) == 1) to reduce dependencies or clarify its usage.
| if 'A' in data and 'B' in data: | ||
| if 'image' in data or 'image_path' in data: | ||
| warnings.warn(f'Will assume unsupported dataset {dataset_name} as a Custom MCQ dataset. ') | ||
| CustomMCQDataset = __getattr__('CustomMCQDataset') | ||
| return CustomMCQDataset(dataset=dataset_name, **kwargs) |
There was a problem hiding this comment.
The lazy loading for CustomMCQDataset, CustomTextMCQDataset, and CustomVQADataset uses __getattr__ directly. While this works, it's generally better to import these classes at the top of the file if they are frequently used or if the lazy loading mechanism is primarily for external modules. For these internal custom datasets, a direct import might be clearer.
1 participant
See title.