-
Notifications
You must be signed in to change notification settings - Fork 58
OLS-2092 Byok log enhancements #2664
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
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3035f96 to
6775874
Compare
|
/retest |
5 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
e9c6284 to
b559dd8
Compare
|
/retest |
ols/utils/token_handler.py
Outdated
|
|
||
| available_tokens = min(tokens_count, max_tokens) | ||
| logger.debug("Available tokens: %d.", tokens_count) | ||
| logger.debug("Available tokens: %d.", available_tokens) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I supose this should show the total number of available tokens in the context window, but it is not showing that:
2025-11-19 08:03:01,970 [ols.utils.token_handler:token_handler.py:152] DEBUG: RAG content tokens count: 345.
2025-11-19 08:03:01,970 [ols.utils.token_handler:token_handler.py:155] DEBUG: Available tokens: 345.
2025-11-19 08:03:01,970 [ols.utils.token_handler:token_handler.py:169] INFO: Document #2 selected: title='Preparing your cluster for OpenShift Virtualization', url='https://docs.openshift.com/container-platform/4.17/virt/install/preparing-cluster-for-virt.html', index='default', score=0.5925, tokens=345
2025-11-19 08:03:01,971 [ols.utils.token_handler:token_handler.py:152] DEBUG: RAG content tokens count: 358.
2025-11-19 08:03:01,971 [ols.utils.token_handler:token_handler.py:155] DEBUG: Available tokens: 358.
2025-11-19 08:03:01,971 [ols.utils.token_handler:token_handler.py:169] INFO: Document #3 selected: title='Installing a cluster on any platform', url='https://docs.openshift.com/container-platform/4.17/installing/installing_platform_agnostic/installing-platform-agnostic.html', index='default', score=0.5889, tokens=358
It is showing the same number twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sriroopar
It still shows the wrong number as available tokens
2025-11-24 08:30:07,444 [ols.utils.token_handler:token_handler.py:124] INFO: Processing 5 retrieved nodes for RAG context
2025-11-24 08:30:07,444 [ols.utils.token_handler:token_handler.py:152] DEBUG: RAG content tokens count: 103.
2025-11-24 08:30:07,444 [ols.utils.token_handler:token_handler.py:155] DEBUG: Available tokens: 103.
Also, please remove the . after the digit.
| """Initialize custom query fusion class.""" | ||
| # Extract custom parameters before passing to parent | ||
| retriever_weights = kwargs.pop("retriever_weights", None) | ||
| index_configs = kwargs.pop("index_configs", None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why pop instead of get? Do you need to modify input kwargs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assumed that there params will be considered into the parent class, but after checking that's not the case. Updating it to be .get()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the pop is required here, so that these custom params are not plugged into the parent class. It's also causing e2e and eval to fail, fixing it now!
91f3def to
350685c
Compare
350685c to
4480685
Compare
#Reverting the storing changes
4480685 to
36d0d5c
Compare
|
/retest |
4 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
@sriroopar: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Description
Added info logs to display information on document retrieved, indices, rank, score and the final document that's picked based on the metrics.
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Please provide detailed steps to perform tests related to this code change.
How were the fix/results from this change verified? Please provide relevant screenshots or results.
Verified by running RAG test suite and logs were checked.