-
Notifications
You must be signed in to change notification settings - Fork 22
LSP reduce no of candidates #729
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
Conversation
PR Reviewer Guide 🔍(Review updated until commit 65a5b6a)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 65a5b6a
Previous suggestionsSuggestions up to commit 65a5b6a
|
|
Persistent review updated to latest commit 65a5b6a |
| # LSP-specific | ||
| N_CANDIDATES_LSP = 3 | ||
| N_TESTS_TO_GENERATE_LSP = 1 | ||
| TOTAL_LOOPING_TIME_LSP = 5.0 |
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.
we should not decrease this, as this can increase performance noise
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.
we should also enable line profiling but reduce the number of candidates for it
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.
we should also enable line profiling but reduce the number of candidates for it
We never disabled it for VSC, the base profiling and optimization candidate profiling all works same as cli. Yeah we can check on reduction of count though.
|
|
||
| # LSP-specific | ||
| N_CANDIDATES_LSP = 3 | ||
| N_TESTS_TO_GENERATE_LSP = 1 |
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.
lets keep it 2
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.
lets move the line profiler candidate count to cli
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.
As discussed with @mohammedahmed18 N_candidates are hardcoded in aiservices currently. so need to make it parameterised with max value.
optimization_response_items, llm_cost = await optimize_python_code(
request.user, ctx, data.dependency_code, n=5, python_version=python_version
)
```
|
saga4 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
PR Type
Enhancement
Description
Add LSP-aware dynamic config getters
Reduce LSP candidates, tests, runtime
Replace constants with getters across code
Improve profiling step logging
Diagram Walkthrough
File Walkthrough
config_consts.py
Add LSP-specific constants and dynamic getterscodeflash/code_utils/config_consts.py
git_utils.py
Use dynamic candidate count for worktreescodeflash/code_utils/git_utils.py
function_optimizer.py
Dynamic tests/candidates/time; better profiling logscodeflash/optimization/function_optimizer.py
test_runner.py
Test runners use dynamic total runtimecodeflash/verification/test_runner.py