Push Feature - Merged #1198

Googleton · 2023-06-23T09:12:11Z

Description

Googleton · 2023-06-23T09:13:47Z

For some reason my formatter still changed a ton of white spaces. Any idea @andreybavt ?

Removed self.idrow to avoid pickling "self"

…to feature/merging-push-feature

…to feature/merging-push-feature # Conflicts: # python-client/giskard/ml_worker/server/ml_worker_service.py

kevinmessiaen · 2023-06-28T03:21:18Z

python-client/tests/test_push.py

+
+
+# Classification
+def test_instance_if_not_none(german_credit_model, german_credit_data):


I think that we can have parameterized tests to avoid rewriting the same test for each model type

kevinmessiaen · 2023-07-03T02:45:45Z

frontend/src/stores/push.ts

+    current: Pushes | undefined;
+    identifier: PushIdentifier | undefined;


You can use ?: in ts to say that a field is optional:

Suggested change

current: Pushes | undefined;

identifier: PushIdentifier | undefined;

current?: Pushes;

identifier?: PushIdentifier;

kevinmessiaen · 2023-07-03T02:49:40Z

frontend/src/stores/push.ts

+                // @ts-ignore
+                this.pushes[modelId][datasetId][rowNb] = await api.getPushes(modelId, datasetId, rowNb);


Suggested change

// @ts-ignore

this.pushes[modelId][datasetId][rowNb] = await api.getPushes(modelId, datasetId, rowNb);

this.pushes[modelId][datasetId][rowNb] = await api.getPushes(modelId, datasetId, rowNb) as Pushes;

kevinmessiaen · 2023-07-03T02:52:43Z

frontend/src/views/main/project/InspectorWrapper.vue

 }

+watch(() => rowNb.value, async (newValue, oldValue) => {
+  console.log("RowNb changed");


We can delete this log

Suggested change

console.log("RowNb changed");

kevinmessiaen · 2023-07-03T03:03:45Z

python-client/giskard/ml_worker/server/ml_worker_service.py

+        if contribs is not None:
+            contrib_grpc = contribs.to_grpc()
+        else:
+            contrib_grpc = None
+
+        if perturbs is not None:
+            perturb_grpc = perturbs.to_grpc()
+        else:
+            perturb_grpc = None
+
+        if overconf is not None:
+            overconf_grpc = overconf.to_grpc()
+        else:
+            overconf_grpc = None
+
+        if borderl is not None:
+            borderl_grpc = borderl.to_grpc()
+        else:
+            borderl_grpc = None


I have feeling that this code is a bit repetitive. To me it would be better if we store those push in a list/dict to better handle them in a loop:

pushes = { PushKind.Perturbation: create_perturbation_push(model, dataset, request.rowidx), ... } pushes_grpc = { push_kind: push.to_grpc() if push is not None else None for push_kind, push for pushes.items()

kevinmessiaen · 2023-07-03T03:04:54Z

python-client/giskard/ml_worker/server/ml_worker_service.py

+            borderl_grpc = None
+
+        if request.cta_kind > 0 and request.push_kind > 0:
+            if request.push_kind == PushKind.Perturbation:


Plus this would become: push = pushes[request.push_kind]

kevinmessiaen · 2023-07-07T03:14:26Z

python-client/giskard/push/contribution.py

+        # slice_df = ds.copy()
+        # slice_df.df = df.reset_index(drop=True)
+        # slice_df.meta.number_of_rows = 1
+        # slice_df = ds.slice(lambda row: row.eq(df.iloc[0]).all(axis=None)) 3 solutions I think the first is the best


Yes I also think that the first looks cleaner

kevinmessiaen · 2023-07-07T03:19:44Z

python-client/giskard/push/contribution.py

-        shap_res = _contribution_push(model, ds, idrow)
-        slice_df = ds.slice(lambda df: df.loc[[idrow]], row_level=False)
+        shap_res = _contribution_push(model, ds, df)
+        slice_df = ds.slice(lambda row: row.equals(df.iloc[0]))


I'm not sure why we need to do a row by row comparison like that, it seems suboptimal.

andreybavt

Check Sonar's issues and please fix them too

andreybavt · 2023-07-07T15:39:53Z

backend/src/main/java/ai/giskard/service/PushService.java

+import java.util.Map;
+
+@Service
+//@Transactional


It's rightfully not transactional since it's communicating with the ML Worker inside, you can remove the comment

andreybavt · 2023-07-07T15:41:37Z

frontend/src/api.ts

+        );
+    },
+    async getPushes(modelId: string, datasetId: string, idx: number, features: any) {
+        return apiV2.post<unknown, unknown>(`/pushes/${modelId}/${datasetId}/${idx}`, {


Why do you use Post request to get elements?

Why not specifying the type of the return object?

andreybavt · 2023-07-07T15:42:15Z

frontend/src/stores/debugging-sessions.ts

+        selectedSlicingFunction: {
+            uuid: undefined,
+            params: [],
+            type: 'SLICING'


'SLICING' should probably be an enum

andreybavt · 2023-07-07T15:42:42Z

frontend/src/stores/push.ts

+            this.identifier = {modelId, datasetId, rowNb, inputData, modelFeatures};
+            this.current = undefined;
+
+            // if (!this.pushes[modelId]) {


andreybavt · 2023-07-07T15:42:58Z

frontend/src/components/PushPopover.vue

+          <v-btn small icon outlined color="warning" class="ml-1"
+                 v-on="{ ...onMenu, ...onTooltip }">
+            <v-icon size="18" color="warning">mdi-alert-outline</v-icon>
+            <!--<v-badge top overlap color="transparent">


# Conflicts: # frontend/src/generated-sources/j2ts-generated-metadata.json # frontend/src/views/main/project/Inspector.vue

…to feature/merging-push-feature

…ature/merging-push-feature

…to feature/merging-push-feature

# Conflicts: # backend/src/main/resources/config/liquibase/master.xml # frontend/src/generated-sources/index.ts # frontend/src/generated-sources/j2ts-generated-metadata.json

…to feature/merging-push-feature

# Conflicts: # frontend/src/views/main/project/Inspector.vue

…to feature/merging-push-feature

…to feature/merging-push-feature # Conflicts: # python-client/giskard/push/utils.py

sonarqubecloud · 2023-09-01T13:01:13Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

46.4% Coverage
0.0% Duplication

) * Push Feature - Merged (#1198) Push feature --------- Co-authored-by: Mathieu Roques <[email protected]> Co-authored-by: Andrey Avtomonov <[email protected]> Co-authored-by: Kevin Messiaen <[email protected]> Co-authored-by: Rabah Abdul Khalek <[email protected]> Co-authored-by: Henrique Chaves <[email protected]> Co-authored-by: Henrique Chaves <[email protected]> * reducing bounds for push contribution * fixed missing if * restored Frontend.run.xml * list to tuple * fixed tests * fixed tests --------- Co-authored-by: Googleton <[email protected]> Co-authored-by: Mathieu Roques <[email protected]> Co-authored-by: Andrey Avtomonov <[email protected]> Co-authored-by: Kevin Messiaen <[email protected]> Co-authored-by: Henrique Chaves <[email protected]> Co-authored-by: Henrique Chaves <[email protected]>

* hashed TextTypoTransformation * Push Feature - Merged (#1198) Push feature --------- Co-authored-by: Mathieu Roques <[email protected]> Co-authored-by: Andrey Avtomonov <[email protected]> Co-authored-by: Kevin Messiaen <[email protected]> Co-authored-by: Rabah Abdul Khalek <[email protected]> Co-authored-by: Henrique Chaves <[email protected]> Co-authored-by: Henrique Chaves <[email protected]> * Update python-client/giskard/push/perturbation.py Co-authored-by: Andrey Avtomonov <[email protected]> * removed hashing logic, using seed-based approach instead * added comment about hashed_seed --------- Co-authored-by: Googleton <[email protected]> Co-authored-by: Mathieu Roques <[email protected]> Co-authored-by: Andrey Avtomonov <[email protected]> Co-authored-by: Kevin Messiaen <[email protected]> Co-authored-by: Henrique Chaves <[email protected]> Co-authored-by: Henrique Chaves <[email protected]>

mathieuroques and others added 3 commits June 22, 2023 15:09

Add new files push feature python

2546d4d

Added Suggest filter to ml_worker_service

82405ea

Reapply changes for push (back,front)

f54633e

Googleton requested a review from andreybavt June 23, 2023 09:12

Googleton and others added 5 commits June 23, 2023 15:51

Push Popover fixes

dc8ac8e

Fixed slicing function

a21528f

Removed self.idrow to avoid pickling "self"

Merge remote-tracking branch 'origin/feature/merging-push-feature' in…

bf5f3fb

…to feature/merging-push-feature

CTA part 1, fix overconf push not showing

1e4778b

CTA part 2, the "Save Slice" CTA now works!

d9df0da

andreybavt requested a review from kevinmessiaen June 27, 2023 13:04

mathieuroques and others added 5 commits June 27, 2023 16:23

Automatic threshold for overconfidence and 0.1 for borderline

3cebf2a

Merge branch 'main' into feature/merging-push-feature

67ecefc

Temp commit

bed425d

Merge remote-tracking branch 'origin/feature/merging-push-feature' in…

e70edd6

…to feature/merging-push-feature # Conflicts: # python-client/giskard/ml_worker/server/ml_worker_service.py

Few more CTAs

f24d036

andreybavt marked this pull request as ready for review June 30, 2023 09:06

kevinmessiaen reviewed Jul 3, 2023

View reviewed changes

Googleton and others added 8 commits July 5, 2023 09:48

Change python side to use row data instead of row index (wip)

8c94135

No more pushes?!

eb9053d

It half works now!

912d4cd

changed 0 to df in ml_worker

249cba5

replaced all id by row in contribution

6e34a4b

replaced id by row in perturbation

1cea7a0

Updated prediction push by replacing idrow by df

97716d8

Fix bug in push call in ml worker

fe14ffb

kevinmessiaen reviewed Jul 7, 2023

View reviewed changes

Debugger push works :)

d2fe292

andreybavt suggested changes Jul 7, 2023

View reviewed changes

Merge branch 'main' into feature/merging-push-feature

1d1a1be

# Conflicts: # frontend/src/generated-sources/j2ts-generated-metadata.json # frontend/src/views/main/project/Inspector.vue

Googleton and others added 26 commits August 29, 2023 14:23

Merge remote-tracking branch 'origin/feature/merging-push-feature' in…

3dde349

…to feature/merging-push-feature

Merge remote-tracking branch 'origin/fix/lambda_upload_issue' into fe…

c45c9dc

…ature/merging-push-feature

Merge branch 'main' into feature/merging-push-feature

e85a395

Merge branch 'main' into feature/merging-push-feature

e5454f5

Merge remote-tracking branch 'origin/feature/merging-push-feature' in…

11d50d8

…to feature/merging-push-feature

Cleanup

48e3521

Cleanup

d890822

Merge branch 'main' into feature/merging-push-feature

632f6c4

attempt to fix the docs

29a2ac2

Fix enum python side

e31b7f4

Merge remote-tracking branch 'origin/feature/merging-push-feature' in…

d672251

…to feature/merging-push-feature

relaunching docs

4d35bc7

Merge branch 'main' into feature/merging-push-feature

c83a449

# Conflicts: # backend/src/main/resources/config/liquibase/master.xml # frontend/src/generated-sources/index.ts # frontend/src/generated-sources/j2ts-generated-metadata.json

regenerated j2ts

70b5093

cleanup

45cc1cb

Cleanup

0a24a32

Merge remote-tracking branch 'origin/feature/merging-push-feature' in…

3de82bb

…to feature/merging-push-feature

More cleanup

d044ddd

Merge branch 'main' into feature/merging-push-feature

3d6b7cb

# Conflicts: # frontend/src/views/main/project/Inspector.vue

More little fixes

b286ce4

Merge branch 'main' into feature/merging-push-feature

bf84900

More sonar codesmell fixes

95be4b5

Merge remote-tracking branch 'origin/feature/merging-push-feature' in…

2623030

…to feature/merging-push-feature

fix sonar complaints

14d9244

fix sonar complaints

92656a0

Merge remote-tracking branch 'origin/feature/merging-push-feature' in…

d1760a8

…to feature/merging-push-feature # Conflicts: # python-client/giskard/push/utils.py

andreybavt merged commit 495d238 into main Sep 1, 2023



		# Classification
		def test_instance_if_not_none(german_credit_model, german_credit_data):

		current: Pushes \| undefined;
		identifier: PushIdentifier \| undefined;

		// @ts-ignore
		this.pushes[modelId][datasetId][rowNb] = await api.getPushes(modelId, datasetId, rowNb);

Uh oh!

Push Feature - Merged #1198

Push Feature - Merged #1198

Uh oh!

Conversation

Googleton commented Jun 23, 2023

Description

Uh oh!

Googleton commented Jun 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinmessiaen Jul 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreybavt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Sep 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants

kevinmessiaen Jul 3, 2023 •

edited

Loading