Feat: support subfolder for skill_id #788
Conversation
wangxingjun778
had a problem deploying
to
testci
GitHub Actions
Error
Summary of ChangesHello @wangxingjun778, 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 significant feature that allows users to specify a subfolder within a skill repository when referencing 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 adds support for specifying a subfolder in a skill_id, allowing for more granular skill loading from ModelScope Hub. The changes include updating the valid_repo_id function, modifying the skill preprocessing logic to handle the new format, and updating documentation.
My review found a few issues:
- There are typos in the example skill IDs in the documentation files.
- In
agent_skill.py, the logic for handling skill IDs could be improved for clarity and to fix a bug in logging. Theskill_idvariable is being reassigned, which makes the code harder to follow and causes the log message to be incorrect.
I've provided suggestions to fix these issues. Overall, the changes look good and implement the intended feature.
| ``` | ||
|
|
||
| - skill_id_or_dir: Path to the local skill directory or skill ID from The ModelScope hub. | ||
| - skill_id_or_dir (str): e.g. 'path/to/skill-directory', 'ms-agent/skill_examples', 'ma-agent/skill_examples/pdf' (in the form of `owner/skill_name` or `owner/skill_name/subfolder`) |
There was a problem hiding this comment.
There appears to be a typo in the example skill ID. It should likely be ms-agent instead of ma-agent to maintain consistency with other examples and the pull request description.
| - skill_id_or_dir (str): e.g. 'path/to/skill-directory', 'ms-agent/skill_examples', 'ma-agent/skill_examples/pdf' (in the form of `owner/skill_name` or `owner/skill_name/subfolder`) | |
| - skill_id_or_dir (str): e.g. 'path/to/skill-directory', 'ms-agent/skill_examples', 'ms-agent/skill_examples/pdf' (in the form of `owner/skill_name` or `owner/skill_name/subfolder`) |
docs/zh/Projects/智能体技能.md
Outdated
There was a problem hiding this comment.
There appears to be a typo in the example skill ID. It should likely be ms-agent instead of ma-agent to maintain consistency with other examples.
| - skill_id_or_dir (str): 示例： 'path/to/skill-directory', 'ms-agent/skill_examples', 'ma-agent/skill_examples/pdf' (格式为 `owner/skill_name` or `owner/skill_name/subfolder`) | |
| - skill_id_or_dir (str): 示例： 'path/to/skill-directory', 'ms-agent/skill_examples', 'ms-agent/skill_examples/pdf' (格式为 `owner/skill_name` or `owner/skill_name/subfolder`) |
| skill_id_parts: List[str] = skill_id.split('/') | ||
| subfolder = skill_id_parts[-1] if len( | ||
| skill_id_parts) == 3 else None | ||
| allow_pattern: str = f'{subfolder}/*' if subfolder else None | ||
| skill_id = '/'.join( | ||
| skill_id_parts[:2]) if subfolder else skill_id | ||
|
|
||
| skill_dir: str = snapshot_download( | ||
| repo_id=skill_id, | ||
| allow_patterns=allow_pattern, | ||
| ) | ||
| logger.info( | ||
| f'Downloaded skill from hub: {skill_id} to {skill_dir}') |
There was a problem hiding this comment.
The current implementation reassigns the skill_id variable. This can be confusing and leads to an incorrect log message, as the modified skill_id (the repository path without the subfolder) is logged instead of the original full skill ID. It's better to use a new variable for the repository ID to improve clarity and fix the logging.
| skill_id_parts: List[str] = skill_id.split('/') | |
| subfolder = skill_id_parts[-1] if len( | |
| skill_id_parts) == 3 else None | |
| allow_pattern: str = f'{subfolder}/*' if subfolder else None | |
| skill_id = '/'.join( | |
| skill_id_parts[:2]) if subfolder else skill_id | |
| skill_dir: str = snapshot_download( | |
| repo_id=skill_id, | |
| allow_patterns=allow_pattern, | |
| ) | |
| logger.info( | |
| f'Downloaded skill from hub: {skill_id} to {skill_dir}') | |
| skill_id_parts: List[str] = skill_id.split('/') | |
| subfolder = skill_id_parts[-1] if len( | |
| skill_id_parts) == 3 else None | |
| allow_pattern: str = f'{subfolder}/*' if subfolder else None | |
| repo_id = '/'.join( | |
| skill_id_parts[:2]) if subfolder else skill_id | |
| skill_dir: str = snapshot_download( | |
| repo_id=repo_id, | |
| allow_patterns=allow_pattern, | |
| ) | |
| logger.info( | |
| f'Downloaded skill from hub: {skill_id} to {skill_dir}') |
| ``` | ||
|
|
||
| - skill_id_or_dir: Path to the local skill directory or skill ID from The ModelScope hub. | ||
| - skill_id_or_dir (str): e.g. 'path/to/skill-directory', 'ms-agent/skill_examples', 'ma-agent/skill_examples/pdf' (in the form of `owner/skill_name` or `owner/skill_name/subfolder`) |
There was a problem hiding this comment.
There appears to be a typo in the example skill ID. It should likely be ms-agent instead of ma-agent to maintain consistency with other examples and the pull request description.
| - skill_id_or_dir (str): e.g. 'path/to/skill-directory', 'ms-agent/skill_examples', 'ma-agent/skill_examples/pdf' (in the form of `owner/skill_name` or `owner/skill_name/subfolder`) | |
| - skill_id_or_dir (str): e.g. 'path/to/skill-directory', 'ms-agent/skill_examples', 'ms-agent/skill_examples/pdf' (in the form of `owner/skill_name` or `owner/skill_name/subfolder`) |
2 participants
owner/skill_name/subfolder, e.g.ms-agent/skill_examples/pdf(refer to: https://modelscope.cn/models/ms-agent/skill_examples)