feat: make nlu penalty factor setting configurable in UI #956
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA new configurable setting, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Chatbot
participant BlockService
participant SettingsService
participant Database
User->>Chatbot: Sends message
Chatbot->>BlockService: Request block match
BlockService->>SettingsService: getDefaultNluPenaltyFactor()
SettingsService->>Database: Fetch setting
Database-->>SettingsService: Return penalty factor (0.95)
SettingsService-->>BlockService: Return penalty factor
BlockService->>BlockService: Apply penalty factor in NLP scoring
BlockService-->>Chatbot: Return best matching block
Chatbot-->>User: Respond with matched block
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
MohamedAliBouhaouala
requested review from
marrouchi,
medchedli,
abdou6666,
IkbelTalebHssan and
yassinedorbozgithub
MohamedAliBouhaouala
force-pushed
the
feat/make-default-nlu-penalty-factor-configurable
branch
from
d0c04c0 to
d8be661
Compare
MohamedAliBouhaouala
force-pushed
the
feat/make-default-nlu-penalty-factor-configurable
branch
from
d8be661 to
d19874a
Compare
MohamedAliBouhaouala
force-pushed
the
feat/implement-block-nlp-prioritization-strategy
branch
2 times, most recently
from
664dbf9 to
909f7c2
Compare
MohamedAliBouhaouala
force-pushed
the
feat/make-default-nlu-penalty-factor-configurable
branch
2 times, most recently
from
65b443f to
169b767
Compare
api/src/migration/migrations/1735836154221-v-2-2-0.migration.ts
Outdated
Show resolved
Hide resolved
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
api/src/chat/services/block.service.ts (2)
222-224: Improve the error message specificity.The error message should specify that it's referring to the NLU penalty factor.
- throw new Error('Penalty Factor must be between 0 and 1.'); + throw new Error('NLU Penalty Factor must be between 0 and 1.');
483-495: Well-documented method for retrieving the NLU penalty factor.The method is well-documented with JSDoc that clearly explains the purpose of this factor in NLU-based scoring. Consider moving the validation into this method to ensure it always returns a valid value.
async getDefaultNluPenaltyFactor(): Promise<number> { const settings: Settings = await this.settingService.getSettings(); - return settings.chatbot_settings.default_nlu_penalty_factor; + const nluPenaltyFactor = settings.chatbot_settings.default_nlu_penalty_factor; + if (nluPenaltyFactor < 0 || nluPenaltyFactor > 1) { + throw new Error('NLU Penalty Factor must be between 0 and 1.'); + } + return nluPenaltyFactor; }api/src/migration/migrations/1745594957327-v-2-2-6.migration.ts (4)
40-40: Fix typo in success message.There's a typo in the success message.
- logger.log('Successfuly added the default NLU penalty factor setting'); + logger.log('Successfully added the default NLU penalty factor setting');
41-43: Improve error logging.Include the error object in the log for better debugging.
- } catch (err) { - logger.error('Unable to add the default NLU penalty factor setting'); + } catch (err) { + logger.error('Unable to add the default NLU penalty factor setting', err);
53-53: Fix typo in success message.There's a typo in the success message.
- logger.log('Successfuly removed the default NLU penalty factor setting'); + logger.log('Successfully removed the default NLU penalty factor setting');
54-56: Improve error logging.Include the error object in the log for better debugging.
- } catch (err) { - logger.error('Unable to remove the default local storage helper setting'); + } catch (err) { + logger.error('Unable to remove the default NLU penalty factor setting', err);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
api/src/chat/services/block.service.spec.ts(2 hunks)api/src/chat/services/block.service.ts(2 hunks)api/src/migration/migrations/1745594957327-v-2-2-6.migration.ts(1 hunks)api/src/setting/seeds/setting.seed-model.ts(4 hunks)frontend/public/locales/en/chatbot_settings.json(1 hunks)frontend/public/locales/fr/chatbot_settings.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
api/src/chat/services/block.service.spec.ts (1)
api/src/utils/test/mocks/block.ts (1)
mockNlpBlock(287-312)
api/src/migration/migrations/1745594957327-v-2-2-6.migration.ts (1)
api/src/migration/types.ts (1)
MigrationServices(39-43)
api/src/chat/services/block.service.ts (1)
frontend/src/components/settings/index.tsx (1)
Settings(72-218)
🔇 Additional comments (10)
frontend/public/locales/fr/chatbot_settings.json (2)
11-12: LGTM! The new label is properly added to the French localization.The label for the new NLU penalty factor setting has been correctly added to the French localization file.
19-20: Complete and well-explained help text.The help description provides a thorough explanation of the NLU penalty factor, including its purpose, range (0-1), and how it affects chatbot behavior. The translation accurately conveys the technical concept in French.
api/src/setting/seeds/setting.seed-model.ts (2)
41-52: LGTM! Well-structured new setting with appropriate constraints.The new
default_nlu_penalty_factorsetting is properly configured:
- Appropriate default value (0.95)
- Correct type (number)
- Proper constraints (min: 0, max: 1, step: 0.01)
- Reasonable weight value (3)
65-97: LGTM! Weight values properly updated.The weight values for subsequent settings have been correctly incremented to maintain proper ordering after inserting the new setting:
- default_storage_helper: 3 → 4
- global_fallback: 4 → 5
- fallback_block: 5 → 6
- fallback_message: 6 → 7
api/src/chat/services/block.service.spec.ts (2)
357-357: LGTM! Test constant updated to match new default value.The
nlpPenaltyFactorconstant has been correctly updated from the previous hardcoded value to 0.95, matching the new default value defined in the settings.
411-411: LGTM! Test expectation updated to reflect new behavior.The expected block in the test assertion has been updated from
mockModifiedNlpBlocktomockNlpBlock, correctly reflecting the change in block selection behavior with the new penalty factor.frontend/public/locales/en/chatbot_settings.json (2)
11-12: LGTM! New label added to English localization.The label for the new NLU penalty factor setting has been correctly added to the English localization file.
19-20: Clear and comprehensive help description.The help text provides an excellent explanation of the NLU penalty factor, clearly describing:
- The acceptable range (0-1)
- Its purpose in prioritizing specific entity matches over broad ones
- How it affects matching without impacting other matching strategies
api/src/chat/services/block.service.ts (1)
218-224: LGTM - Retrieving NLU penalty factor from settings.This change makes the previously hardcoded NLU penalty factor configurable through global settings, which enhances flexibility and user control.
api/src/migration/migrations/1745594957327-v-2-2-6.migration.ts (1)
16-44: LGTM - Setting up the default NLU penalty factor.The migration properly adds the new configurable setting with appropriate constraints.
api/src/migration/migrations/1745594957327-v-2-2-6.migration.ts
Outdated
Show resolved
Hide resolved
| { | ||
| group: 'chatbot_settings', | ||
| label: 'default_nlu_penalty_factor', | ||
| value: 0.95, | ||
| type: SettingType.number, | ||
| config: { | ||
| min: 0, | ||
| max: 1, | ||
| step: 0.01, | ||
| }, | ||
| weight: 3, | ||
| }, |
There was a problem hiding this comment.
Do the other chatbot_settings weights need migration?
This might lead to inconsistencies in the UI.
There was a problem hiding this comment.
I don't think so. We didn't migrate weights when we added a default storage helper in global settings.
MohamedAliBouhaouala
force-pushed
the
feat/make-default-nlu-penalty-factor-configurable
branch
from
5727e1b to
27be2c6
Compare
MohamedAliBouhaouala
force-pushed
the
feat/make-default-nlu-penalty-factor-configurable
branch
from
b7f241c to
1c39413
Compare
MohamedAliBouhaouala
force-pushed
the
feat/implement-block-nlp-prioritization-strategy
branch
from
6686e3e to
a8666ce
Compare
…os in log messages
MohamedAliBouhaouala
force-pushed
the
feat/make-default-nlu-penalty-factor-configurable
branch
from
1c39413 to
e3ce245
Compare
| // Log the matched patterns | ||
| this.logger.debug( | ||
| `Matched patterns: ${JSON.stringify(matchesWithPatterns.map((p) => p.matchedPattern))}`, | ||
| ); | ||
|
|
||
| // Retrieve Nlu Penalty Factor from global settings | ||
| const nluPenaltyFactor: number = |
There was a problem hiding this comment.
I think we should remove the typing since it can be inferred by typescript.
| @@ -354,7 +354,7 @@ describe('BlockService', () => { | |||
| }); | |||
|
|
|||
| describe('matchBestNLP', () => { | |||
| const nlpPenaltyFactor = 2; | |||
| const nlpPenaltyFactor = 0.95; | |||
There was a problem hiding this comment.
I think we should re-use the constant declared in nlp.ts
MohamedAliBouhaouala
changed the base branch from
feat/implement-block-nlp-prioritization-strategy
to
feat/implement-block-nlp-prioritization-strategy-v2
marrouchi
force-pushed
the
feat/implement-block-nlp-prioritization-strategy-v2
branch
from
2465ae1 to
9642e82
Compare
yassinedorbozgithub
force-pushed
the
feat/implement-block-nlp-prioritization-strategy-v2
branch
from
9642e82 to
2465ae1
Compare
medchedli
changed the base branch from
feat/implement-block-nlp-prioritization-strategy-v2
to
main
Motivation
The following PR PR introduces support for configuring the NLU Penalty Factor via the UI, allowing chatbot builders to control how entity match specificity influences block selection.
Key changes
default_nlu_penalty_factorto the chatbot settings section in the UI.Fixes # (issue)
Type of change:
Please delete options that are not relevant.
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Chores