Support for handling sensitive data sent to remote services

[pawelknes]

Added modifications to loaders and the RemoteMetric class so that a metric can check if it's allowed to handle data which is sent to a remote service as per #646

[pawelknes]

I think it might simplify things if you use AddFields operator to add the classification:

self.add_data_classification = AddFields({"data_classification": self.data_classification})
multi_stream = MultiStream.from_iterables(dataset)
return self.add_data_classification(multi_stream)

I think this approach is also more plausible from the prespective of code responsibility seperation.

Also do we want to have some sort of hierarchy of data classification types?

Thanks for the suggestion

@yoavkatz what about this hierarchy?

[yoavkatz]

Hi @pawelknes . Thanks for the PR. It looks very good.

Since the issue started, we saw the need for this expand beyond metrics. For example, a person added a Translate post processor that sends the predicted data to IBM Cloud translation service. There is also a new InferenceEngine class that is sends data to 3rd party services (used by LLM as judges metric)

So perhaps instead of making metric centric, we should make it more generic (applicable for all Artifacts).

Similar to how class ComputeExpressionMixin(Artifact) adds ability to compute expressions.

What do you think?

[pawelknes]

Hi @pawelknes . Thanks for the PR. It looks very good.

Since the issue started, we saw the need for this expand beyond metrics. For example, a person added a Translate post processor that sends the predicted data to IBM Cloud translation service. There is also a new InferenceEngine class that is sends data to 3rd party services (used by LLM as judges metric)

So perhaps instead of making metric centric, we should make it more generic (applicable for all Artifacts).

Similar to how class ComputeExpressionMixin(Artifact) adds ability to compute expressions.

What do you think?

Sure, it sounds good and I can work on this. Though we want to add this in the current PR, or firstly merge it and then create a new issue for expanding it to a general case?

[yoavkatz]

Hi @pawelknes . Thanks for the PR. It looks very good. Since the issue started, we saw the need for this expand beyond metrics. For example, a person added a Translate post processor that sends the predicted data to IBM Cloud translation service. There is also a new InferenceEngine class that is sends data to 3rd party services (used by LLM as judges metric) So perhaps instead of making metric centric, we should make it more generic (applicable for all Artifacts). Similar to how class ComputeExpressionMixin(Artifact) adds ability to compute expressions. What do you think?

Sure, it sounds good and I can work on this. Though we want to add this in the current PR, or firstly merge it and then create a new issue for expanding it to a general case?

I tend to think that we should do it now, because in anycase, we will need to document it in details and instruct the users. We probably don't want to do it twice. Looking the code, it may not be a huge change. More of renaming and moving the code from metrics to artifact.

[pawelknes]

Hi @pawelknes . Thanks for the PR. It looks very good. Since the issue started, we saw the need for this expand beyond metrics. For example, a person added a Translate post processor that sends the predicted data to IBM Cloud translation service. There is also a new InferenceEngine class that is sends data to 3rd party services (used by LLM as judges metric) So perhaps instead of making metric centric, we should make it more generic (applicable for all Artifacts). Similar to how class ComputeExpressionMixin(Artifact) adds ability to compute expressions. What do you think?

Sure, it sounds good and I can work on this. Though we want to add this in the current PR, or firstly merge it and then create a new issue for expanding it to a general case?

I tend to think that we should do it now, because in anycase, we will need to document it in details and instruct the users. We probably don't want to do it twice. Looking the code, it may not be a huge change. More of renaming and moving the code from metrics to artifact.

Okay, so I'll start working on it

[pawelknes]

@yoavkatz I've moved functionalities so that they can be used by any artifact. As for the operator responsible for verifications, I made it accepting either an artifact name (and read a respective policy from env) or a Python object (Accuracy etc.) and read a policy from the attribute

[yoavkatz]

Pawel - thanks for this PR. It's an important one, especially since we have LLM as judges. I've made some additions and changes to the PR - I'll be glad for your review.

1. I added default data classifications policies to loaders, so users will not be required to add them always.
2. Reviewed and added more documentation
3. Made the use of the environment variable more consistent with other place
4. Made the default policy 'None' in artifact so we know if the user explicitly set it , or not.

[yoavkatz]

@pawelknes - I think the next steps are:

1. Remove allow_passing_data_to_remote_api from settings.

2. In class InferenceEngine(abc.ABC, Artifact): Make sure infer() checks data classification of the inferred instance (best to do it in the base method, so people writing inference engines won't need to handle lit). Make BAM and WML handle 'public' and 'proprietary' data by default.

3. LlamaIndexCorrectness(InstanceMetric) - make it process only 'public' data by default.

4. class RemoteMetric(SingleStreamOperator, Metric) - make it handle 'public' and 'proprietary' data by default.

[pawelknes]

@pawelknes - I think the next steps are:

1. Remove allow_passing_data_to_remote_api from settings.
2. In class InferenceEngine(abc.ABC, Artifact): Make sure infer() checks data classification of the inferred instance (best to do it in the base method, so people writing inference engines won't need to handle lit). Make BAM and WML handle 'public' and 'proprietary' data by default.
3. LlamaIndexCorrectness(InstanceMetric) - make it process only 'public' data by default.
4. class RemoteMetric(SingleStreamOperator, Metric) - make it handle 'public' and 'proprietary' data by default.

Yup, I'll make these changes