feat(RHOAIENG-33702): Update LMEval configuration to support environment variable for Kubernetes usage #58
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… for Kubernetes usage
Contributor
Reviewer's GuideThis PR refactors LMEval’s Kubernetes setup by deferring namespace resolution and client/CR builder initialization until first use, introduces explicit guards in evaluation entry points, relaxes the configuration to allow non-K8s mode, and updates run.yaml to source the use_k8s flag and namespace from environment variables. Sequence diagram for deferred Kubernetes initialization in LMEvalsequenceDiagram
participant LMEval
participant "K8s Client"
participant "CR Builder"
participant "Config"
participant "Logger"
LMEval->>LMEval: Call _ensure_k8s_initialized()
alt use_k8s is False
LMEval->>Logger: Log warning (Non-K8s backend not implemented)
LMEval-->>LMEval: Return
else use_k8s is True
alt _k8s_client is None
LMEval->>LMEval: _init_k8s_client()
end
alt _namespace is None
LMEval->>LMEval: _resolve_namespace(config)
LMEval->>Logger: Log resolved namespace
end
alt _cr_builder is None
LMEval->>"CR Builder": Initialize with namespace and service_account
LMEval->>Logger: Log CR builder initialization
end
end
Class diagram for updated LMEval initialization and configurationclassDiagram
class LMEval {
- _config: LMEvalEvalProviderConfig
- _namespace: str | None
- _k8s_client: k8s_client.ApiClient | None
- _k8s_custom_api: k8s_client.CustomObjectsApi | None
- _cr_builder: LMEvalCRBuilder | None
+ __init__(config: LMEvalEvalProviderConfig)
+ _ensure_k8s_initialized()
+ _init_k8s_client()
+ run_eval(...)
+ evaluate_rows(...)
+ job_status(...)
+ job_cancel(...)
+ job_result(...)
}
class LMEvalCRBuilder {
- namespace: str | None
- service_account: str | None
- _config: LMEvalEvalProviderConfig
+ create_cr(...)
}
LMEval --> LMEvalCRBuilder
LMEval --> k8s_client.ApiClient
LMEval --> k8s_client.CustomObjectsApi
LMEval --> LMEvalEvalProviderConfig
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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_lmeval/config.py:133` </location>
<code_context>
"""Validate the configuration"""
if not isinstance(self.use_k8s, bool):
raise LMEvalConfigError("use_k8s must be a boolean")
- if self.use_k8s is False:
- raise LMEvalConfigError(
- "Only Kubernetes LMEval backend is supported at the moment"
- )
</code_context>
<issue_to_address>
Removing the check for use_k8s disables early error reporting for unsupported backends.
This change may result in less informative error messages, as misconfigured backends will fail later at runtime instead of during configuration validation.
</issue_to_address>
### Comment 2
<location> `src/llama_stack_provider_lmeval/lmeval.py:939` </location>
<code_context>
+
+ def _ensure_k8s_initialized(self):
+ """Ensure Kubernetes client and namespace are initialized when needed."""
+ if not self.use_k8s:
+ logger.warning("Non-K8s evaluation backend is not implemented yet")
+ return
+
</code_context>
<issue_to_address>
Logging a warning for non-K8s backend may be redundant given the subsequent NotImplementedError.
Consider removing the warning in _ensure_k8s_initialized, as NotImplementedError is raised elsewhere if use_k8s is False. Alternatively, update the warning to clarify where the error will occur.
Suggested implementation:
```python
def _ensure_k8s_initialized(self):
"""Ensure Kubernetes client and namespace are initialized when needed."""
if not self.use_k8s:
return
```
If you want to clarify in the log that the error will be raised elsewhere, you could replace the warning with:
```python
logger.warning("use_k8s is False; NotImplementedError will be raised in subsequent calls.")
```
But the cleanest approach is simply to remove the warning as shown above.
</issue_to_address>
### Comment 3
<location> `src/llama_stack_provider_lmeval/lmeval.py:950` </location>
<code_context>
+ self._namespace = _resolve_namespace(self._config)
+ logger.debug("LMEval provider resolved namespace: %s", self._namespace)
+
+ if self._cr_builder is None:
self._cr_builder = LMEvalCRBuilder(
namespace=self._namespace,
</code_context>
<issue_to_address>
Raising LMEvalConfigError for missing CR builder may mask underlying initialization issues.
Consider providing a more specific error or additional diagnostics to help users identify whether the issue is with K8s client initialization or namespace resolution.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if self._cr_builder is None:
self._cr_builder = LMEvalCRBuilder(
namespace=self._namespace,
service_account=getattr(self._config, "service_account", None),
)
self._cr_builder._config = self._config
logger.debug("Initialized Kubernetes client and CR builder with namespace: %s", self._namespace)
=======
if self._cr_builder is None:
self._cr_builder = LMEvalCRBuilder(
namespace=self._namespace,
service_account=getattr(self._config, "service_account", None),
)
self._cr_builder._config = self._config
logger.debug("Initialized Kubernetes client and CR builder with namespace: %s", self._namespace)
# Additional diagnostics for CR builder initialization
if self._cr_builder is None:
diagnostics = (
f"K8s client initialized: {self._k8s_client is not None}\n"
f"Namespace resolved: {self._namespace}\n"
f"Config: {self._config}\n"
)
raise LMEvalConfigError(
"Failed to initialize LMEvalCRBuilder. "
"This may be due to issues with Kubernetes client initialization, "
"namespace resolution, or CR builder construction.\n"
f"Diagnostics:\n{diagnostics}"
)
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `run.yaml:19` </location>
<code_context>
provider_type: remote::trustyai_lmeval
config:
- use_k8s: True
+ use_k8s: ${env.TRUSTYAI_LMEVAL_USE_K8S:True}
base_url: ${env.VLLM_URL:http://localhost:8000/v1}
namespace: ${env.TRUSTYAI_LM_EVAL_NAMESPACE}
</code_context>
<issue_to_address>
Switching use_k8s to an environment variable increases configurability but may introduce type ambiguity.
Ensure that the environment variable is parsed to a boolean to prevent type-related issues.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
-133
to
-134
| if self.use_k8s is False: | ||
| raise LMEvalConfigError( |
Contributor
There was a problem hiding this comment.
question: Removing the check for use_k8s disables early error reporting for unsupported backends.
This change may result in less informative error messages, as misconfigured backends will fail later at runtime instead of during configuration validation.
Comment on lines
+939
to
+940
| if not self.use_k8s: | ||
| logger.warning("Non-K8s evaluation backend is not implemented yet") |
Contributor
There was a problem hiding this comment.
suggestion: Logging a warning for non-K8s backend may be redundant given the subsequent NotImplementedError.
Consider removing the warning in _ensure_k8s_initialized, as NotImplementedError is raised elsewhere if use_k8s is False. Alternatively, update the warning to clarify where the error will occur.
Suggested implementation:
def _ensure_k8s_initialized(self):
"""Ensure Kubernetes client and namespace are initialized when needed."""
if not self.use_k8s:
returnIf you want to clarify in the log that the error will be raised elsewhere, you could replace the warning with:
logger.warning("use_k8s is False; NotImplementedError will be raised in subsequent calls.")But the cleanest approach is simply to remove the warning as shown above.
Comment on lines
950
to
956
| if self._cr_builder is None: | ||
| self._cr_builder = LMEvalCRBuilder( | ||
| namespace=self._namespace, | ||
| service_account=getattr(self._config, "service_account", None), | ||
| ) | ||
| self._cr_builder._config = self._config | ||
| logger.debug("Initialized Kubernetes client and CR builder with namespace: %s", self._namespace) |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Raising LMEvalConfigError for missing CR builder may mask underlying initialization issues.
Consider providing a more specific error or additional diagnostics to help users identify whether the issue is with K8s client initialization or namespace resolution.
Suggested change
| if self._cr_builder is None: | |
| self._cr_builder = LMEvalCRBuilder( | |
| namespace=self._namespace, | |
| service_account=getattr(self._config, "service_account", None), | |
| ) | |
| self._cr_builder._config = self._config | |
| logger.debug("Initialized Kubernetes client and CR builder with namespace: %s", self._namespace) | |
| if self._cr_builder is None: | |
| self._cr_builder = LMEvalCRBuilder( | |
| namespace=self._namespace, | |
| service_account=getattr(self._config, "service_account", None), | |
| ) | |
| self._cr_builder._config = self._config | |
| logger.debug("Initialized Kubernetes client and CR builder with namespace: %s", self._namespace) | |
| # Additional diagnostics for CR builder initialization | |
| if self._cr_builder is None: | |
| diagnostics = ( | |
| f"K8s client initialized: {self._k8s_client is not None}\n" | |
| f"Namespace resolved: {self._namespace}\n" | |
| f"Config: {self._config}\n" | |
| ) | |
| raise LMEvalConfigError( | |
| "Failed to initialize LMEvalCRBuilder. " | |
| "This may be due to issues with Kubernetes client initialization, " | |
| "namespace resolution, or CR builder construction.\n" | |
| f"Diagnostics:\n{diagnostics}" | |
| ) |
run.yaml
Outdated
| provider_type: remote::trustyai_lmeval | ||
| config: | ||
| use_k8s: True | ||
| use_k8s: ${env.TRUSTYAI_LMEVAL_USE_K8S:True} |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Switching use_k8s to an environment variable increases configurability but may introduce type ambiguity.
Ensure that the environment variable is parsed to a boolean to prevent type-related issues.
Contributor
nathan-weinberg
left a comment
nathan-weinberg
left a comment
There was a problem hiding this comment.
One comment but generally LGTM!
leseb
reviewed
Sep 9, 2025
Co-authored-by: Sébastien Han <[email protected]>
Co-authored-by: Sébastien Han <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refer to RHOAIENG-33702.
Summary by Sourcery
Add environment variable support for Kubernetes usage and refactor LMEval provider initialization for lazy Kubernetes client and custom resource builder setup
New Features:
Enhancements: