Enabling streaming in BaseSQLTableQueryEngine #13599
Conversation
dosubot
bot
added
the
size:XS
| 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.
why are we commenting out **kwargs here?
There was a problem hiding this comment.
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.
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.
but shouldn't the kwargs be removed, if it is not used by the base class (BaseQueryEngine)
There was a problem hiding this comment.
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.
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.
I agree, I would go with that 👍🏻
There was a problem hiding this comment.
ok, i have done this change, please review
|
@dhirajsuvarna can you kindly run |
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
NLSQLTableQueryEngineto 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
streamingflag could be passed to the ResponseSynthesizer but was not being handled in theBaseSQLTableQueryEngineFixes # (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 lintto appease the lint gods