feat: rpt client updated to 1.6

[yamaceay]

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:

1. components.schemas.SchemaFieldConfig from spec: We add the missing data types to the dtype field in the current DataType
2. components.schemas.PredictionResult from spec: confidence_interval is added into Predictionitem (current implementation). And also, we add minimum / maximum bounds to confidence as in 1.6.
3. components.schemas.PredictionConfig from spec: explanations are added to PredictionConfig response, so that it is not silently discarded.
4. context_mode is added to PredictionConfig and ResponseMetadata (1.6 specific issue).
5. components.schemas.TargetColumnConfig from spec: top_k is added to TargetColumn. Second of all, prediction_placeholder is casted to be nullable: as of now >=1.5 allows null or numeric as placeholders, while current model enforces str. Corresponding link
6. components.schemas.{ExplanationConfig / ExplanationResult} models didn't exist before, so they are implemented from scratch.

Definition of Done

• Code is tested (Unit, Integration, E2E)
• Error handling created / updated & covered by the tests above
• Documentation updated
  • Only Public APIs are allowed to be used in documentation/tutorials/sample code
• (Optional) Aligned changes with the JS/TS and Java SDK
• (Optional) Release notes updated -->

[alpkom]

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?

[yamaceay]

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 prediction_payload, fixing it now

[alpkom]

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 prediction_payload, fixing it now

It's not only about fixing tests. We need to adjust the mock to reflect the backend behavior properly.
And we need to adjust/extend the integration tests to test the new features/attributes in the API, as well.

[yamaceay]

In the current test environment, there is no such RPT-1.6 deployment, and we need to deploy it for the tests to work

[yamaceay]

@alpkom Here is my thought process: How do we happy-test if there is only one client (which is 1.6)?

For example in case of PredictionConfig, 1.0 models only have target_columns, 1.5 introduces explanations and 1.6 introduces further context_mode. If we for example allow context_mode to have default values that makes the 1.0 and 1.5 happy, we detach from 1.6 spec. If we solely stick on context_mode being a required parameter, then 1.0 and 1.5 calls will not work.

Since we only have decided to keep only one RPT client (which is fully 1.6 compliant) and want to also preserve 1.0 and 1.5 functionalities, the only way out is to automatically detect what version the current RPT deployment belongs to. Names can be customized, so I don't think pattern-matching on deployment names is a maintainable solution. Does that match to your comments?

[mwien]

looks good overall, just a few small additional comments

[alpkom]

@alpkom Here is my thought process: How do we happy-test if there is only one client (which is 1.6)?

For example in case of PredictionConfig, 1.0 models only have target_columns, 1.5 introduces explanations and 1.6 introduces further context_mode. If we for example allow context_mode to have default values that makes the 1.0 and 1.5 happy, we detach from 1.6 spec. If we solely stick on context_mode being a required parameter, then 1.0 and 1.5 calls will not work.

Since we only have decided to keep only one RPT client (which is fully 1.6 compliant) and want to also preserve 1.0 and 1.5 functionalities, the only way out is to automatically detect what version the current RPT deployment belongs to. Names can be customized, so I don't think pattern-matching on deployment names is a maintainable solution. Does that match to your comments?

We need to be backward compatible. So, for the newly added optional parameters, don't set the default value at all. Leave them optional, without a default value, so if the parameter is not set, they'll be dropped when the HTTP request is formed. No harm done, since the backend will anyways set the default value. Only reason we set them in SDK was convenience.
When we handle it this way, we won't need to discover the model version at all.

[yamaceay]

@alpkom I think sap-rpt-1.5 needs to be deployed for the tests to pass

[alpkom]

In the tests/proxy/native/test_sap_rpt.py, we only have unit tests for the new model response. We need to define mock server tests for that, as well. Take a look at the sap_rpt_mock_response_code_0() function in mock.py, and create a similar one for the new model response, and use that one to introduce new mock server tests in the test_sap_rpt.py.

[alpkom]

@alpkom I think sap-rpt-1.5 needs to be deployed for the tests to pass

I've commented in-line about how to fix it.

[yamaceay]

Perfect, thank you for your comments @alpkom Actually the mock responses now are fully 1.6, I removed all other happy path mock responses for 1.0 and 1.5, and renamed the mock response of 1.6 to sap_rpt_mock_response_code_0 to keep it overall more consistent with the rest of the code. If I didn't misunderstood, you thought this function is still 1.0 rather than 1.6. Let me know if that is not the case

[alpkom]

Perfect, thank you for your comments @alpkom Actually the mock responses now are fully 1.6, I removed all other happy path mock responses for 1.0 and 1.5, and renamed the mock response of 1.6 to sap_rpt_mock_response_code_0 to keep it overall more consistent with the rest of the code. If I didn't misunderstood, you thought this function is still 1.0 rather than 1.6. Let me know if that is not the case

Yeah, that's right, ignore the relevant comment. I missed that you changed the existing function.

[alpkom]

Looks good to me.
Just one minor thing in the in-line comments.

[yamaceay]

@alpkom tests pass again, feel free to review. FYI, now we have a chain of model_dumps until TargetColumn, and the prediction_placeholder is not excluded anymore, while other attributes are excluded if None set.