Conversation
alpkom
left a comment
alpkom
left a comment
There was a problem hiding this comment.
Could you adjust the RPT mock responses in the tests/mock.py to reflect the API changes?
And maybe adjust the integration_tests as well, in order to make use of the new attributes in the API?
Current tests are failing partly because of the optional vs. required behavior of |
It's not only about fixing tests. We need to adjust the mock to reflect the backend behavior properly. |
|
In the current test environment, there is no such RPT-1.6 deployment, and we need to deploy it for the tests to work |
|
|
||
| name: str | ||
| prediction_placeholder: str = "[PREDICT]" | ||
| prediction_placeholder: Optional[Union[str, int, float]] |
There was a problem hiding this comment.
This is a mandatory field, not optional.
There was a problem hiding this comment.
"TargetColumnConfig": {
...,
"properties": {
..., "prediction_placeholder": {
"anyOf": [
{
"type": "string"
},
{
"type": "number"
},
{
"type": "null"
}
],
"description": "The prediction placeholder in any column for which to predict a value. The model will predict a value for all table cells containing this value.",
"title": "Prediction Placeholder"
}, ..., "required": [
"name",
"prediction_placeholder"
], ...According to this spec, yes, it is required. But still, it needs to be a nullable number | string union. Is it wrong to set optional here?
| import unittest | ||
|
|
||
| from integration_tests.constants import SAP_RPT_1_SMALL_TEST_MODEL | ||
| from integration_tests.constants import SAP_RPT_1_6_SMALL_TEST_MODEL |
There was a problem hiding this comment.
Let's not only test against 1.6.
The full set of tests should run against 1.6, but we should have 1 happy path test for both 1.0 and 1.5.
| yield | ||
|
|
||
| @contextmanager | ||
| def sap_rpt_moke_response_code_0_with_explanations(url: str): |
There was a problem hiding this comment.
type at "moke". It was probably a typo in the name of the already existing function.
| self.assertIsNone(response.explanations) | ||
|
|
||
| def test_response_with_explanations(self): | ||
| from tests.mock import RPT_RESPONSE_CODE_0_WITH_EXPLANATIONS |
| self.assertEqual(response.metadata.context_mode, "default") | ||
|
|
||
| def test_response_explanations_none_by_default(self): | ||
| from tests.mock import RPT_RESPONSE_CODE_0 |
| self.assertIsNone(result.top_relevant_context_rows) | ||
|
|
||
| def test_response_metadata_includes_context_mode(self): | ||
| from tests.mock import RPT_RESPONSE_CODE_0 |
| with sap_rpt_moke_response_code_2(url_mock.return_value): | ||
| with self.assertRaises(RPTException) as err: | ||
| self.client.predict(body=request_by_row_dict, model_name="sap-rpt-1-small") | ||
| self.client.predict(body=request_by_row_dict, model_name="sap-rpt-1.6-small") |
There was a problem hiding this comment.
Changing the model name here doesn't matter. What matters is the mock function used with the "with" clause.
| with patch.object(RPTClient, "_get_url", return_value=mock_url) as url_mock: | ||
| with sap_rpt_moke_response_code_0(url_mock.return_value): | ||
| response = await self.client.apredict(body=request_by_row_dict, model_name="sap-rpt-1-small") | ||
| response = await self.client.apredict(body=request_by_row_dict, model_name="sap-rpt-1.6-small") |
There was a problem hiding this comment.
Changing the model name here doesn't matter. What matters is the mock function used with the "with" clause.
This branch has not been deployed
2 participants
Context
Closes SAP/ai-sdk-python-backlog#17.
What this PR does and why it is needed
This PR aims to upgrade the RPT Client logic on the SDK side for the newest version 1.6, grouped in 6 significant sections:
components.schemas.SchemaFieldConfigfrom spec: We add the missing data types to the dtype field in the currentDataTypecomponents.schemas.PredictionResultfrom spec:confidence_intervalis added into Predictionitem (current implementation). And also, we addminimum/maximumbounds toconfidenceas in 1.6.components.schemas.PredictionConfigfrom spec:explanationsare added to PredictionConfig response, so that it is not silently discarded.context_modeis added to PredictionConfig and ResponseMetadata (1.6 specific issue).components.schemas.TargetColumnConfigfrom spec:top_kis added to TargetColumn. Second of all,prediction_placeholderis casted to be nullable: as of now >=1.5 allowsnullornumericas placeholders, while current model enforcesstr. Corresponding linkcomponents.schemas.{ExplanationConfig / ExplanationResult}models didn't exist before, so they are implemented from scratch.Definition of Done