OLS-1379: Add oc tools #2216

onmete · 2025-02-07T08:50:08Z

Description

Adding oc tools.

OLS-1410

Type of change

New feature

openshift-ci-robot · 2025-02-07T08:50:13Z

@onmete: This pull request references OLS-1379 which is a valid jira issue.

In response to this:

Description

Adding oc tools.

Type of change

New feature

Instructions 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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2025-02-07T08:50:56Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign xrajesh for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

onmete · 2025-02-07T08:52:07Z

@asamal4 this should be easy to integrate once you have your PR ready.
I've put there a shot of tools per oc command as we discussed.
I'll add tests later (in this PR), once we conclude the right approach.

onmete · 2025-02-07T08:55:56Z

/hold

asamal4 · 2025-02-12T10:20:38Z

ols/src/tools/oc_cli.py

+            raise e
+
+
+def oc_get(args: list[str]) -> subprocess.CompletedProcess:


input type: Why it is list[str] ? shouldn't it be just str ?
output type: I haven't tested, but I am thinking we can keep this as str (we can convert the output to str)

Yup, output should be a string.
As for the input, technically it is a list of strings. From my experiments, llm had no issue returning it, but I'll try to double-check what is better understandable to the llm (list of strings/one string).

asamal4 · 2025-02-12T10:21:48Z

ols/src/tools/oc_cli.py

+        # List one or more resources by their type and names.
+        oc get rc/web service/frontend pods/web-pod-13je7
+    """
+    result = run_oc(["get", *sanitize_oc_args(args)])


What will be the args value ? part of the command except oc get ?

yes, besides sanitization, we can also catch the case when llm returns not just the args, but the full command oc get whatever instead of just whatever.

Probably we are relying too much on model to generate the command here. Also this will require us to handle lot of corner cases (lot of rules to sanitize).
Anyways let's see if others have any opinion on this.

onmete · 2025-02-19T13:10:32Z

@asamal4 @tisnik I've pushed the oc tools. Check it out please, I'll add tests later (not e2e for tools).

onmete · 2025-02-20T11:29:48Z

/assign @asamal4

openshift-ci-robot · 2025-02-20T11:30:19Z

@onmete: This pull request references OLS-1379 which is a valid jira issue.

In response to this:

Description

Adding oc tools.

OLS-1410

Type of change

New feature

Instructions 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 openshift-eng/jira-lifecycle-plugin repository.

asamal4 · 2025-02-24T05:18:04Z

ols/src/query_helpers/docs_summarizer.py

+        is_final_round: bool,
+    ) -> tuple[AIMessage, TokenCounter]:
+        """Invoke LLM with or without tools based on conditions."""
+        llm = (


Perhaps nitpicking, but can we add this condition to _invoke_llm method itself. We can avoid additional lines of code.

asamal4 · 2025-02-24T05:19:50Z

ols/src/query_helpers/docs_summarizer.py

+            return {}
+
+        logger.info("Introspection enabled - using default tools selection")
+        tools_map: dict = {}  # place for other/default tools


Do we need to have this here ?
in tools.py file we are defining all tools. Default can also be added there.
This will also help us not to have separate description len check.

asamal4 · 2025-02-24T05:29:23Z

We discussed about identifying different test scenarios. But for time being could you please modify e2e test case (just one). So that once introspectionEnabled property, any can simply un-comment the tool calling e2e test suites.

Test any one query for which you don't have to do additional set up.
Test with all 3 provider/models that we are actively testing and modify the response. So that test case will work for all.

onmete · 2025-02-24T13:13:08Z

@asamal4 About the e2e, I would rather do that in a separate PR or after we figure out how to test the tools.
Is there a specific reason why you want it to happen here?

onmete · 2025-02-24T13:15:25Z

pyproject.toml

@@ -1,7 +1,7 @@
 [tool.ruff]

 # description of all rules are available on https://docs.astral.sh/ruff/rules/
-lint.select = ["D", "E", "F", "W", "C", "S", "I", "G", "TCH", "SLOT", "RUF", "C90", "N", "YTT", "ASYNC", "A", "C4", "T10", "PGH", "FURB", "PERF", "AIR", "NPY", "FLY", "PLW2901"]
+lint.select = ["D", "E", "F", "W", "C", "S", "I", "G", "TCH", "SLOT", "RUF", "C90", "N", "YTT", "ASYNC", "A", "C4", "T10", "FURB", "PERF", "AIR", "NPY", "FLY", "PLW2901"]


I've removed the PGH from linting - when we want to ignore some error (eg. false positive) we can do that just by # type: ignore instead of writing down specific - especially in cases when there are more errors in one line.
cc @tisnik

asamal4 · 2025-02-24T14:00:30Z

@asamal4 About the e2e, I would rather do that in a separate PR or after we figure out how to test the tools. Is there a specific reason why you want it to happen here?

Separate PR is fine.. BTW I am only requesting to update the existing one e2e test case.

I am not sure how much time we will take to figure out all the scenarios. But we can still enable tool calling e2e test case, once operator adds the flag (This will happen before we figure out scenarios). This is just to make sure that tool calling is working. I believe we need to make sure that things are working. And this is the reason we have added rhelai test flow with openai url.
Later we can modify or add more test cases based on final conclusion regarding how we want to test.

onmete · 2025-02-24T15:10:20Z

@asamal4 Updated the question, is this what you were after?

asamal4 · 2025-02-25T05:07:24Z

@asamal4 Updated the question, is this what you were after?

Yes. Thank you. Most likely it is going to fail for granite. But we can deal with that in separate PR.
lgtm
Please rebase.

asamal4 · 2025-02-25T08:02:12Z

Based on testing, we will update the tool definition.
Improving granite is also a separate effort.
lgtm as far as this PR is concerned.
/lgtm

tisnik · 2025-02-25T11:29:51Z

ols/src/tools/oc_cli.py

+            if char in blocked_chars:
+                # stop processing further characters in this argument
+                logger.warning(
+                    "Problematic character(s) found in oc tool argument '%s'", arg


probably we should stop processing such input as is, not try to sanitize it further

we'll tackle this in OLS-1443

ols/src/query_helpers/docs_summarizer.py

openshift-ci · 2025-02-25T12:46:59Z

New changes are detected. LGTM label has been removed.

openshift-ci · 2025-02-25T14:28:02Z

@onmete: all tests passed!

Full PR test history. Your PR dashboard.

Instructions 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.

onmete marked this pull request as draft February 7, 2025 08:50

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 7, 2025

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 7, 2025

openshift-ci bot requested review from bparees and joshuawilson February 7, 2025 08:50

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 7, 2025

asamal4 reviewed Feb 12, 2025

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 18, 2025

onmete force-pushed the ols-tools branch from 546e58d to 4c46300 Compare February 19, 2025 13:04

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 19, 2025

onmete force-pushed the ols-tools branch from 4c46300 to fc2c2a2 Compare February 20, 2025 11:29

onmete marked this pull request as ready for review February 20, 2025 11:29

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 20, 2025

openshift-ci bot assigned asamal4 Feb 20, 2025

openshift-ci bot requested a review from xrajesh February 20, 2025 11:30

asamal4 reviewed Feb 24, 2025

View reviewed changes

onmete force-pushed the ols-tools branch from fc2c2a2 to 8a81c3c Compare February 24, 2025 13:07

onmete commented Feb 24, 2025

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 24, 2025

onmete force-pushed the ols-tools branch from 637175f to 9099a3b Compare February 25, 2025 07:04

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 25, 2025

onmete force-pushed the ols-tools branch from 9099a3b to 06dae4a Compare February 25, 2025 07:55

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 25, 2025

tisnik reviewed Feb 25, 2025

View reviewed changes

tisnik suggested changes Feb 25, 2025

View reviewed changes

ols/src/query_helpers/docs_summarizer.py Outdated Show resolved Hide resolved

ols/src/query_helpers/docs_summarizer.py Outdated Show resolved Hide resolved

ols/src/query_helpers/docs_summarizer.py Outdated Show resolved Hide resolved

Add oc tools

1d03250

onmete force-pushed the ols-tools branch from 06dae4a to 1d03250 Compare February 25, 2025 12:46

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 25, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OLS-1379: Add oc tools #2216

OLS-1379: Add oc tools #2216

onmete commented Feb 7, 2025 •

edited

Loading

openshift-ci-robot commented Feb 7, 2025 •

edited by openshift-ci bot

Loading

Description

Type of change

openshift-ci bot commented Feb 7, 2025

onmete commented Feb 7, 2025

onmete commented Feb 7, 2025

asamal4 Feb 12, 2025

onmete Feb 18, 2025

asamal4 Feb 12, 2025

onmete Feb 18, 2025

asamal4 Feb 18, 2025

onmete commented Feb 19, 2025

onmete commented Feb 20, 2025

openshift-ci-robot commented Feb 20, 2025 •

edited by openshift-ci bot

Loading

Description

Type of change

asamal4 Feb 24, 2025

onmete Feb 24, 2025

asamal4 Feb 24, 2025

onmete Feb 24, 2025

asamal4 commented Feb 24, 2025

onmete commented Feb 24, 2025

onmete Feb 24, 2025

asamal4 commented Feb 24, 2025 •

edited

Loading

onmete commented Feb 24, 2025

asamal4 commented Feb 25, 2025

asamal4 commented Feb 25, 2025 •

edited

Loading

tisnik Feb 25, 2025

onmete Feb 25, 2025

openshift-ci bot commented Feb 25, 2025

openshift-ci bot commented Feb 25, 2025

		raise e


		def oc_get(args: list[str]) -> subprocess.CompletedProcess:

OLS-1379: Add oc tools #2216

Are you sure you want to change the base?

OLS-1379: Add oc tools #2216

Conversation

onmete commented Feb 7, 2025 • edited Loading

Description

Type of change

openshift-ci-robot commented Feb 7, 2025 • edited by openshift-ci bot Loading

Description

Type of change

openshift-ci bot commented Feb 7, 2025

onmete commented Feb 7, 2025

onmete commented Feb 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onmete commented Feb 19, 2025

onmete commented Feb 20, 2025

openshift-ci-robot commented Feb 20, 2025 • edited by openshift-ci bot Loading

Description

Type of change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asamal4 commented Feb 24, 2025

onmete commented Feb 24, 2025

Choose a reason for hiding this comment

asamal4 commented Feb 24, 2025 • edited Loading

onmete commented Feb 24, 2025

asamal4 commented Feb 25, 2025

asamal4 commented Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 25, 2025

openshift-ci bot commented Feb 25, 2025

onmete commented Feb 7, 2025 •

edited

Loading

openshift-ci-robot commented Feb 7, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Feb 20, 2025 •

edited by openshift-ci bot

Loading

asamal4 commented Feb 24, 2025 •

edited

Loading

asamal4 commented Feb 25, 2025 •

edited

Loading