NEOS-1560: expose entities in api for transformpiitext #2836

nickzelei · 2024-10-22T21:51:33Z

adds new rpc in transformers service that exposes available pii entities
updates transform_pii_text config to allow specifying allowed entities

linear · 2024-10-22T21:51:36Z

NEOS-1560 Expose Entities allow/deny in API for TransformPiiText

vercel · 2024-10-22T21:51:36Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
neosync-docs	⬜️ Ignored (Inspect)	Visit Preview		Oct 22, 2024 11:15pm

github-actions · 2024-10-22T21:51:52Z

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
`✅ passed`	`✅ passed`	`✅ passed`	`✅ passed`	Oct 22, 2024, 11:16 PM

nickzelei · 2024-10-22T21:52:35Z

backend/protos/mgmt/v1alpha1/transformer.proto

@@ -501,4 +519,9 @@ service TransformersService {
  rpc IsTransformerNameAvailable(IsTransformerNameAvailableRequest) returns (IsTransformerNameAvailableResponse) {}
  rpc ValidateUserJavascriptCode(ValidateUserJavascriptCodeRequest) returns (ValidateUserJavascriptCodeResponse) {}
  rpc ValidateUserRegexCode(ValidateUserRegexCodeRequest) returns (ValidateUserRegexCodeResponse) {}
+
+  // Retrieve a list of available Pii recognizers for use with the TransformPiiText transformer
+  rpc GetTransformPiiRecognizers(GetTransformPiiRecognizersRequest) returns (GetTransformPiiRecognizersResponse) {


I put this here because it seemed more generic than just shoving it in the anonymization service since it is specific to the transformPiitext transformer.
wdyt @alishakawaguchi, @evisdrenova ?

Is this referring to the getEntities endpoint that we talked about or something else? If so, are we just using recognizers instead of entities everywhere? I like entities because that's industry standard i.e. Named Entity Recognition model.

oh yeah actually good call, I'm surfacing the wrong function. I should be surfacing the entities, not the entity recognziers.

That is kind of beside my original question though.

yeah back to your original point - IMO i'd put it in the anonymizer service since it retrieves the entities from the presidio api and functionally is currently only used for anonymization even if it is consumed by a transformer

only problem is that ceases to be true once we wire it up to the async job flow. it's a supporting value for a transformer

which i think is okay imo - you're still getting those values from presidio it's just being used by the transformer, for ex. we have the connection rpcs in the connection_data service even though it's an input into a job. but i think you can go either way with it

I think the transformer service makes the most sense

nickzelei · 2024-10-22T21:58:49Z

backend/protos/mgmt/v1alpha1/transformer.proto

+  // Configure a list of recognizers to be used for PII analysis. If not provided or empty, all recognizers are used
+  // If this is specified, any ad-hoc, or deny_recognizers names must also be provided.
+  // To see available builtin recognizers, call the GetPiiTextRecognizers() RPC method to see what is available for your account.
+  repeated string allowed_recognizers = 4 [(buf.validate.field).repeated = {


What do we think about this name?

Will it be confusing when looking at the property above called deny_recognizers?

Should we have a different name here?
wdyt: @alishakawaguchi @evisdrenova ?

Maybe it makes more sense once we rename the deny_recognizers to just be recognizers or adhoc_recognizers

Hmm I think it shoudl be called something different since it's not technically a recognizer as we have it defined in the deny_recognizer from a type perspective. I think it might be be clearer to call it something like allowed_terms or allowed_words or something along that line?

sorry I was calling it the wrong thing as I was exposing the wrong function. this is will more clearly be named allowed_entities now

codecov · 2024-10-22T22:03:07Z

Codecov Report

Attention: Patch coverage is 17.80822% with 60 lines in your changes missing coverage. Please review.

Project coverage is 30.36%. Comparing base (dd554a2) to head (f038598).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...ces/mgmt/v1alpha1/transformers-service/entities.go	0.00%	24 Missing ⚠️
...alpha1/transformers-service/system_transformers.go	40.74%	16 Missing ⚠️
backend/internal/cmds/mgmt/serve/connect/cmd.go	0.00%	15 Missing ⚠️
...1/transformers-service/userdefined_transformers.go	0.00%	4 Missing ⚠️
...ices/mgmt/v1alpha1/transformers-service/service.go	0.00%	1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (dd554a2) and HEAD (f038598). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (dd554a2) HEAD (f038598)

2 1

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2836      +/-   ##
==========================================
- Coverage   37.33%   30.36%   -6.98%     
==========================================
  Files         296      297       +1     
  Lines       34693    34743      +50     
==========================================
- Hits        12953    10549    -2404     
- Misses      20210    23023    +2813     
+ Partials     1530     1171     -359

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

evisdrenova · 2024-10-22T23:04:13Z

backend/services/mgmt/v1alpha1/transformers-service/entities.go

+		return nil, fmt.Errorf("was unable to retrieve available recognizers: %w", err)
+	}


Suggested change

return nil, fmt.Errorf("was unable to retrieve available recognizers: %w", err)

}

return nil, fmt.Errorf("was unable to retrieve available entities: %w", err)

}

evisdrenova · 2024-10-22T23:04:45Z

backend/services/mgmt/v1alpha1/transformers-service/entities.go

+		return nil, fmt.Errorf("received non-200 response from recognizer api: %s %d %s", resp.Status(), resp.StatusCode(), string(resp.Body))
+	}


Suggested change

return nil, fmt.Errorf("received non-200 response from recognizer api: %s %d %s", resp.Status(), resp.StatusCode(), string(resp.Body))

}

return nil, fmt.Errorf("received non-200 response from entities api: %s %d %s", resp.Status(), resp.StatusCode(), string(resp.Body))

}

evisdrenova · 2024-10-22T23:07:28Z

backend/services/mgmt/v1alpha1/transformers-service/system_transformers.go

+	baseSystemTransformerSourceMap = map[mgmtv1alpha1.TransformerSource]*mgmtv1alpha1.SystemTransformer{}
+
+	allSystemTransformersSourceMap = map[mgmtv1alpha1.TransformerSource]*mgmtv1alpha1.SystemTransformer{}


later would be nice to show all the transformers regardless of plan but have some disabled with like an "upgrade now" button to incentivize users to upgrade in order to get access to those transformers if they're in like a personal plan

evisdrenova

LGTM - just left a couple of small comments

nickzelei added 5 commits October 22, 2024 14:00

adds generated code

1d7de62

wires up entities, adds ability to get recognizers

86524a6

wires up recognizer

1964a2a

adds mock recognizer

1153893

adds integration test for recognizers

88322d8

nickzelei self-assigned this Oct 22, 2024

Simplify method

201f33d

nickzelei commented Oct 22, 2024

View reviewed changes

nickzelei added the enhancement New feature or request label Oct 22, 2024

nickzelei requested a review from alishakawaguchi October 22, 2024 22:00

nickzelei marked this pull request as ready for review October 22, 2024 22:13

updates to be allowed entities

e708fee

vercel bot deployed to Preview October 22, 2024 22:26 View deployment

nickzelei requested a review from evisdrenova October 22, 2024 22:28

evisdrenova reviewed Oct 22, 2024

View reviewed changes

evisdrenova approved these changes Oct 22, 2024

View reviewed changes

updates error messages

f038598

alishakawaguchi approved these changes Oct 22, 2024

View reviewed changes

nickzelei merged commit b43319b into main Oct 22, 2024
16 of 19 checks passed

nickzelei deleted the nick/neos-1560-expose-entities-allowdeny-in-api-for-transformpiitext branch October 22, 2024 23:53

BrewTestBot mentioned this pull request Oct 24, 2024

neosync 0.4.81 Homebrew/homebrew-core#195413

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEOS-1560: expose entities in api for transformpiitext #2836

NEOS-1560: expose entities in api for transformpiitext #2836

nickzelei commented Oct 22, 2024 •

edited

Loading

linear bot commented Oct 22, 2024

vercel bot commented Oct 22, 2024 •

edited

Loading

github-actions bot commented Oct 22, 2024 •

edited

Loading

nickzelei Oct 22, 2024

evisdrenova Oct 22, 2024

nickzelei Oct 22, 2024

nickzelei Oct 22, 2024

evisdrenova Oct 22, 2024

nickzelei Oct 22, 2024

evisdrenova Oct 22, 2024

alishakawaguchi Oct 22, 2024

nickzelei Oct 22, 2024

evisdrenova Oct 22, 2024

nickzelei Oct 22, 2024

codecov bot commented Oct 22, 2024 •

edited

Loading

evisdrenova Oct 22, 2024

evisdrenova Oct 22, 2024

evisdrenova Oct 22, 2024 •

edited

Loading

evisdrenova left a comment

		return nil, fmt.Errorf("was unable to retrieve available recognizers: %w", err)
		}

		return nil, fmt.Errorf("received non-200 response from recognizer api: %s %d %s", resp.Status(), resp.StatusCode(), string(resp.Body))
		}

		baseSystemTransformerSourceMap = map[mgmtv1alpha1.TransformerSource]*mgmtv1alpha1.SystemTransformer{}

		allSystemTransformersSourceMap = map[mgmtv1alpha1.TransformerSource]*mgmtv1alpha1.SystemTransformer{}

NEOS-1560: expose entities in api for transformpiitext #2836

NEOS-1560: expose entities in api for transformpiitext #2836

Conversation

nickzelei commented Oct 22, 2024 • edited Loading

linear bot commented Oct 22, 2024

vercel bot commented Oct 22, 2024 • edited Loading

github-actions bot commented Oct 22, 2024 • edited Loading

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

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

codecov bot commented Oct 22, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evisdrenova Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

evisdrenova left a comment

Choose a reason for hiding this comment

nickzelei commented Oct 22, 2024 •

edited

Loading

vercel bot commented Oct 22, 2024 •

edited

Loading

github-actions bot commented Oct 22, 2024 •

edited

Loading

codecov bot commented Oct 22, 2024 •

edited

Loading

evisdrenova Oct 22, 2024 •

edited

Loading