-
Notifications
You must be signed in to change notification settings - Fork 11
feat: ability to pass in KFP token via env var. #35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
a5268dc
c9f3f6c
ab73187
e70b5de
a07cc9b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,38 +1,32 @@ | ||
| import logging | ||
| import os | ||
| from typing import List # noqa | ||
|
|
||
| from dotenv import load_dotenv | ||
| from kfp import dsl | ||
| from kubernetes import client | ||
| from kubernetes.client.exceptions import ApiException | ||
|
|
||
| from ...constants import ( | ||
| DEFAULT_RAGAS_PROVIDER_IMAGE, | ||
| KUBEFLOW_CANDIDATE_NAMESPACES, | ||
| RAGAS_PROVIDER_IMAGE_CONFIGMAP_KEY, | ||
| RAGAS_PROVIDER_IMAGE_CONFIGMAP_NAME, | ||
| ) | ||
| from .utils import _load_kube_config | ||
|
|
||
| load_dotenv() | ||
| logger = logging.getLogger(__name__) | ||
| logger.setLevel(logging.INFO) | ||
|
|
||
|
|
||
| def get_base_image() -> str: | ||
| """Get base image from env, fallback to k8s ConfigMap, fallback to default image.""" | ||
|
|
||
| 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, | ||
| ) | ||
|
Comment on lines
-12
to
-23
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
|
||
| if (base_image := os.environ.get("KUBEFLOW_BASE_IMAGE")) is not None: | ||
| return base_image | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
| logger.setLevel(logging.INFO) | ||
|
|
||
| try: | ||
| config.load_incluster_config() | ||
| except config.ConfigException: | ||
| config.load_kube_config() | ||
|
|
||
| _load_kube_config() | ||
| api = client.CoreV1Api() | ||
|
|
||
| for candidate_namespace in KUBEFLOW_CANDIDATE_NAMESPACES: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| import logging | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def _load_kube_config(): | ||
| from kubernetes import config | ||
| from kubernetes.client.configuration import Configuration | ||
|
|
||
| kube_config = Configuration() | ||
|
|
||
| try: | ||
| config.load_incluster_config(client_configuration=kube_config) | ||
| logger.info("Loaded in-cluster Kubernetes configuration") | ||
|
Comment on lines
+12
to
+14
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| except config.ConfigException: | ||
| config.load_kube_config(client_configuration=kube_config) | ||
| logger.info("Loaded Kubernetes configuration from kubeconfig file") | ||
|
|
||
| return kube_config | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,7 +71,7 @@ def kfp_client(self): | |
| try: | ||
| import kfp | ||
|
|
||
| token = self._get_token() | ||
| token = self._get_kfp_token() | ||
| if not token: | ||
| raise RagasEvaluationError( | ||
| "No token found. Please run `oc login` and try again." | ||
|
|
@@ -108,14 +108,16 @@ def kfp_client(self): | |
|
|
||
| return self._kfp_client | ||
|
|
||
| def _get_token(self) -> str: | ||
| def _get_kfp_token(self) -> str: | ||
| if self.config.kubeflow_config.pipelines_token: | ||
| logger.info("Using KUBEFLOW_PIPELINES_TOKEN from config") | ||
| return self.config.kubeflow_config.pipelines_token | ||
|
|
||
| try: | ||
| from kubernetes.client.configuration import Configuration | ||
| from kubernetes.config.kube_config import load_kube_config | ||
| from .kubeflow.utils import _load_kube_config | ||
|
|
||
| config = Configuration() | ||
| 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]) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| except ImportError as e: | ||
| raise RagasEvaluationError( | ||
| "Kubernetes client is not installed. Install with: pip install .[remote]" | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.