-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added NP + PD support to parsing for both normal + raw cases #229
Conversation
/test trustyai-explainability-e2e |
/retest-required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
.../main/java/org/kie/trustyai/service/endpoints/explainers/global/GlobalExplainerEndpoint.java
Outdated
Show resolved
Hide resolved
...lity-service/src/main/java/org/kie/trustyai/service/endpoints/consumer/ConsumerEndpoint.java
Outdated
Show resolved
Hide resolved
.addFp64Contents(values.get(2)); | ||
|
||
ModelInferResponse.InferOutputTensor outputTensor = ModelInferResponse.InferOutputTensor.newBuilder() | ||
.setDatatype("FP64") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be nice to have the possible types in some enum or similar but I don't think we can easily customise these generated classes so let's stay with this for now. We can create a follow up ticket if we agree
...lity-connectors/src/test/java/org/kie/trustyai/connectors/kserve/v2/TensorConverterTest.java
Outdated
Show resolved
Hide resolved
...ainability-connectors/src/main/java/org/kie/trustyai/connectors/kserve/v2/PayloadParser.java
Show resolved
Hide resolved
...ainability-connectors/src/main/java/org/kie/trustyai/connectors/kserve/v2/PayloadParser.java
Show resolved
Hide resolved
...bility-connectors/src/test/java/org/kie/trustyai/connectors/kserve/v2/KServeFormatsTest.java
Outdated
Show resolved
Hide resolved
...e/src/test/java/org/kie/trustyai/service/endpoints/consumer/ConsumerEndpointFormatsTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On top of @danielezonca's comments and mine, I think we should add unit tests to the enforced dimensions, eg.
- fail when dimensions do not match
- test without hardcoded dimensions
- test the
InferencePayloadReconciler
with malformed payloads
...nability-connectors/src/main/java/org/kie/trustyai/connectors/kserve/v2/TensorConverter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm :)
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danielezonca, ruivieira, tteofili The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
New changes are detected. LGTM label has been removed. |
/test trustyai-explainability-e2e |
/test trustyai-explainability-e2e |
Refactor Rui's NP+PD PR to enable support for raw parsing as well, as well as fix float type issues.
Resolves: #136
Many thanks for submitting your Pull Request ❤️!
Please make sure that your PR meets the following requirements:
FAI-XYZ Subject
[0.3.x] FAI-XYZ Subject