Skip to content
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

feature: added new cody receipe optimize #51974

Merged
merged 12 commits into from
May 26, 2023

Conversation

Momilijaz96
Copy link
Contributor

Test plan

Tested the feature locally by running Cody from "Run and Debug" option of VSCode; with different DSA algorithms in python and general DS/ML based applications and projects. The recipe suggests good code optimizationsto improve memory and time optimizations.

@cla-bot
Copy link

cla-bot bot commented May 16, 2023

We require contributors to sign our Contributor License Agreement (CLA), and we don't have yours on file. In order for us to review and merge your code, please sign CLA to get yourself added.

Sourcegraph teammates should refer to Accepting contributions for guidance.

@cla-bot
Copy link

cla-bot bot commented May 16, 2023

We require contributors to sign our Contributor License Agreement (CLA), and we don't have yours on file. In order for us to review and merge your code, please sign CLA to get yourself added.

Sourcegraph teammates should refer to Accepting contributions for guidance.

@Momilijaz96
Copy link
Contributor Author

@cla-bot check

@cla-bot cla-bot bot added the cla-signed label May 16, 2023
@cla-bot
Copy link

cla-bot bot commented May 16, 2023

The GitHub CLA Bot is rechecking to see that you have signed the CLA - note that it may take up to 30 minutes for your response to be synchronized.

@Momilijaz96
Copy link
Contributor Author

@cla-bot check

@cla-bot
Copy link

cla-bot bot commented May 18, 2023

The GitHub CLA Bot is rechecking to see that you have signed the CLA - note that it may take up to 30 minutes for your response to be synchronized.

@mrnugget mrnugget requested a review from a team May 19, 2023 08:17
Copy link
Contributor

@mrnugget mrnugget left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @Momilijaz96! Very interesting. Do you have examples of how it worked? Did you test it on some code and got good results?

There's some linting issues, according to CI:



> @ format:check /root/buildkite/build/sourcegraph
--
  | > prettier '**/{*.{js?(on),ts?(x),graphql,md,scss},.*.js?(on)}' --config prettier.config.js --check --write=false
  |  
  | Checking formatting...
  | [warn] client/cody-shared/src/chat/recipes/vscode-recipes.ts
  | [warn] client/cody/src/main.ts
  | [warn] Code style issues found in 2 files. Forgot to run Prettier?

Those need to be fixed

new GenerateDocstring(),
new GenerateTest(),
new GitHistory(),
new ImproveVariableNames(),
new InlineChat(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the reorder?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the reordering, I guess that was extra.

Copy link
Contributor

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really interesting. I feel like we should have a recipe regarding time and space complexity!! Thank you so much for this.

I would also love to see some examples as @mrnugget suggested

@Momilijaz96
Copy link
Contributor Author

Thanks for the PR, @Momilijaz96! Very interesting. Do you have examples of how it worked? Did you test it on some code and got good results?

There's some linting issues, according to CI:



> @ format:check /root/buildkite/build/sourcegraph
--
  | > prettier '**/{*.{js?(on),ts?(x),graphql,md,scss},.*.js?(on)}' --config prettier.config.js --check --write=false
  |  
  | Checking formatting...
  | [warn] client/cody-shared/src/chat/recipes/vscode-recipes.ts
  | [warn] client/cody/src/main.ts
  | [warn] Code style issues found in 2 files. Forgot to run Prettier?

Those need to be fixed

I installed prettier and ran it manually by prettier --write <file-name> for both pointed out files to fix it the formatting.

@Momilijaz96
Copy link
Contributor Author

This is really interesting. I feel like we should have a recipe regarding time and space complexity!! Thank you so much for this.

I would also love to see some examples as @mrnugget suggested

Sure thing!
So I tested it on a couple different codes:
a. Some python data structures and algorithms code [Tree data structure + dynamic programming problems] - Result: Suggest optimization if possible.
b. Yaml and Json files [Docker image and kuberentes manifest files] - Result: No optimization should be suggested
c. Model's predict function -Result: Depends on implementation.
I have attached the result snaps, of the local run here:
docker image
dynamic_prog_result
kubernetes_yaml
Model predict
TreeNode

@Momilijaz96
Copy link
Contributor Author

Momilijaz96 commented May 22, 2023

Overall the models suggest good optimizations for the code but
there are few cases that I could'nt handle, even with iterative prompt development:
a. I could'nt fix the format of the output, I wanted the "optimization steps", "time/space consumption improvement" in text and "updated code block" in a code block, but no changes in the prompt helped me achieve that.
b. I also struggled with the input code not be included in response, but it is included sometimes with current prompt.
c. I tested it on limited file formats and code types.
Let me know what you guys think!
I

@thenamankumar thenamankumar self-requested a review May 22, 2023 21:51
@mrnugget
Copy link
Contributor

Looking at the examples here (#51974 (comment)) it seems like the formatting is broken, no? Every reply is prefixed with "Here is the response" and then it's raw Markdown.

That should be fixed before we merge this

Copy link
Contributor

@mrnugget mrnugget left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment inline and the previous comment further up

Don't include the input code in your response. Beautify the response for better readability.\
Response format should be: This code can/cannot be optimzed. Optimization Steps: {} Time and Space Usage: {} Updated Code: {}\
However if no optimization is possible; just say the code is already optimized. \n\n\`\`\`${extension}\n${truncatedSelectedText}\n\`\`\`\n${MARKDOWN_FORMAT_PROMPT}`
const assistantResponsePrefix = `Here is the response \n\`\`\`${extension}\n`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this breaks the formatting: we don't want every response prefixed with "Here is the response"

Copy link
Contributor Author

@Momilijaz96 Momilijaz96 May 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out... removing the response prefix, fixed the response format.
Check out these examples now:
Screenshot 2023-05-23 at 3 39 23 PM

Screenshot 2023-05-23 at 3 39 54 PM Screenshot 2023-05-23 at 3 40 18 PM

@philipp-spiess
Copy link
Contributor

🎉

@Momilijaz96 do you mind solving the merge conflict? I can look into it as well if you're busy. Ping me here when you did this so I can run the latest changes on CI and we can ship this 🚀

@Momilijaz96
Copy link
Contributor Author

🎉

@Momilijaz96 do you mind solving the merge conflict? I can look into it as well if you're busy. Ping me here when you did this so I can run the latest changes on CI and we can ship this 🚀

Hey @philipp-spiess I don't have the write access to resolve the conflict.
Screenshot 2023-05-24 at 12 06 29 PM

@philipp-spiess
Copy link
Contributor

@Momilijaz96 You should be able to update your forked repository. Try something like this in the terminal. If that doesn't work, I can look into it :)

git remote -v # This should list your fork as the `origin` for this to work. 
git fetch
git merge origin/main

@Momilijaz96
Copy link
Contributor Author

@Momilijaz96 You should be able to update your forked repository. Try something like this in the terminal. If that doesn't work, I can look into it :)

git remote -v # This should list your fork as the `origin` for this to work. 
git fetch
git merge origin/main

Alright, i guess it's ready to be merged.

@@ -0,0 +1,60 @@
import { ChatQuestion } from './chat-question'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh there seems to be a conflict here, this file was moved into the client/cody folder on main

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I accidently merged current branch into main, I "undo" it and pushed again, I guess that would resolve this conflict.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think linting is failing on some files too, I can't see which ones though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file still seems to be added instead. Is it okay if I push onto your branch to try and resolve this??

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah sure, go ahead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently my forked repo's main branch is a few commits ahead of the upstream's main. I am trying to fetch upstream and merge into my fork's main, but that is not introducing any changes.

@Momilijaz96
Copy link
Contributor Author

Hey @philipp-spiess , Thanks for taking care of this. 👍
Is the conflict resolved now? or something else needs be done from my end.

@philipp-spiess
Copy link
Contributor

@Momilijaz96 All done from your end 🎉 I’m just working on getting CI to pass before I can merge this 🙂

@philipp-spiess
Copy link
Contributor

@Momilijaz96 There appears to be a bug in our CI setup right now, I've asked internally for some advice. I'll have this PR bookmarked though and will continue to look into it 🙇

@Momilijaz96
Copy link
Contributor Author

@Momilijaz96 There appears to be a bug in our CI setup right now, I've asked internally for some advice. I'll have this PR bookmarked though and will continue to look into it 🙇

No worries, Sounds good.

@philipp-spiess philipp-spiess merged commit cf26cdc into sourcegraph:main May 26, 2023
5 of 6 checks passed
@philipp-spiess
Copy link
Contributor

Y'all we did it! 🎉 @Momilijaz96 @mrnugget

@Momilijaz96
Copy link
Contributor Author

Momilijaz96 commented May 26, 2023

Y'all we did it! 🎉 @Momilijaz96 @mrnugget

Great 💯

@Momilijaz96 Momilijaz96 deleted the momal-new-cody-recepie branch May 26, 2023 19:24
@jdorfman
Copy link
Member

This is a fantastic addition, great work @Momilijaz96!

And thank you @mrnugget @philipp-spiess for reviewing it.

ErikaRS pushed a commit that referenced this pull request Jun 22, 2023
## Test plan

Tested the feature locally by running Cody from "Run and Debug" option
of VSCode; with different DSA algorithms in python and general DS/ML
based applications and projects. The recipe suggests good code
optimizationsto improve memory and time optimizations.

---------

Co-authored-by: Philipp Spiess <hello@philippspiess.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants