aws-genai-llm-chatbot
aws-genai-llm-chatbot copied to clipboard
aws-samples
Feature request: Option to disable cross encoder models
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.
@azaylamba We have migrated to AppSync. This PR has conflicts. Could you please fix this? And than we can merge
@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 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.
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.
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.
@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.
@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 I think @azaylamba commit the last change requested... Could you please have a look?
Hi @azaylamba , @toeteuf ,
Apologies for the delay.
I am making changes based on your PR to fix/set the list of models in the config instead of adding new properties (and reviewing your change at the same time).
I will most likely create a new PR with these changes and follow up until merged (and mention you are the original author @azaylamba ). I will also verify the unit tests/format is passing verifications.
Please tell me if you have any concern.
Hi @azaylamba , @toeteuf ,
Apologies for the delay.
I am making changes based on your PR to fix/set the list of models in the config instead of adding new properties (and reviewing your change at the same time).
I will most likely create a new PR with these changes and follow up until merged (and mention you are the original author @azaylamba ). I will also verify the unit tests/format is passing verifications.
Please tell me if you have any concern.
Hi @charles-marion I don't have any objection, please proceed with the changes.