Skip to content

feat: ability to pass in KFP token via env var.#35

Merged
dmaniloff merged 5 commits intotrustyai-explainability:mainfrom
dmaniloff:read-kube-token-as-env
Oct 16, 2025
Merged

feat: ability to pass in KFP token via env var.#35
dmaniloff merged 5 commits intotrustyai-explainability:mainfrom
dmaniloff:read-kube-token-as-env

Conversation

@dmaniloff
Copy link
Copy Markdown
Collaborator

@dmaniloff dmaniloff commented Oct 16, 2025

Add KUBEFLOW_PIPELINES_TOKEN env var to the config so users can pass that in manually.

Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there - I've reviewed your changes and they look great!

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location> `src/llama_stack_provider_ragas/remote/ragas_remote_eval.py:120` </location>
<code_context>
-            load_kube_config(client_configuration=config)
-            token = str(config.api_key["authorization"].split(" ")[-1])
+            kube_config = _load_kube_config()
+            token = str(kube_config.api_key["authorization"].split(" ")[-1])
         except ImportError as e:
             raise RagasEvaluationError(
</code_context>

<issue_to_address>
**issue (bug_risk):** Splitting the authorization header may be brittle if the format changes.

The code relies on a fixed 'Bearer <token>' format, which may not always be present. Please add validation or error handling to manage unexpected or missing formats.
</issue_to_address>

### Comment 2
<location> `src/llama_stack_provider_ragas/config.py:115-112` </location>
<code_context>
+        default=None,
+    )
+
+    kube_token: str | None = Field(
+        description=(
+            "Kubernetes token for Kubeflow pipeline execution. ",
+            "If not provided via env var, the token will be read from the local kubeconfig file.",
         ),
         default=None,
     )
</code_context>

<issue_to_address>
**nitpick:** The description field is a tuple, which may not render as intended.

Pydantic expects the description to be a single string. Please join the tuple elements into one string to ensure proper rendering.
</issue_to_address>

### Comment 3
<location> `src/llama_stack_provider_ragas/remote/kubeflow/utils.py:12-14` </location>
<code_context>
-    logger = logging.getLogger(__name__)
-    logger.setLevel(logging.INFO)
-
-    try:
-        config.load_incluster_config()
-    except config.ConfigException:
</code_context>

<issue_to_address>
**suggestion (bug_risk):** Catching ConfigException without handling other exceptions may miss other errors.

Other exceptions, such as file not found or permission errors, may also occur when loading kubeconfig. Consider handling these cases to improve robustness.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

load_kube_config(client_configuration=config)
token = str(config.api_key["authorization"].split(" ")[-1])
kube_config = _load_kube_config()
token = str(kube_config.api_key["authorization"].split(" ")[-1])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Splitting the authorization header may be brittle if the format changes.

The code relies on a fixed 'Bearer ' format, which may not always be present. Please add validation or error handling to manage unexpected or missing formats.

Comment on lines +12 to +14
try:
config.load_incluster_config(client_configuration=kube_config)
logger.info("Loaded in-cluster Kubernetes configuration")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (bug_risk): Catching ConfigException without handling other exceptions may miss other errors.

Other exceptions, such as file not found or permission errors, may also occur when loading kubeconfig. Consider handling these cases to improve robustness.

@dmaniloff dmaniloff changed the title feat: add kube_token support for Kubeflow configuration and refactor token retrieval in RagasEvaluatorRemote. feat: ability to pass in KFP token via env var. Oct 16, 2025
@trustyai-explainability trustyai-explainability deleted a comment from sourcery-ai bot Oct 16, 2025
Comment on lines -12 to -23
import logging
import os

from kubernetes import client, config
from kubernetes.client.exceptions import ApiException

from llama_stack_provider_ragas.constants import (
DEFAULT_RAGAS_PROVIDER_IMAGE,
KUBEFLOW_CANDIDATE_NAMESPACES,
RAGAS_PROVIDER_IMAGE_CONFIGMAP_KEY,
RAGAS_PROVIDER_IMAGE_CONFIGMAP_NAME,
)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these imports were unnecessarily inside the scope of the function making it look like a kfp component but it is actually not.

Copy link
Copy Markdown

@adolfo-ab adolfo-ab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm as a temporary solution, but I think long-term we need RBAC (not sure if through the TrustyAI operator or through LLS, though)

@dmaniloff dmaniloff requested a review from adolfo-ab October 16, 2025 13:28
@dmaniloff dmaniloff merged commit 94dcc4a into trustyai-explainability:main Oct 16, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants