Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DERCBOT-1267] Gen AI - Document Compressor #1788

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

assouktim
Copy link
Contributor

@assouktim assouktim commented Nov 18, 2024

This pull request adds support for a new Document Compressor feature in the bot administration service. The changes include adding new data models, services, and API endpoints to manage the Document Compressor configuration. Additionally, some existing code has been updated to integrate this new functionality.

Document Compressor Feature:

Other Changes:

  • bot/connector-rest-client/src/main/kotlin/model/ClientFootnote.kt and bot/connector-web-model/src/main/kotlin/ai/tock/bot/connector/web/send/Footnote.kt: Added a new score field to the Footnote data class. [1] [2]
  • bot/connector-web/src/main/kotlin/WebMessageProcessor.kt and bot/connector-web/src/test/kotlin/WebConnectorResponseTest.kt: Updated the WebMessageProcessor and its test to handle the new score field in Footnote. [1] [2]
  • bot/engine/src/main/kotlin/admin/bot/compressor/BotDocumentCompressorConfiguration.kt and bot/engine/src/main/kotlin/admin/bot/compressor/BotDocumentCompressorConfigurationDAO.kt: Added new data models and DAO interfaces for the Document Compressor configuration. [1] [2]
  • bot/engine/src/main/kotlin/definition/BotDefinition.kt and bot/engine/src/main/kotlin/definition/BotDefinitionBase.kt: Updated the BotDefinition and BotDefinitionBase to include the Document Compressor configuration. [1] [2] [3] [4]
  • bot/engine/src/main/kotlin/engine/BotRepository.kt: Added import for BotDocumentCompressorConfigurationMonitor.

Copy link
Member

@Benvii Benvii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding all the check document compressor settings to the orchestrator 👍️, juste a small comment about an exception raising that could be improved.

We begin to have a lot of Gen AI related configuration directly on the Bot Definition, I'm not sure but in a futur refactory I might be a good option to group them especially if we want to expose the to python scripted stories (tock-py) and also kotlin scripted stories. It would be great to have access to some BotDefinition configurations inside buildin sotries (stories behind the Bot API). Maybe we should create an issue about that.
My idea is in a near futur to expose Gen AI settings to build in stories (especially python, tock-py ones) so that python developpers could easily build there own langchain chains using the bot settings and the gen-ai-orchestrator factories provided in a separated python package.

We also have a lot of gen ai related configuration endpoint directly on the bot admin verticle we should in the futur group them in a different verticle.

@@ -6,6 +6,7 @@
<option name="PARENT_ENVS" value="true" />
<envs>
<env name="PYTHONUNBUFFERED" value="1" />
<env name="REQUESTS_CA_BUNDLE" value="/etc/ssl/certs/ca-certificates.crt" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be committed here, put it in a dot env file instead.

cause=f'Response: {response.text}, Reason: {response.reason}',
request=f'[POST] {url}',
))
except Exception as exc:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you will catch the already formatted exception inside the try block at L80 as GenAIDocumentCompressorErrorException inherits from Exception. This will lead to a wrapping of the exception inside an other GenAIDocumentCompressorErrorException object which is a bit strange and will make it difficult to handle it.

Instead you can do the following :

except GenAIDocumentCompressorErrorException:
    # Re-raise GenAIDocumentCompressorErrorException without modification
    raise
except Exception as exc:
    # Catch all other exceptions
    logger.error(f'Unknown error ! {exc}')
    raise GenAIDocumentCompressorErrorException(ErrorInfo(
        error=exc.__class__.__name__,
        cause=str(exc),
        request=f'[POST] {url}',
    ))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, absolutely

class BaseDocumentCompressorSetting(BaseModel):
provider: DocumentCompressorProvider = Field(
description='The document compressor provider.',
examples=[DocumentCompressorProvider.BLOOMZ],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit late for this comment but I think that all document compressor will have max_documents and min_score but we only have one kind of compressor for now so can't be sure this refactoring could still be done in the futur if we add other kind of compressors.

package ai.tock.genai.orchestratorcore.models.compressor

data class BloomzDocumentCompressorSetting(
val maxDocuments: Int,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's optional on the orchestrator side but we can make it required at the bot api / bot admin level. Same for label.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is already required for Tock Studio, botApi and botAdmin

@@ -35,6 +37,7 @@ object Constants {

const val GEN_AI_COMPLETION_SENTENCE_GENERATION="$GEN_AI_COMPLETION/sentenceGeneration"

const val GEN_AI_COMPRESSION="$GEN_AI/COMPRESSION"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it used somewhere ? Or is it for futur use in case we have a compressor with secrets ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I'm going to delete it, it's not in use now.

@@ -26,6 +26,8 @@ object Constants {
const val OPEN_SEARCH = "OpenSearch"
const val PG_VECTOR = "PGVector"

const val BLOOMZ = "BloomzRerank"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bloomz can also be used a retriever / embedding model and also as guardrail model, this contant should have a more explicit name indicating that this is the compressor type.
Please don't apply the suggestion do a refactoring using an IDE.

Suggested change
const val BLOOMZ = "BloomzRerank"
const val BLOOMZ_COMPRESSOR = "BloomzRerank"

@assouktim assouktim force-pushed the feature/dercbot-1267 branch from 84e3c63 to ea378c3 Compare January 10, 2025 14:41
@assouktim
Copy link
Contributor Author

Thanks for adding all the check document compressor settings to the orchestrator 👍️, juste a small comment about an exception raising that could be improved.

We begin to have a lot of Gen AI related configuration directly on the Bot Definition, I'm not sure but in a futur refactory I might be a good option to group them especially if we want to expose the to python scripted stories (tock-py) and also kotlin scripted stories. It would be great to have access to some BotDefinition configurations inside buildin sotries (stories behind the Bot API). Maybe we should create an issue about that. My idea is in a near futur to expose Gen AI settings to build in stories (especially python, tock-py ones) so that python developpers could easily build there own langchain chains using the bot settings and the gen-ai-orchestrator factories provided in a separated python package.

We also have a lot of gen ai related configuration endpoint directly on the bot admin verticle we should in the futur group them in a different verticle.

Yes, we need to include it in the analysis when we refactor the orchestrator code. We need to put this in a separate ticket.

@assouktim assouktim force-pushed the feature/dercbot-1267 branch from 98d7c19 to c9f9ccb Compare January 10, 2025 15:51
@assouktim assouktim force-pushed the feature/dercbot-1267 branch from c9f9ccb to 1a917bf Compare January 13, 2025 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Review in progress
Development

Successfully merging this pull request may close these issues.

2 participants