Skip to content

Tweak candidate selection#2486

Merged
starcke merged 6 commits intomainfrom
starcke/candidate-selection
Jun 7, 2023
Merged

Tweak candidate selection#2486
starcke merged 6 commits intomainfrom
starcke/candidate-selection

Conversation

@starcke
Copy link
Copy Markdown
Contributor

@starcke starcke commented Jun 6, 2023 •
edited
Loading

Tweak candidate selection:

  • Do not send methods modeled outside the file as candidates.
  • At most 6 usages.
  • Send -1 for this argument.
  • Do not send only some arguments for candidate or sample.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

starcke added 4 commits June 6, 2023 16:49
@starcke
Tweak candidate selection:
76fb55f 
- At most 6 usages.
- Send -1 for `this` argument.
- Do not send only some arguments for candidate or sample.
@starcke
Fix supported.
895c22e
@starcke
Update tests.
222c0d7
@starcke
Add new test.
e3f192b
@starcke starcke force-pushed the starcke/candidate-selection branch from 8214e9c to e3f192b Compare June 7, 2023 09:24
@starcke starcke marked this pull request as ready for review June 7, 2023 09:27
@starcke starcke requested a review from a team as a code owner June 7, 2023 09:27
koesie10
koesie10 reviewed Jun 7, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

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

My main concern with this PR is that we're not using the correct name for the input. In the most recent versions of CodeQL, the MaD input for this is Argument[this], while we would be sending and saving Argument[-1], which would not work.

Perhaps this should be dealt with in the pre-processing on the LLM side rather than in the extension? I believe methods that are being modeled as Argument[-1] will now simply not work when running queries with them.

Comment thread extensions/ql-vscode/src/data-extensions-editor/auto-model.ts Outdated
Comment thread extensions/ql-vscode/src/data-extensions-editor/auto-model.ts
const samples = [];
for (
let argumentIndex = 0;
let argumentIndex = -1; // Start at -1 which means `this` as in `this.method()`
Copy link
Copy Markdown
Member

@koesie10 koesie10 Jun 7, 2023

Choose a reason for hiding this comment

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

Just something we might need to think about in the future (not something we should fix right now, it's also not supported in the editor itself): Do we need to check whether the this argument even exists? For example, a constructor does not have a this argument, and neither do static methods.

Copy link
Copy Markdown
Contributor Author

@starcke starcke Jun 7, 2023

Choose a reason for hiding this comment

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

Yeah I also thought about that. It probably fits into the discussion we had about what endpoints to model.

@starcke @koesie10
Update extensions/ql-vscode/src/data-extensions-editor/auto-model.ts
e14b4c3 
Co-authored-by: Koen Vlaswinkel <koesie10@users.noreply.github.com>
@starcke
Copy link
Copy Markdown
Contributor Author

starcke commented Jun 7, 2023

My main concern with this PR is that we're not using the correct name for the input. In the most recent versions of CodeQL, the MaD input for this is Argument[this], while we would be sending and saving Argument[-1], which would not work.

Yeah I agree. My plan was to deal with it in the extension by mapping -1 back to Argument[this]. However maybe that is a bit to fiddly and we can just send the right thing (and update turbomodel to use input instead of ArgumentIndex. That is also the way CodeML does in their newest version.

I can update the PR to send Argument[this] (still unconditionally) and then make a PR for turbomodel?

@koesie10
Copy link
Copy Markdown
Member

koesie10 commented Jun 7, 2023

I can update the PR to send Argument[this] (still unconditionally) and then make a PR for turbomodel?

Sounds good, I think that's perfect, then the extension doesn't need to care about the implementation details of the LLM.

@starcke
Copy link
Copy Markdown
Contributor Author

starcke commented Jun 7, 2023

Made the change and have a PR for turbomodel here: https://github.com/github/turbomodel/pull/44

@starcke
Copy link
Copy Markdown
Contributor Author

starcke commented Jun 7, 2023 •
edited
Loading

Merged https://github.com/github/turbomodel/pull/44 and verified that I got an Argument[this] classified as a sink back.

@starcke starcke merged commit 7c47a99 into main Jun 7, 2023
@starcke starcke deleted the starcke/candidate-selection branch June 7, 2023 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@charisk charisk charisk approved these changes

@koesie10 koesie10 koesie10 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@starcke @koesie10 @charisk