-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(runners): Add get_session_config property to RunConfig #3662
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?
feat(runners): Add get_session_config property to RunConfig #3662
Conversation
|
Hi @nimanthadilz ,Thank you for your contribution! We appreciate you taking the time to submit this pull request. Your PR has been received by the team and is currently under review. We will provide feedback as soon as we have an update to share. |
|
Hi @wyf7107 , can you please review this |
| app_name=self.app_name, | ||
| user_id=user_id, | ||
| session_id=session_id, | ||
| config=run_config.get_session_config if run_config else 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.
To maintain the same logic, I recommend adding this code at the beginning of the run_debug method and removing the ternary check from the config parameter assignment
run_config = run_config or RunConfig()
| ) -> None: | ||
| """Rewinds the session to before the specified invocation.""" | ||
| session = await self.session_service.get_session( | ||
| app_name=self.app_name, user_id=user_id, session_id=session_id |
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.
To maintain the same logic, I recommend passing the config parameter to the get_session method.
- Update the
rewind_asyncsignature to include:run_config: Optional[RunConfig] = None. - Initialize the config before the
get_sessioncall:run_config = run_config or RunConfig() - Pass the config in the
get_sessioncall:config=run_config.get_session_config
| # Check both calls had config=None | ||
| for call in self.mock_session_service.get_session.call_args_list: | ||
| assert call.kwargs["config"] is 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.
I recommend adding unit tests for rewind_async and run_live methods
|
Hi @nimanthadilz , Thank you for your patience here. I apologize for the delay in getting to this review; I know this has been sitting for a while. This PR has merge conflicts that require changes from your end. Could you please rebase your branch with the latest main branch to address these? Once this is complete, please let us know so we can proceed with the review. |
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
Problem:
Currenlty, there is no way to limit the number of events fetched from the session store when using
Runner.run_asyncmethod. In some cases, this could cause to fetch a large num. of events from the session store.Solution:
The
BaseSessionService.get_sessionmethod accepts aconfigparameter of typeGetSessionConfig. But this is not used insideRunner.run_async. This PR updatesRunConfigto include an optional field to pass aGetSessionConfigobject and use it inrun_async,run_debug&run_live.Testing Plan
Added new unit tests to verify that new optional field is properly passed down when calling
get_sessionfromRunner.Unit Tests:
Please include a summary of passed

pytestresults.Manual End-to-End (E2E) Tests:
SqliteSessionServiceas the session service.Runner.run_async, pass aRunConfigincluding aget_session_config.aiosqlitepackage.LIMITclause matching the config you passed.If you want a quick setup to test this change, I have a GitHub repo here.
Checklist