Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| description="Path to directory containing skill subdirectories", | ||
| ) | ||
|
|
||
| embed_model_path: Optional[str] = Field( |
There was a problem hiding this comment.
Should this be configurable? We need to use the same embedding as we have for RAG/ToolsRAG.
There was a problem hiding this comment.
We are doing exactly what we are doing in tools Rag. Its local configuration only (per Ashutosh request). Not exposed in CR
ols/src/skills/skills_rag.py
Outdated
| for child in sorted(skills_path.iterdir()): | ||
| if not child.is_dir(): | ||
| continue | ||
| skill_file = child / "skill.md" |
There was a problem hiding this comment.
While this is consistent with what is brought in the PR - the usual format is SKILL.md (uppercase). So some further tuning will be required - making it case insensitive.
ols/src/skills/skills_rag.py
Outdated
| except (UnicodeDecodeError, ValueError): | ||
| continue | ||
| if entry.name == "skill.md": | ||
| parts.insert(0, raw.split("---", 2)[2].strip()) |
There was a problem hiding this comment.
There is a lib for parsing frontmatter:
import frontmatter
post = frontmatter.load("skills/pod-failure-diagnosis/skill.md")
post.metadata # {"name": "pod-failure-diagnosis", "description": "..."}
post.content # everything after the frontmatter blockWorth adding it as a new dependency.
There was a problem hiding this comment.
good idea, fixed
| ) | ||
|
|
||
|
|
||
| class SkillsRAG: |
There was a problem hiding this comment.
Can you explore the option to reuse ToolsRAG? Because this seems like a reimplementation of it.
Maybe you can just create a new instance of that index with skills. Like dynamically build skills as tools
StructuredTool.from_function(
name="skill_pod_failure_diagnosis",
description="Diagnose pods stuck in CrashLoopBackOff, ImagePullBackOff, Pending...",
func=lambda: skill.load_content(),
)Basically:
- Create a ToolsRAG instance
- Index skills into it using name + description as the document text
- At query time, call retrieve_hybrid(query) to get the best match - future work
- Load the matched skill's content and inject into system prompt - future work
There was a problem hiding this comment.
We never considered straight reuse. The conversation was about reusing the approach.
The reason SkillsRAG is its own class (~80 lines) rather than a ToolsRAG instance is that ToolsRAG carries a lot of tool-specific concerns that don't apply to skills: server filtering in both dense and sparse paths, tool_json metadata, _convert_langchain_tool_to_dict, server-grouped return format, and remove_tools for dynamic MCP updates. We'd have to bypass or stub out most of that.
There was a problem hiding this comment.
My point here is that if we wrap each skill as a StructuredTool (just name + description, func never called) and populate a new ToolsRAG instance with them, we can use retrieve_hybrid to find the matching skill and completely avoid creating SkillsRAG.
The retrieval logic is identical between the two classes. The only new code needed is a skill loader that reads directories, parses frontmatter, and returns list[StructuredTool].
It would be just a way of reusing existing components to get the appropriate results for the given query. After that, skills have a different path than tools.
There was a problem hiding this comment.
A single index mixing skills and tools will require post-filtering to separate them. The shared infrastructure (QdrantStore, BM25Okapi) is already reused; SkillsRAG only adds ~30 lines of domain-specific wiring that doesn't fit ToolsRAG's server-centric model (server filtering, tool_json metadata, grouped-by-server return format).
|
is the expectation that this functionality is later migrated into LCORE? Or is it going to remain an OLS-specific capability carried within the OLS codebase? |
|
@bparees We are now focused on troubleshooting the scenario. Any activities regarding OLS adopting LCORE are postponed - thinking about if/how this can be in LCORE too, sadly falls into these postponed activities. But generally speaking, given how "hot topic" the skills are, I would assume that if we have it in OLS, it should be in LCORE before we migrate. |
Llama stack supports skills differently. For now its OLS only. Lets get some experience first and evaluate how useful it is |
|
/retest |
|
@blublinsky: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Description
load_content reads entire skill directory tree recursively, filters binary files via UnicodeDecodeError catch (no suffix allowlist), strips skill.md frontmatter, separates additional files with ## {relative_path} headers
Type of change
Related Tickets & Documents
https://redhat.atlassian.net/browse/OLS-2589
https://redhat.atlassian.net/browse/OLS-2718
https://redhat.atlassian.net/browse/OLS-2589
https://redhat.atlassian.net/browse/OLS-2718
Checklist before requesting a review
Testing