Add Tests for Exp-Type, Model and Term settings support in Bulk API and Cluster support in datasources#2013
Open
khansaad wants to merge 2 commits into
Open
Add Tests for Exp-Type, Model and Term settings support in Bulk API and Cluster support in datasources#2013khansaad wants to merge 2 commits into
khansaad wants to merge 2 commits into
Conversation
Contributor
Reviewer's GuideAdds cluster-aware datasource support and new bulk API controls (cluster_name, model_settings, term_settings, experiment_types) with validation, plus namespace-level experiments, updated recommendation notifications, and comprehensive tests and docs. Sequence diagram for bulk API experiment types and cluster overridesequenceDiagram
actor User
participant BulkAPI as BulkInput
participant Validator as BulkServiceValidation
participant Manager as BulkJobManager
User->>BulkAPI: POST bulk payload
BulkAPI->>Validator: validate(payload, jobID)
Validator-->>BulkAPI: ValidationOutputData
alt [validation passed]
BulkAPI->>Manager: getExperimentMap(labelString, jobData, metadataInfo, datasource)
Manager->>Manager: resolveExperimentType(experiment_types)
loop datasources/clusters/namespaces
alt [resolvedType == NAMESPACE]
Manager->>Manager: frameNamespaceExperimentName(labelString, dsc, namespace)
Manager->>Manager: prepareNamespaceExperimentJSONInput(dsc, namespace, experiment_name, list)
else [resolvedType == CONTAINER]
Manager->>Manager: frameExperimentName(labelString, dsc, namespace, dsw, dc)
Manager->>Manager: prepareCreateExperimentJSONInput(dc, dsc, dsw, namespace, experiment_name, list)
end
end
end
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 - I've found 4 issues, and left some high level feedback:
- The error messages in BulkServiceValidation (e.g., for cluster_name length and invalid experiment_types) do not match the strings asserted in the new Python bulk API tests; consider aligning the Java message templates with the test expectations to avoid brittle failures.
- DataSourceInfoAdapter serializes the authentication field as "authenticationConfig"; if existing consumers expect the previous JSON shape, you may want to keep the original key name or add a compatibility path to avoid changing the ListDatasources API contract.
- BulkJobManagerPassthroughTest mostly asserts mocked BulkInput getters and does not drive BulkJobManager methods that use cluster_name/model_settings/term_settings; consider extending it to call prepareCreateExperimentJSONInput/namespace experiment creation to verify real passthrough behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The error messages in BulkServiceValidation (e.g., for cluster_name length and invalid experiment_types) do not match the strings asserted in the new Python bulk API tests; consider aligning the Java message templates with the test expectations to avoid brittle failures.
- DataSourceInfoAdapter serializes the authentication field as "authenticationConfig"; if existing consumers expect the previous JSON shape, you may want to keep the original key name or add a compatibility path to avoid changing the ListDatasources API contract.
- BulkJobManagerPassthroughTest mostly asserts mocked BulkInput getters and does not drive BulkJobManager methods that use cluster_name/model_settings/term_settings; consider extending it to call prepareCreateExperimentJSONInput/namespace experiment creation to verify real passthrough behavior.
## Individual Comments
### Comment 1
<location path="tests/scripts/local_monitoring_tests/rest_apis/test_bulkAPI.py" line_range="424-432" />
<code_context>
+@pytest.mark.parametrize("model_settings, expected_status, expected_error", [
</code_context>
<issue_to_address>
**suggestion (testing):** Extend model_settings tests to cover null/blank model names and detailed error paths
`validateModelSettings` has additional branches that aren’t yet covered by this parametrization:
- `models` containing `null` entries (should return `"model_settings.models contains null or empty model name"`).
- `models` containing blank/whitespace-only names.
- Invalid models where the error message includes both the invalid value and the list of valid models.
Please add cases like `{"models": [None]}`, `{"models": [" "]}`, and an invalid model asserting on the full error message, so the tests fully exercise these paths and guard against regressions in error formatting.
Suggested implementation:
```python
@pytest.mark.test_bulk_api_ros
@pytest.mark.parametrize("model_settings, expected_status, expected_error", [
({"models": ["performance"]}, SUCCESS_200_STATUS_CODE, None), # Valid single model
({"models": ["cost"]}, SUCCESS_200_STATUS_CODE, None), # Valid cost model
({"models": ["performance", "cost"]}, SUCCESS_200_STATUS_CODE, None), # Valid multiple models
({"models": ["Performance", "COST"]}, SUCCESS_200_STATUS_CODE, None), # Valid case-insensitive
({"models": []}, ERROR_STATUS_CODE, "model_settings.models cannot be null or empty"), # Empty list
({"models": [None]}, ERROR_STATUS_CODE, "model_settings.models contains null or empty model name"), # Null model name
({"models": [" "]}, ERROR_STATUS_CODE, "model_settings.models contains null or empty model name"), # Blank/whitespace model name
({"models": ["invalid"]}, ERROR_STATUS_CODE, "Invalid model name 'invalid'. Valid models are: ['performance', 'cost']"), # Invalid model with full error
({"models": ["performance", "invalid"]}, ERROR_STATUS_CODE, "Invalid model name"), # Mixed valid/invalid
({}, ERROR_STATUS_CODE, "model_settings.models cannot be null or empty"), # Missing models field
])
def test_bulk_api_model_settings_validation(cluster_type, model_settings, expected_status, expected_error, caplog):
```
If the actual implementation of `validateModelSettings` uses a slightly different error message format (for example different punctuation, ordering, or representation of the valid models list), you should adjust the two new `expected_error` strings to exactly match the real messages:
1. The "contains null or empty model name" string for `{"models": [None]}` and `{"models": [" "]}`.
2. The full invalid-model error for `{"models": ["invalid"]}`, including the precise formatting of the valid-models list.
</issue_to_address>
### Comment 2
<location path="tests/scripts/local_monitoring_tests/rest_apis/test_bulkAPI.py" line_range="464-473" />
<code_context>
+@pytest.mark.parametrize("term_settings, expected_status, expected_error", [
</code_context>
<issue_to_address>
**suggestion (testing):** Add term_settings cases for null/blank term entries and ensure error messages are asserted
`validateTermSettings` also has a branch for null/blank term entries, which isn’t covered by the current parametrization. Please add cases for:
- `{"terms": [None]}`
- `{"terms": [" "]}`
expecting the "term_settings.terms contains null or empty term name" message.
Additionally, for at least one invalid-term case, assert on the full error message (including the valid-term list) to lock in the error contract.
Suggested implementation:
```python
message = response.json()["message"]
if expected_full_error:
assert message == expected_full_error, \
f"Expected error message to be '{expected_full_error}' but got: {message}"
elif expected_error:
assert expected_error in message, \
f"Expected error message to contain '{expected_error}' but got: {message}"
else:
# Valid model settings should create a job
assert "job_id" in response.json(), "Expected job_id in response for valid model_settings"
@pytest.mark.test_bulk_api_ros
@pytest.mark.parametrize("term_settings, expected_status, expected_error, expected_full_error", [
({"terms": ["short"]}, SUCCESS_200_STATUS_CODE, None, None), # Valid single term
({"terms": ["medium"]}, SUCCESS_200_STATUS_CODE, None, None), # Valid medium term
({"terms": ["long"]}, SUCCESS_200_STATUS_CODE, None, None), # Valid long term
({"terms": ["short", "long"]}, SUCCESS_200_STATUS_CODE, None, None), # Valid multiple terms
({"terms": ["Short", "LONG"]}, SUCCESS_200_STATUS_CODE, None, None), # Valid case-insensitive
({"terms": ["short", "medium", "long"]}, SUCCESS_200_STATUS_CODE, None, None), # All terms
({"terms": []}, ERROR_STATUS_CODE, "term_settings.terms cannot be null or empty", None), # Empty list
({"terms": [None]}, ERROR_STATUS_CODE, "term_settings.terms contains null or empty term name", None), # Null term entry
({"terms": [" "]}, ERROR_STATUS_CODE, "term_settings.terms contains null or empty term name", None), # Blank term entry
({"terms": ["invalid"]}, ERROR_STATUS_CODE, "Invalid term name", "Invalid term name. Valid terms are: short, medium, long"), # Invalid term with full error
({"terms": ["short", "invalid"]}, ERROR_STATUS_CODE, "Invalid term name", None), # Mixed valid/invalid
({}, ERROR_STATUS_CODE, "term_settings.terms cannot be null or empty", None), # Missing terms field
```
1. Ensure the test function that consumes these parameters is updated to accept the new `expected_full_error` argument in its signature (e.g., `def test_...(..., expected_status, expected_error, expected_full_error, ...)`).
2. If the actual full error message for invalid terms differs from `"Invalid term name. Valid terms are: short, medium, long"`, adjust the `expected_full_error` string in the parametrization to exactly match the implementation's message, including punctuation and spacing.
</issue_to_address>
### Comment 3
<location path="design/KruizeDatasource.md" line_range="388-389" />
<code_context>
+}
+```
+
+#### Multiple Clusters Example
+TBA
+
+### Behavior
</code_context>
<issue_to_address>
**issue:** Complete or clarify the 'Multiple Clusters Example' section instead of leaving 'TBA'.
Since this section is user-facing, please either add a concrete multi-cluster configuration example or clearly note that it will be documented once full multi-cluster support is available. Leaving it as-is makes the document appear unfinished and may confuse readers.
</issue_to_address>
### Comment 4
<location path="design/NotificationCodes.md" line_range="86-92" />
<code_context>
+| 323007 | NOTICE | CPU_REQUESTS_OVER_PROVISIONED | Specifies that the workload is over-provisioned for CPU requests | Workload is over-provisioned for CPU. Kruize recommends reducing CPU allocation to optimize costs. | DATA USER |
</code_context>
<issue_to_address>
**nitpick (typo):** Align spelling of 'optimise/optimize' for consistency across the notification descriptions.
These new rows use 'optimize costs', while existing entries (e.g., OPTIMISED) use 'optimised'. Please standardize on either US or UK spelling across the table for consistency, unless the difference is intentional.
Suggested implementation:
```
| 323007 | NOTICE | CPU_REQUESTS_OVER_PROVISIONED | Specifies that the workload is over-provisioned for CPU requests | Workload is over-provisioned for CPU. Kruize recommends reducing CPU allocation to optimise costs. | DATA USER |
```
```
| 324006 | NOTICE | MEMORY_REQUESTS_OVER_PROVISIONED | Specifies that the workload is over-provisioned for Memory requests | Workload is over-provisioned for Memory. Kruize recommends reducing Memory allocation to optimise costs. | DATA USER |
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Saad Khan <saakhan@ibm.com>
khansaad
force-pushed
the
test-cluster-support
branch
from
July 8, 2026 11:43
996c9b8 to
140f780
Compare
khansaad
changed the title
khansaad
changed the title
Signed-off-by: Saad Khan <saakhan@ibm.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Description
This PR contains tests for the exp-type, model and term settings support in Bulk API along with cluster support in datasources.
Fixes # (issue)
Type of change
How has this been tested?
Please describe the tests that were run to verify your changes and steps to reproduce. Please specify any test configuration required.
Test Configuration
Checklist 🎯
Additional information
Include any additional information such as links, test results, screenshots here
Summary by Sourcery
Extend bulk API and datasource support for clusters and experiment types, and validate and expose these through new tests and documentation.
New Features:
Enhancements:
Documentation:
Tests:
Chores: