[AI Explainability] TrustyAI DSC based LMEval configuration#672
[AI Explainability] TrustyAI DSC based LMEval configuration#672sheltoncyril merged 4 commits intoopendatahub-io:mainfrom
Conversation
📝 WalkthroughWalkthroughReplaces a ConfigMap-based fixture with a DataScienceCluster-based fixture to patch lmeval-related settings, updates imports and fixture signatures, and adjusts dependent tests and conftest fixtures to consume the new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/fixtures/trustyai.py (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/verified', '/cherry-pick', '/hold', '/build-push-pr-image', '/wip', '/lgtm'} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/fixtures/trustyai.py(2 hunks)tests/llama_stack/eval/test_lmeval_provider.py(1 hunks)tests/model_explainability/lm_eval/conftest.py(4 hunks)utilities/trustyai_utils.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-05T10:05:17.642Z
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Applied to files:
tests/fixtures/trustyai.py
🧬 Code graph analysis (4)
tests/llama_stack/eval/test_lmeval_provider.py (1)
tests/fixtures/trustyai.py (1)
patched_dsc_lmeval_allow_all(26-36)
utilities/trustyai_utils.py (1)
tests/fixtures/trustyai.py (1)
trustyai_operator_deployment(16-22)
tests/model_explainability/lm_eval/conftest.py (1)
tests/fixtures/trustyai.py (1)
patched_dsc_lmeval_allow_all(26-36)
tests/fixtures/trustyai.py (3)
utilities/infra.py (1)
get_data_science_cluster(865-866)utilities/trustyai_utils.py (1)
patch_dsc_trustyai_lmeval_config(8-48)tests/conftest.py (1)
admin_client(68-69)
🪛 Ruff (0.13.2)
tests/llama_stack/eval/test_lmeval_provider.py
55-55: Unused method argument: minio_pod
(ARG002)
55-55: Unused method argument: minio_data_connection
(ARG002)
55-55: Unused method argument: patched_dsc_lmeval_allow_all
(ARG002)
tests/model_explainability/lm_eval/conftest.py
30-30: Unused function argument: patched_dsc_lmeval_allow_all
(ARG001)
77-77: Unused function argument: patched_dsc_lmeval_allow_all
(ARG001)
107-107: Unused function argument: patched_dsc_lmeval_allow_all
(ARG001)
🔇 Additional comments (9)
tests/llama_stack/eval/test_lmeval_provider.py (1)
54-56: LGTM! Fixture parameter update aligns with DSC-based configuration.The change from
patched_trustyai_configmap_allow_onlinetopatched_dsc_lmeval_allow_allcorrectly reflects the refactor to DataScienceCluster-based configuration. The static analysis warnings about unused arguments are false positives—pytest fixtures are used for setup/teardown side effects, not necessarily for direct reference in the test body.utilities/trustyai_utils.py (4)
1-6: LGTM! Imports are appropriate.The imports correctly bring in the necessary types and resources for patching the DataScienceCluster and managing the operator deployment.
8-13: LGTM! Well-designed function signature.The signature uses safe defaults (deny by default) and clear typing. The Generator return type is appropriate for use in pytest fixtures with
yield from.
26-43: LGTM! Patch structure is correct.The ResourceEditor context manager correctly structures the patch for the DataScienceCluster's trustyai.eval.lmeval settings, and will automatically handle cleanup if an exception occurs.
44-48: Handle zero-initial replicas to guarantee restart
In utilities/trustyai_utils.py (lines 44–48),num_replicasmay be 0, making the scale 0→0 a no-op. Add before scaling back up:if num_replicas == 0: num_replicas = 1Also confirm that
wait_for_replicas()enforces a finite timeout and raises on failure.tests/model_explainability/lm_eval/conftest.py (1)
25-32: LGTM! Fixture parameter updates are consistent and correct.All three fixtures (
lmevaljob_hf,lmevaljob_local_offline,lmevaljob_vllm_emulator) have been consistently updated to usepatched_dsc_lmeval_allow_all: DataScienceCluster, aligning with the refactor from ConfigMap-based to DataScienceCluster-based configuration.The static analysis warnings about unused arguments are false positives—pytest fixtures ensure the DSC is properly patched before the LMEvalJob runs, even if the fixture object isn't directly referenced in the function body.
Also applies to: 72-79, 103-111
tests/fixtures/trustyai.py (3)
1-12: LGTM! Imports correctly support the DSC-based fixture.The new imports for
DataScienceCluster,Generator, and the utility functions (get_data_science_cluster,patch_dsc_trustyai_lmeval_config) properly support the refactored fixture.
15-22: LGTM! Deployment fixture with class scope is appropriate.The
trustyai_operator_deploymentfixture correctly uses class scope, which is appropriate since the deployment configuration is shared across tests in a class and doesn't need per-test isolation.
25-36: Class-scopedpatched_dsc_lmeval_allow_allis safe. Tests using it only read from the shared cluster and don’t mutate its state.
Co-authored-by: Adolfo Aguirrezabal <aaguirre@redhat.com>
rhods-ci-bot
added
commented-by-adolfo-ab
commented-by-sheltoncyril
and removed
commented-by-adolfo-ab
labels
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
utilities/trustyai_utils.py (1)
44-47: Consider adding a wait between scale operations.The deployment scaling logic scales to 0 and immediately scales back without waiting for the pods to terminate. While this might work in practice, it could lead to race conditions where:
- The operator pods haven't fully terminated before the scale-up
- The operator might not properly pick up the DSC changes
Consider adding a wait after scaling down:
num_replicas: int = trustyai_operator_deployment.replicas trustyai_operator_deployment.scale_replicas(replica_count=0) + trustyai_operator_deployment.wait_for_replicas(replica_count=0) trustyai_operator_deployment.scale_replicas(replica_count=num_replicas) trustyai_operator_deployment.wait_for_replicas()This ensures pods fully terminate before scaling back up, giving the operator a clean restart to reconcile the DSC changes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
utilities/trustyai_utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
utilities/trustyai_utils.py (1)
tests/fixtures/trustyai.py (1)
trustyai_operator_deployment(16-22)
🔇 Additional comments (3)
utilities/trustyai_utils.py (3)
1-5: LGTM!All imports are necessary and properly used in the function implementation.
8-13: Function extraction is reasonable despite single usage.Regarding the past comment questioning whether this file/function is needed since it's only used in
patched_dsc_lmeval_allow_all: while it's true this is currently used in one place, extracting this logic into a utility function is a valid design choice that:
- Separates fixture concerns from DSC patching logic
- Improves testability of the patching logic
- Enables easier reuse if additional tests need similar DSC lmeval configuration
The function signature is well-typed and clear.
26-48: Implementation approach is sound for a test fixture.The use of
ResourceEditoras a context manager ensures the DSC patch is automatically reverted after the test completes, which is the correct pattern for a test fixture. The deployment scaling inside the context ensures the operator reconciles the DSC changes while the patch is active.The patch structure correctly targets
spec.components.trustyai.eval.lmevalwith the appropriatepermitCodeExecutionandpermitOnlinesettings.
|
Status of building tag latest: success. |
|
/cherry-pick 2.25 |
* refactor: replace TrustyAI ConfigMap patching with DataScienceCluster config * Apply suggestion from @adolfo-ab Co-authored-by: Adolfo Aguirrezabal <aaguirre@redhat.com> * refactor: remove utils file and fn * refactor: remove unused file and code --------- Co-authored-by: Adolfo Aguirrezabal <aaguirre@redhat.com>
|
Cherry pick action created PR #673 successfully 🎉! |
) * refactor: replace TrustyAI ConfigMap patching with DataScienceCluster config * Apply suggestion from @adolfo-ab * refactor: remove utils file and fn * refactor: remove unused file and code --------- Co-authored-by: Shelton Cyril <sheltoncyril@gmail.com> Co-authored-by: Adolfo Aguirrezabal <aaguirre@redhat.com>
…hub-io#672) * refactor: replace TrustyAI ConfigMap patching with DataScienceCluster config * Apply suggestion from @adolfo-ab Co-authored-by: Adolfo Aguirrezabal <aaguirre@redhat.com> * refactor: remove utils file and fn * refactor: remove unused file and code --------- Co-authored-by: Adolfo Aguirrezabal <aaguirre@redhat.com>
5 participants
This PR adds tests for the newly introduced DSC config feature where the LMEval flags allowCodeExecution and allowOnline are configurable from the DSC.
Summary by CodeRabbit
Tests
Refactor
Notes