-
Notifications
You must be signed in to change notification settings - Fork 7
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
initial version of a sdk issue triage bot #264
Conversation
Sample output from a --dry-run against a recent issue:
|
PR HealthBreaking changes ✔️Details
Changelog Entry ✔️Details
Changes to files need to be accounted for in their respective changelogs. Coverage ✔️Details
This check for test coverage is informational (issues shown here will not fail the PR). API leaks ✔️DetailsThe following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
License Headers ✔️Details
All source files should start with a license header. Package publish validation ✔️Details
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
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.
LGTM, just some nits and ideas.
|
||
Here are the descriptions of the different triage areas: | ||
|
||
area-analyzer: Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. |
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.
Does it make sense to give an even longer prompt including examples?
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.
So, two things:
The area descriptions here are taken directly from the github label descriptions. I see value in keeping the label descriptions as the source of truth - they can also help serve as docs for manual triage. We're limited to 100 chars for those descriptions however.
We tune against the most recent ~800 issues. That tuning effectively serves the same purpose as feeding lots of examples into this prompt.
I think however that we should feel free to expand this prompt. Perhaps that means including additional triage instructions (some per-area)? That could be on top of the above area-
label descriptions.
Future<List<String>> classify(String prompt); | ||
} | ||
|
||
class GeminiServiceImpl implements GeminiService { |
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.
[nit] I don't think we're gaining any utility from splitting into an interface and Impl
- can we combine these for now?
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.
This separation help make clear what the mocks should implement / the boundaries of the interface. Were you thinking of something like having the mocks just implement the implementations directly?
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.
Yes - having test doubles use the interface from the implementations is the common pattern. The impl should be able to keep any non-interface members private, so from the outside the interface and test doubles won't be any different.
Thanks to the Dart paradigm of all-classes-are-interfaces (by default at least)*Impl
classes are typically only used in Dart for cases where the impl needs heavyweight dependencies that a modular build can elide when only the interface is required.
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.
OK - I did remove the extra interfaces. The code is a bit simpler (and I was able to hide the non public interface portions of the implementation classes w/ private fields).
comment += '> $summary\n'; | ||
|
||
// create github comment | ||
await githubService.createComment(sdkSlug, issueNumber, comment); |
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.
Do you ever find that the model isn't able to add value beyond repeating/restating the issue input, or does it seem like it's always useful to get a summary added? I'm wondering if we'll want to find a way to let the model choose not to summarize issues that are already short.
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.
The summary is mostly better than the user-authored title, and a very compact tldr for the issue; reading it is quicker than reading the first comment.
If after using this for a while we decide the summary's not more valuable than the noise it adds, we can remove the summary, choose to only apply it to some issues, ...
required GithubService githubService, | ||
required GeminiService geminiService, | ||
}) async { | ||
print('Triaging $sdkSlug...'); |
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 we move to productionize this and use it consistently in the long term this we'll probably want to take out the print
in favor of a more log driven approach. I don't think we should block on that for now.
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.
👍
Co-authored-by: Nate Bosch <nbosch@google.com>
Revisions updated by `dart tools/rev_sdk_deps.dart`. collection (https://github.com/dart-lang/collection/compare/586a5e8..141d83a): 141d83a 2024-06-07 Graciliano Monteiro Passos `CanonicalizedMap`: added constructor `fromEntries`. (dart-archive/collection#347) convert (https://github.com/dart-lang/convert/compare/302af1b..70940e3): 70940e3 2024-06-10 Alex Li ⚡ Upper-cast the return type of the decoder (dart-archive/convert#99) dartdoc (https://github.com/dart-lang/dartdoc/compare/ddb8fb4..3decf1e): 3decf1ed 2024-06-07 Sam Rawlins Restrict validation of runtime renderer files to a single, target version of (dart-lang/dartdoc#3778) ecosystem (https://github.com/dart-lang/ecosystem/compare/bc25c0c..865b2c5): 865b2c5 2024-06-07 Devon Carew update docs and metadata for package:sdk_triage_bot (dart-lang/ecosystem#266) 268b516 2024-06-06 Devon Carew initial version of a sdk issue triage bot (dart-lang/ecosystem#264) http (https://github.com/dart-lang/http/compare/a3567f8..b522000): b522000 2024-06-08 Anikate De fix inconsistent test server behavior (dart-lang/http#1227) mime (https://github.com/dart-lang/mime/compare/8d2d559..4ca2f5e): 4ca2f5e 2024-06-10 Sarah Zakarias Add `topics` to `pubspec.yaml` (dart-archive/mime#126) Change-Id: I6b6279a4ff0af5ba19cc3c4180389b949f48d623 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/370660 Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Commit-Queue: Devon Carew <devoncarew@google.com>
Initial version of a sdk issue triage bot:
dart bin/triage.dart <issue>
area-vm
,area-analyzer
, ...)This is not quite yet ready to roll out. We are able to tune a model on ~800 existing triaged issues (see
tool/create_tuning_data.dart
), but are not able to call that tuned model via the current gemini apis.Contribution guidelines:
dart format
.Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.