Feature request: Option to disable cross encoder models #286

azaylamba · 2023-12-23T14:37:34Z

Issue #222
Description of changes: Currently cross encoder models are used to rank the search results but the models available need to be hosted on Sagemaker which increases cost significantly. Having an option to disable cross encoder models would be helpful while exploring the chatbot so that Sagemaker costs can be avoided.

Added a config to enable/disable embeddings via Sagemaker which in turn derives cross encoding models.
Persisted enableSagemakerModels config so that it can be used directly instead of relying on sagemakerModels length.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…t the models available need to be hosted on Sagemaker which increases cost significantly. Having an option to disable cross encoder models would be helpful while exploring the chatbot so that Sagemaker costs can be avoided. Added a config to enable/disable cross encoder models. Also added options to selected embedding models, so that sagemakerModels are not created automatically. Persisted enableSagemakerModels config so that it can be used directly instead of relying on sagemakerModels length.

Added basic feedback mechanism for responses generated by the chatbot. The feedbacks are stored in DynamoDB which can be queried to do analysis as required by admin users. In future we can add a UI page to display the feedbacks, but for now these are being stored and manual analysis would be required. The feedbacks are not adding to the learning of the chatbot.

This reverts commit 1c3b8ce.

azaylamba · 2024-01-02T15:32:36Z

@massi-ang could you please have a look?

azaylamba · 2024-01-17T17:11:42Z

@bigadsoleiman I have resolved the merge conflicts in the latest commit.

spugachev · 2024-01-23T13:11:57Z

@azaylamba We have migrated to AppSync. This PR has conflicts. Could you please fix this? And than we can merge

azaylamba · 2024-01-25T06:53:03Z

@spugachev I couldn't find any conflicts in the PR. I have merged the main branch.

Note: I have not tested the changes after syncing with main due to some constraints with my AWS account. Any help in testing the changes is appreciated.

massi-ang

On top of my comments on the changes, I would expect changes to the front end. If cross encoder models are not enabled the the menu should not be displayed.
Also, if cross encoder models are not selected, it should not be possible to enable hybrid search on the workspaces.

cli/magic-create.ts

lib/shared/layers/python-sdk/python/genai_core/aurora/query.py

lib/shared/layers/python-sdk/python/genai_core/opensearch/query.py

azaylamba · 2024-01-25T08:58:56Z

@massi-ang It seems syncing with main has caused some unwanted changes, as the original changes were made prior to AppSync migration. I will work on this.

…tbot

Hybrid Search won't be available if cross encoding is not enabled.

azaylamba · 2024-02-04T16:33:45Z

@massi-ang I have addressed the review comments, please have a look.

massi-ang

Can you explain why there are 2 settings (Cross Encoder / Embeddings) , since if you enable embeddings models via SM, you can get Cross Encoder for free.

cli/magic-create.ts

azaylamba · 2024-02-05T17:54:46Z

Can you explain why there are 2 settings (Cross Encoder / Embeddings) , since if you enable embeddings models via SM, you can get Cross Encoder for free.

@massi-ang I understand your point that if we enable embeddings models via SM, we can get cross encoder for free. I kept the settings separate to have more control for cross encoding and to make the intent clear in the backend code. Excluding execution of cross encoding code based on sagemaker models can be a little confusing there.
I think having two separate settings won't harm.

Please let me know if I am missing something.

massi-ang · 2024-02-05T18:28:51Z

I think the configuration should be simple and meaningful for the user not the backend. You could think of an alternative naming for the parameter if you think that could create confusion, or better, you could create a function called is_cross_encoding_enabled and use that in all your backend logic. In this way your code is self describing. In the function you can add a comment explaining why enabling embeddings is equivalent to enabling the cross encoder (with the current implementation). This approach provides an easy way to make the code evolve in the future.

…iaSagemaker config.

azaylamba · 2024-02-09T18:40:08Z

@massi-ang I have removed the prompt for crossEncodingEnabled and now it is being driven from the config enableEmbeddingModelsViaSagemaker. The reason I still kept crossEncodingEnabled in the config is because similar parameter (crossEncodersEnabled) is already being used in existing code.

azaylamba · 2024-02-21T17:07:43Z

@massi-ang Would you be able to have a look at this again?

Default embeddings models was not being set correctly. Also error was thrown related to suppression rules if sagemaker models were not enabled. Used props.config.llms.enableSagemakerModels config to add the NAG suppression rules.

azaylamba · 2024-04-13T06:28:55Z

@massi-ang Please let me know if more changes are required on this one.

toeteuf · 2024-07-11T21:15:08Z

@massi-ang I wanted to follow up on this PR submitted by @azaylamba that is still pending review. Your feedback and approval are crucial for us, we also would like this feature. Regards

massi-ang · 2024-07-12T07:55:19Z

Hi @azaylamba,
Please look at my comments and fix accordingly. Thanks.

massi-ang

Good work but few fixes left

massi-ang · 2024-02-26T09:42:47Z

cli/magic-config.ts

@@ -328,7 +341,7 @@ async function processCreateOptions(options: any): Promise<void> {
      choices: embeddingModels.map((m) => ({ name: m.name, value: m })),
      initial: options.defaultEmbedding || undefined,
      skip(): boolean {
-        return !(this as any).state.answers.enableRag;


why this change?

Hi @azaylamba, are you able to address this comment?

massi-ang · 2024-07-12T07:30:08Z

lib/rag-engines/index.ts

-      props.config.rag.engines.aurora.enabled ||
-      props.config.rag.engines.opensearch.enabled
-    ) {
+    if (props.config.llms.enableSagemakerModels) {


This should be checking crossEncodingEnabled and not enableSageMakerModels

ok, but won't that be confusing that crossEncodingEnabled is driving the Sagemaker models instead of the config props.config.llms.enableSagemakerModels which is specific for sagemaker models?

You are right and props.config.llms.enableSagemakerModels is better

lib/user-interface/react-app/src/pages/rag/create-workspace/aurora-form.tsx

lib/user-interface/react-app/src/pages/rag/create-workspace/create-workspace-aurora.tsx

lib/user-interface/react-app/src/pages/rag/create-workspace/opensearch-form.tsx

cli/magic-config.ts

massi-ang · 2024-07-12T07:54:38Z

cli/magic-config.ts

+    };
+  }
+  if (!config.rag.enableEmbeddingModelsViaSagemaker) {
+    config.rag.embeddingsModels = embeddingModels.filter(model => model.provider !== "sagemaker");


This logic should also be applied to the list of models shown in the UI when selecting the default embedding model.

azaylamba · 2024-07-13T14:30:09Z

@massi-ang Addressed the review comments, please have a look.

cli/magic-config.ts

lib/user-interface/react-app/src/common/types.ts

Also removed duplicated config.

massi-ang

One last change and it seems to be all good.

massi-ang · 2024-07-16T07:20:08Z

lib/rag-engines/index.ts

-      props.config.rag.engines.aurora.enabled ||
-      props.config.rag.engines.opensearch.enabled
-    ) {
+    if (props.config.rag.crossEncodingEnabled) {


You were right, props.config.llms.enableSagemakerModels is better.

Updated the condition.

…tbot

azaylamba · 2024-07-18T15:54:11Z

@massi-ang Addressed the last review comment, please have a look.

azaylamba added 3 commits December 23, 2023 20:03

Revert "Enhancement: Add user feedback for responses"

c8dc554

This reverts commit 1c3b8ce.

bigadsoleiman requested a review from spugachev January 9, 2024 02:45

bigadsoleiman approved these changes Jan 17, 2024

View reviewed changes

Merge branch 'main' into main

550d2d0

Merge branch 'aws-samples:main' into main

8dd11d8

massi-ang requested changes Jan 25, 2024

View reviewed changes

azaylamba mentioned this pull request Jan 29, 2024

Feature request: Option to disable cross encoder models #222

Open

azaylamba closed this Feb 4, 2024

azaylamba force-pushed the main branch from 8dd11d8 to 1079079 Compare February 4, 2024 15:58

azaylamba added 2 commits February 4, 2024 21:31

Merge branch 'main' of https://github.com/azaylamba/aws-genai-llm-cha…

42c6edd

…tbot

Addressed review comments related to cross encoding.

efb1a99

Hybrid Search won't be available if cross encoding is not enabled.

azaylamba reopened this Feb 4, 2024

Removed prompt for selecting embedding models as it is not required now.

b58737d

massi-ang requested changes Feb 5, 2024

View reviewed changes

cli/magic-create.ts Outdated Show resolved Hide resolved

cli/magic-create.ts Outdated Show resolved Hide resolved

azaylamba added 4 commits February 9, 2024 17:03

Resolving merge conflicts

cb8793d

Resolving merge conflicts

cf0dfc1

Derived value of crossEncodingEnabled based on enableEmbeddingModelsV…

13ce71e

…iaSagemaker config.

Reverted unwanted change

2522839

Merge branch 'main' into main

4669419

azaylamba and others added 4 commits February 24, 2024 12:58

Merge branch 'main' into main

1667e9c

Default embeddings model prompt was not set

1102491

Default embeddings models was not being set correctly. Also error was thrown related to suppression rules if sagemaker models were not enabled. Used props.config.llms.enableSagemakerModels config to add the NAG suppression rules.

Merge branch 'main' into main

2047641

Merge branch 'main' into main

a09713e

Corrected the NagSuppression conditions

dca47d0

massi-ang requested changes Jul 12, 2024

View reviewed changes

azaylamba added 2 commits July 13, 2024 18:30

Merge branch 'main' into main

c2eabf4

Addressed review comments

6a7c92b

massi-ang requested changes Jul 15, 2024

View reviewed changes

cli/magic-config.ts Outdated Show resolved Hide resolved

lib/user-interface/react-app/src/common/types.ts Outdated Show resolved Hide resolved

Added default value for cross encoder models

494f3b1

Also removed duplicated config.

massi-ang requested changes Jul 16, 2024

View reviewed changes

azaylamba added 3 commits July 18, 2024 21:20

Merge branch 'main' into main

efa9fa8

Used enableSagemakerModels config for SM models

61b73d2

Merge branch 'main' of https://github.com/azaylamba/aws-genai-llm-cha…

feb5752

…tbot

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature request: Option to disable cross encoder models #286

Feature request: Option to disable cross encoder models #286

Feature request: Option to disable cross encoder models #286

Are you sure you want to change the base?

Feature request: Option to disable cross encoder models #286

Conversation

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

Choose a reason for hiding this comment