RHODS-8871: Remove mandatory typing from requests #186

RobGeada · 2023-05-30T10:34:03Z

Metric requests now look like:

curl -X POST --location "http://{{host}}/metrics/spd/request" \
    -H "Content-Type: application/json" \
    -d "{
          \"modelId\": \"example-model-1\",
          \"protectedAttribute\": \"input-2\",
          \"favorableOutcome\": 1.0
          \"outcomeName\": \"output-0\",
          \"privilegedAttribute\": 1.0,
          \"unprivilegedAttribute\": 0.0
        }"

Request values are compared to see if they are compatible with the expected types from the metadata.

Many thanks for submitting your Pull Request ❤️!

Please make sure that your PR meets the following requirements:

You have read the contributors guide
Pull Request title is properly formatted: FAI-XYZ Subject
Pull Request title contains the target branch if not targeting main: [0.3.x] FAI-XYZ Subject
Pull Request contains link to the JIRA issue
Pull Request contains link to any dependent or related Pull Request
Pull Request contains description of the issue
Pull Request does not include fixes for issues other than the main ticket

openshift-ci-robot · 2023-05-30T10:34:07Z

@RobGeada: This pull request references RHODS-8871 which is a valid jira issue.

In response to this:

Metric requests now look like:
curl -X POST --location "http://{{host}}/metrics/spd/request" \
   -H "Content-Type: application/json" \
   -d "{
         \"modelId\": \"example-model-1\",
         \"protectedAttribute\": \"input-2\",
         \"favorableOutcome\": 1.0
         \"outcomeName\": \"output-0\",
         \"privilegedAttribute\": 1.0,
         \"unprivilegedAttribute\": 0.0
       }"
Request values are compared to see if they are compatible with the expected types from the metadata.

Many thanks for submitting your Pull Request ❤️!

Please make sure that your PR meets the following requirements:

You have read the contributors guide

Pull Request title is properly formatted: FAI-XYZ Subject

Pull Request title contains the target branch if not targeting main: [0.3.x] FAI-XYZ Subject

Pull Request contains link to the JIRA issue

Pull Request contains link to any dependent or related Pull Request

Pull Request contains description of the issue

Pull Request does not include fixes for issues other than the main ticket

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2023-05-30T10:34:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RobGeada

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:

~~OWNERS~~ [RobGeada]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

danielezonca

Overall looks good to me, some comments but mainly minor/codestyle

Good PR :)

danielezonca · 2023-05-31T17:03:35Z

tests/scripts/install.sh

-    sed -i "s#value: \"quay.io/trustyai/trustyai-service:latest\"#value: \"quay.io/trustyai/trustyai-service-ci:${BRANCH_SHA}#"
+    BRANCH_SHA=$(curl  https://api.github.com/repos/trustyai-explainability/trustyai-explainability/pulls/164 | jq ".head.sha" | tr -d '"')
+    sed -i "s#value: \"quay.io/trustyai/trustyai-service:latest\"#value: \"quay.io/trustyai/trustyai-service-ci:${BRANCH_SHA}\"#" ./${KFDEF_FILENAME}
+    cat ./${KFDEF_FILENAME}


This is useful in the logs of the CI, I'd like to keep it

...c/test/java/org/kie/trustyai/service/endpoints/metrics/DisparateImpactRatioEndpointTest.java

danielezonca · 2023-05-31T17:10:24Z

explainability-service/src/main/java/org/kie/trustyai/service/payloads/values/DataType.java

@@ -7,5 +7,6 @@ public enum DataType {
    INT32,
    INT64,
    STRING,
-    MAP
+    MAP,
+    UNKNOWN,


Unused? I don't see any usage of it

Which? I use UNKNOWN. Tommaso added MAP

explainability-service/src/main/java/org/kie/trustyai/service/payloads/PayloadConverter.java

...e/src/main/java/org/kie/trustyai/service/endpoints/metrics/DisparateImpactRatioEndpoint.java

danielezonca · 2023-05-31T17:19:00Z

...e/src/main/java/org/kie/trustyai/service/endpoints/metrics/DisparateImpactRatioEndpoint.java

-        return Response.ok(calculator.getDIRDefinition(request.getMetricValue(), request)).build();
+        final ReconciledMetricRequest reconciledMetricRequest;
+        try {
+            reconciledMetricRequest = ReconciledMetricRequest.reconcile(request, dataSource);


Why the reconcile method here is invoked in a try/catch while in the dir method this is not the case?
Are you sure the DataframeCreateException cannot happen in that method too?

In this function, I'm calling:

ReconciledMetricRequest.reconcile(rawRequest, dataSource)

In dir, I'm calling:

ReconciledMetricRequest.reconcile(rawRequest, metadata)

The DataframeCreateException can be thrown on the creation of metadata from a dataSource, which happens only in the first invocation.

...ava/org/kie/trustyai/service/endpoints/metrics/GroupStatisticalParityDifferenceEndpoint.java

danielezonca · 2023-05-31T17:30:37Z

...ability-service/src/main/java/org/kie/trustyai/service/payloads/ReconciledMetricRequest.java

+        this.privilegedAttribute = new TypedValue();
+        this.privilegedAttribute.setType(protectedType);
+        this.privilegedAttribute.setValue(request.getPrivilegedAttribute());
+
+        this.unprivilegedAttribute = new TypedValue();
+        this.unprivilegedAttribute.setType(protectedType);
+        this.unprivilegedAttribute.setValue(request.getUnprivilegedAttribute());


Are we sure that privilegedAttribute and unprivilegedAttribute have the same type (protectedType)?

They will need to, yes

danielezonca · 2023-05-31T17:33:10Z

...ability-service/src/main/java/org/kie/trustyai/service/payloads/ReconciledMetricRequest.java

+    public static ReconciledMetricRequest reconcile(BaseMetricRequest request, Instance<DataSource> dataSource) {
+        final Metadata metadata = dataSource.get().getMetadata(request.getModelId());


It is a minor comment but I would prefer to do the get() outside this class (aka remove this overload and keep the other method) to avoid to bring usage of javax.enterprise.inject.Instance (aka J2EE) into this class.
As alternative you can replace Instance with Supplier and invoke this method like reconcile(request, dataSource::get)
Btw this is minor, it is mainly to limit the spreading of J2EE classes

danielezonca · 2023-05-31T17:46:29Z

...ability-service/src/main/java/org/kie/trustyai/service/payloads/ReconciledMetricRequest.java

+    public ReconciledMetricRequest(String protectedAttribute, String outcomeName, String modelId, String requestName, String metricName, Double thresholdDelta, Integer batchSize,
+            TypedValue privilegedAttribute, TypedValue favorableOutcome, TypedValue unprivilegedAttribute) {
+        this.protectedAttribute = protectedAttribute;
+        this.outcomeName = outcomeName;
+        this.modelId = modelId;
+        this.requestName = requestName;
+        this.metricName = metricName;
+        this.thresholdDelta = thresholdDelta;
+        this.batchSize = batchSize;
+        this.privilegedAttribute = privilegedAttribute;
+        this.favorableOutcome = favorableOutcome;
+        this.unprivilegedAttribute = unprivilegedAttribute;
+    }


As far as I see it is not used, if so, can you remove it?
I'm not a fan of constructors with a lot of fields with the same type like here (String abc, String def, String ghi) because it is easy to swap a parameter and get a bug.
Especially if it is not used :)

It is used in the creation of the ReconciledDefinitionRequest class, in the super() invocation

RobGeada · 2023-06-02T12:33:00Z

/ok-to-test

openshift-ci · 2023-06-02T15:45:30Z

@RobGeada: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/trustyai-explainability-e2e	`80928cc`	link	true	`/test trustyai-explainability-e2e`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

RobGeada requested a review from a team as a code owner May 30, 2023 10:34

RobGeada requested review from ruivieira and tteofili and removed request for a team May 30, 2023 10:34

openshift-merge-robot added the needs-rebase label May 30, 2023

openshift-ci bot requested review from danielezonca and taneem-ibrahim May 30, 2023 10:34

openshift-ci bot added the approved label May 30, 2023

openshift-merge-robot removed the needs-rebase label May 30, 2023

RobGeada added 3 commits May 30, 2023 11:43

Removed mandatory typing from request body

573fd98

Removed mandatory typing, ensure long compatibility

4941a7b

Update ci tests

4ae1b51

RobGeada force-pushed the RHODS-8871 branch from 9e2c5cd to 4ae1b51 Compare May 30, 2023 10:44

fix broken merge

d1d7f21

alexcreasy mentioned this pull request May 30, 2023

TrustyAI Service Client opendatahub-io/odh-dashboard#1152

Closed

RobGeada added 6 commits May 30, 2023 14:08

fix missing sed file

31a27f5

fix/debug sed patch

de12c5e

added missing quote

2106cf1

debug operator logs

83e78ed

update location of debug echo of operator logs

a2ca3da

update mm manifest schema

1ac61aa

danielezonca reviewed May 31, 2023

View reviewed changes

RobGeada added 5 commits June 1, 2023 10:26

test different method of identifying branch

59884cd

add composite workflows

6214cff

removed action changes

94e92cd

removed action changes 2

20218bd

fixed pull number issue

22531ba

openshift-ci bot added the ok-to-test label Jun 2, 2023

RobGeada added 2 commits June 2, 2023 14:06

Addressed Daniele's comments

70c5cb9

Restored kfdef sed

80928cc

This was referenced Jun 5, 2023

Update Trusty AI client to handle API changes opendatahub-io/odh-dashboard#1336

Closed

Update Trusty AI client to handle API changes (#1336) opendatahub-io/odh-dashboard#1337

Merged

DaoDaoNoCode mentioned this pull request Jun 6, 2023

Add bias metrics configuration modal opendatahub-io/odh-dashboard#1343

Merged

6 tasks

RobGeada merged commit e928a2a into trustyai-explainability:main Jun 9, 2023
6 of 8 checks passed

ruivieira mentioned this pull request Jul 15, 2023

[Enhancement] Remove types from request payloads #197

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RHODS-8871: Remove mandatory typing from requests #186

RHODS-8871: Remove mandatory typing from requests #186

RobGeada commented May 30, 2023

openshift-ci-robot commented May 30, 2023 •

edited by openshift-ci bot

openshift-ci bot commented May 30, 2023

danielezonca left a comment

danielezonca May 31, 2023

RobGeada Jun 2, 2023

danielezonca May 31, 2023

RobGeada Jun 2, 2023

danielezonca May 31, 2023

RobGeada Jun 2, 2023 •

edited

danielezonca May 31, 2023

RobGeada Jun 2, 2023

danielezonca May 31, 2023

danielezonca May 31, 2023

RobGeada Jun 2, 2023

RobGeada commented Jun 2, 2023

openshift-ci bot commented Jun 2, 2023

		public static ReconciledMetricRequest reconcile(BaseMetricRequest request, Instance<DataSource> dataSource) {
		final Metadata metadata = dataSource.get().getMetadata(request.getModelId());

RHODS-8871: Remove mandatory typing from requests #186

RHODS-8871: Remove mandatory typing from requests #186

Conversation

RobGeada commented May 30, 2023

openshift-ci-robot commented May 30, 2023 • edited by openshift-ci bot

openshift-ci bot commented May 30, 2023

danielezonca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobGeada Jun 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobGeada commented Jun 2, 2023

openshift-ci bot commented Jun 2, 2023

openshift-ci-robot commented May 30, 2023 •

edited by openshift-ci bot

RobGeada Jun 2, 2023 •

edited