-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Enabling streaming in BaseSQLTableQueryEngine #13599
Enabling streaming in BaseSQLTableQueryEngine #13599
Conversation
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.
Thanks @dhirajsuvarna -- I've left a couple of comments.
super().__init__( | ||
callback_manager=callback_manager | ||
or callback_manager_from_settings_or_context(Settings, service_context), | ||
**kwargs, | ||
# **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.
why are we commenting out **kwargs here?
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.
BaseQueryEngine
doesn't take any kwargs
so need to comment this, else i get the below error
BaseQueryEngine.__init__() got an unexpected keyword argument 'streaming'
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.
Right. So if we do what the other comment proposes then this should no longer be a problem then, right?
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.
yup, right
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.
but shouldn't the kwargs
be removed, if it is not used by the base class (BaseQueryEngine
)
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.
that's fair! In this case, let's just remove it as opposed to just commenting out.
@@ -351,10 +351,11 @@ def __init__( | |||
|
|||
self._synthesize_response = synthesize_response | |||
self._verbose = verbose | |||
self._streaming = kwargs["streaming"] if "streaming" in kwargs else False |
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 it might be better to include streaming
as a param in __init__()
instead and give it a default value of False
(i.e. treat it like verbose
).
CC: @logan-markewich
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 agree, I would go with that 👍🏻
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.
ok, i have done this change, please review
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.
Thanks!
@dhirajsuvarna can you kindly run |
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.
Lgtm!
By the way I ran this command, but this kind of introduced around 800+ changes in the files, mostly likely related to newline. Do know what's going on, but would like to understand it. Any tips is highly appreiciated. |
@dhirajsuvarna could be the way your IDE is configuring newlines |
Description
Was using
NLSQLTableQueryEngine
to build a Text to SQL chatbot and found that this particular Query Engine was not able to stream the response.On analysis found that the
streaming
flag could be passed to the ResponseSynthesizer but was not being handled in theBaseSQLTableQueryEngine
Fixes # (issue)
Didn't find any issue reported in Github Issues
Type of Change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
I made changes in the code locally and tested it in my local enviornment
Suggested Checklist:
make format; make lint
to appease the lint gods