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

[alpkom]

This is a mandatory field, not optional.

[yamaceay]

"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?

[alpkom]

It might be actually the case that it's optional, and they just defined it in a weird way in the spec.

To be on the safe side, could you find out, how the server behaves when it's sent as null vs. when it's not sent at all as part of the request? And how would the model know what to predict at all, if it's not provided?

[mwien]

My interpretation: It is indeed required (not send at all will error) and if it is sent as null then all null entries in the table will be replaced by the RPT predictions.

The current code should enforce this if I understand the Pydantic conventions correctly. Though Union[str, int, float, None] should be the same and may make intentions more clear. All under the assumptions that it is indeed required ofc.

[yamaceay]

I didn't fully get the difference between Optional[Union[str, int, float]] and Union[str, int, float, None]. Subjectively, the latter seems to have a "required" energy by leaving out Optional type. But I believe it might be more consistent with the overall repo to still keep the first option with explicit Optional, so I would rather keep it as it is. At the end, I leave it up to you, what do you think?

[mwien]

Both should be identical on a technical level. If you prefer the Optional one, then I'm fine with that. More important is to double-check that the field is indeed required.

[alpkom]

I still believe it's counterintuitive to set a default value implicitly for a parameter which is required by the backend. (I don't think it matters if the 'null' is an accepted value for that parameter or not.)
Regarding the consistency with the rest of the SDK: for the cases where SDK defines a parameter as Optional, that parameters is actually optional in the backend, as well. (Let me know if there's an exception to this). So that's where this case differs from the rest. This one is required by the backend. It just happens to be that 'null' is a valid value.

[mwien]

If I understand correctly, then only doing prediction_placeholder: Optional[Union[str, int, float]] = None would set None implicitly as default value. In the code above (without = None), it would set no default value and Pydantic excepts either None, str, int, float but one of those needs to be explicitly given.

[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.

[alpkom]

This is imported but not used. So, the new mock response is not tested at all.