internal/llmapp: refactor Overview to avoid unexported parameter
Change-Id: Id12e674c0ab81426f42e8ce48aec9c08b78746da
Reviewed-on: https://go-review.googlesource.com/c/oscar/+/622856
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
diff --git a/internal/gaby/overview.go b/internal/gaby/overview.go
index 7f6d582..e2ae1ec 100644
--- a/internal/gaby/overview.go
+++ b/internal/gaby/overview.go
@@ -67,7 +67,7 @@
overviewForm: form,
Result: &overviewResult{
IssueOverviewResult: *overview,
- OverviewHTML: htmlutil.MarkdownToSafeHTML(overview.Overview),
+ OverviewHTML: htmlutil.MarkdownToSafeHTML(overview.Overview.Overview),
},
}
}
diff --git a/internal/gaby/templates_test.go b/internal/gaby/templates_test.go
index 12e2dd0..a396dcc 100644
--- a/internal/gaby/templates_test.go
+++ b/internal/gaby/templates_test.go
@@ -19,6 +19,7 @@
"golang.org/x/net/html/atom"
"golang.org/x/oscar/internal/actions"
"golang.org/x/oscar/internal/github"
+ "golang.org/x/oscar/internal/llmapp"
"golang.org/x/oscar/internal/search"
)
@@ -40,7 +41,10 @@
IssueOverviewResult: github.IssueOverviewResult{
URL: "https://example.com",
NumComments: 2,
- Overview: "an overview",
+ Overview: &llmapp.OverviewResult{
+ Overview: "an overview",
+ Prompt: []string{"a prompt"},
+ },
},
OverviewHTML: safehtml.HTMLEscaped("an overview"),
}}},
diff --git a/internal/gaby/tmpl/overviewpage.tmpl b/internal/gaby/tmpl/overviewpage.tmpl
index 9016298..800f46d 100644
--- a/internal/gaby/tmpl/overviewpage.tmpl
+++ b/internal/gaby/tmpl/overviewpage.tmpl
@@ -66,7 +66,7 @@
<div class="toggle" onclick="togglePrompt()">[show prompt]</div>
<div id="prompt">
<ul>
- {{- range .Prompt -}}
+ {{- range .Overview.Prompt -}}
<li>
<pre>{{.}}</pre>
</li>
diff --git a/internal/github/overview.go b/internal/github/overview.go
index b1e8bcd..a73a4ee 100644
--- a/internal/github/overview.go
+++ b/internal/github/overview.go
@@ -16,57 +16,45 @@
// It contains the generated overview and metadata about
// the issue.
type IssueOverviewResult struct {
- URL string // the issue's URL
- NumComments int // number of comments for this issue
- Overview string // the LLM-generated issue and comment summary
- Prompt []string // the prompt(s) used to generate the result
+ URL string // the issue's URL
+ NumComments int // number of comments for this issue
+ Overview *llmapp.OverviewResult // the LLM-generated issue and comment summary
}
// IssueOverview returns an LLM-generated overview of the issue and its comments.
// It does not make any requests to GitHub; the issue and comment data must already
// be stored in db.
func IssueOverview(ctx context.Context, g llm.TextGenerator, db storage.DB, project string, issue int64) (*IssueOverviewResult, error) {
- var docs []*llmapp.Doc
- var m issueMetadata
+ var post *llmapp.Doc
+ var comments []*llmapp.Doc
for e := range events(db, project, issue, issue) {
- m.update(e)
- doc := e.toLLMDoc()
+ doc, isIssue := e.toLLMDoc()
if doc == nil {
continue
}
- docs = append(docs, doc)
+ if isIssue {
+ post = doc
+ continue
+ }
+ comments = append(comments, doc)
}
- overview, err := llmapp.Overview(ctx, g, llmapp.PostAndComments, docs...)
+ overview, err := llmapp.PostOverview(ctx, g, post, comments)
if err != nil {
return nil, err
}
return &IssueOverviewResult{
- URL: m.url,
- NumComments: m.numComments,
+ URL: post.URL,
+ NumComments: len(comments),
Overview: overview,
- Prompt: llmapp.OverviewPrompt(llmapp.PostAndComments, docs),
}, nil
}
-type issueMetadata struct {
- url string
- numComments int
-}
-
-// update updates the issueMetadata given the event.
-func (m *issueMetadata) update(e *Event) {
- switch v := e.Typed.(type) {
- case *Issue:
- m.url = v.HTMLURL
- case *IssueComment:
- m.numComments++
- }
-}
-
// toLLMDoc converts an Event to a format that can be used as
// an input to an LLM.
-// It returns nil if the Event cannot be converted to a document.
-func (e *Event) toLLMDoc() *llmapp.Doc {
+// isIssue is true if the event represents a GitHub issue (as opposed to
+// a comment or another event).
+// It returns (nil, false) if the Event cannot be converted to a document.
+func (e *Event) toLLMDoc() (_ *llmapp.Doc, isIssue bool) {
switch v := e.Typed.(type) {
case *Issue:
return &llmapp.Doc{
@@ -75,7 +63,7 @@
Author: v.User.Login,
Title: v.Title,
Text: v.Body,
- }
+ }, true
case *IssueComment:
return &llmapp.Doc{
Type: "issue comment",
@@ -83,7 +71,7 @@
Author: v.User.Login,
// no title
Text: v.Body,
- }
+ }, false
}
- return nil
+ return nil, false
}
diff --git a/internal/github/overview_test.go b/internal/github/overview_test.go
index 137a590..6dc8cd7 100644
--- a/internal/github/overview_test.go
+++ b/internal/github/overview_test.go
@@ -33,13 +33,17 @@
check(c.Add("robpike/ivy"))
check(c.Sync(ctx))
- got, err := IssueOverview(ctx, llm.EchoTextGenerator(), db, "robpike/ivy", 19)
+ echo := llm.EchoTextGenerator()
+
+ got, err := IssueOverview(ctx, echo, db, "robpike/ivy", 19)
if err != nil {
t.Fatal(err)
}
- prompt := llmapp.OverviewPrompt(llmapp.PostAndComments, []*llmapp.Doc{
- {
+ // This merely checks that the correct call to [llmapp.PostOverview] is made.
+ // The internals of [llmapp.PostOverview] are tested in the llmapp package.
+ wantOverview, err := llmapp.PostOverview(ctx, echo,
+ &llmapp.Doc{
Type: "issue",
URL: "https://github.com/robpike/ivy/issues/19",
Author: "xunshicheng",
@@ -50,25 +54,27 @@
could you get me a hand！
`,
},
- {
- Type: "issue comment",
- URL: "https://github.com/robpike/ivy/issues/19#issuecomment-169157303",
- Author: "robpike",
- Text: `See the import comment, or listen to the error message. Ivy uses a custom import.
+ []*llmapp.Doc{
+ {
+ Type: "issue comment",
+ URL: "https://github.com/robpike/ivy/issues/19#issuecomment-169157303",
+ Author: "robpike",
+ Text: `See the import comment, or listen to the error message. Ivy uses a custom import.
go get robpike.io/ivy
It is a fair point though that this should be explained in the README. I will fix that.
`,
- },
- })
- overview := llm.EchoResponse(prompt...)
+ },
+ })
+ if err != nil {
+ t.Fatal(err)
+ }
want := &IssueOverviewResult{
URL: "https://github.com/robpike/ivy/issues/19",
- Overview: overview,
+ Overview: wantOverview,
NumComments: 1,
- Prompt: prompt,
}
if diff := cmp.Diff(got, want); diff != "" {
diff --git a/internal/llmapp/overview.go b/internal/llmapp/overview.go
index f4bc6e0..57e9075 100644
--- a/internal/llmapp/overview.go
+++ b/internal/llmapp/overview.go
@@ -33,24 +33,55 @@
Text string `json:"text"` // required
}
+// OverviewResult is the result of [Overview] or [PostOverview].
+type OverviewResult struct {
+ Overview string // the LLM-generated summary
+ Prompt []string // the prompt(s) used to generate the result
+}
+
// Overview returns an LLM-generated overview of the given documents,
// styled with markdown.
+// Overview returns an error if no documents are provided or the LLM is unable
+// to generate a response.
+func Overview(ctx context.Context, g llm.TextGenerator, docs ...*Doc) (*OverviewResult, error) {
+ return overview(ctx, g, documents, docs)
+}
+
+// PostOverview returns an LLM-generated overview of the given post and comments,
+// styled with markdown.
+// PostOverview returns an error if no post is provided or the LLM is unable to generate a response.
+func PostOverview(ctx context.Context, g llm.TextGenerator, post *Doc, comments []*Doc) (*OverviewResult, error) {
+ if post == nil {
+ return nil, errors.New("llmapp PostOverview: no post")
+ }
+ return overview(ctx, g, postAndComments, append([]*Doc{post}, comments...))
+}
+
+// overview returns an LLM-generated overview of the given documents,
+// styled with markdown.
// The kind argument is a descriptor for the given documents, used to add
// additional context to the LLM prompt.
// Overview returns an error if no documents are provided or the LLM is unable
// to generate a response.
-// TODO(tatianabradley): Chunk large prompts.
-func Overview(ctx context.Context, g llm.TextGenerator, kind docsKind, docs ...*Doc) (string, error) {
+func overview(ctx context.Context, g llm.TextGenerator, kind docsKind, docs []*Doc) (*OverviewResult, error) {
if len(docs) == 0 {
- return "", errors.New("llmapp.Overview: no documents")
+ return nil, errors.New("llmapp overview: no documents")
}
- return g.GenerateText(ctx, OverviewPrompt(kind, docs)...)
+ prompt := overviewPrompt(kind, docs)
+ overview, err := g.GenerateText(ctx, prompt...)
+ if err != nil {
+ return nil, err
+ }
+ return &OverviewResult{
+ Overview: overview,
+ Prompt: prompt,
+ }, nil
}
-// OverviewPrompt converts the given docs into a slice of
+// overviewPrompt converts the given docs into a slice of
// text prompts, followed by an instruction prompt based
// on the documents kind.
-func OverviewPrompt(kind docsKind, docs []*Doc) []string {
+func overviewPrompt(kind docsKind, docs []*Doc) []string {
var inputs = make([]string, len(docs))
for i, d := range docs {
inputs[i] = string(storage.JSON(d))
@@ -59,15 +90,14 @@
}
// docsKind is a descriptor for a group of documents.
-// unexported so that clients must use a predefined kind.
-type docsKind struct{ name string }
+type docsKind string
var (
// Represents a group of documents of an unspecified kind.
- Documents docsKind = docsKind{"documents"}
+ documents docsKind = "documents"
// The documents represent a post and comments/replies
// on that post. For example, a GitHub issue and its comments.
- PostAndComments docsKind = docsKind{"post_and_comments"}
+ postAndComments docsKind = "post_and_comments"
)
//go:embed prompts/*.tmpl
@@ -78,7 +108,7 @@
// document kind.
func (k docsKind) instructions() string {
w := &strings.Builder{}
- err := tmpls.ExecuteTemplate(w, k.name, nil)
+ err := tmpls.ExecuteTemplate(w, string(k), nil)
if err != nil {
// unreachable except bug in this package
panic(err)
diff --git a/internal/llmapp/overview_test.go b/internal/llmapp/overview_test.go
index 2280f0d..9034ecc 100644
--- a/internal/llmapp/overview_test.go
+++ b/internal/llmapp/overview_test.go
@@ -9,6 +9,7 @@
"strings"
"testing"
+ "github.com/google/go-cmp/cmp"
"golang.org/x/oscar/internal/llm"
)
@@ -17,19 +18,45 @@
g := llm.EchoTextGenerator()
d1 := &Doc{URL: "https://example.com", Author: "rsc", Title: "title", Text: "some text"}
d2 := &Doc{Text: "some text 2"}
- got, err := Overview(ctx, g, Documents, d1, d2)
+ got, err := Overview(ctx, g, d1, d2)
if err != nil {
t.Fatal(err)
}
- prompt := Documents.instructions()
- want := llm.EchoResponse(
+ promptParts := []string{
`{"url":"https://example.com","author":"rsc","title":"title","text":"some text"}`,
`{"text":"some text 2"}`,
- prompt)
- if got != want {
- t.Errorf("Overview() = %s, want %s", got, want)
+ documents.instructions(),
}
- t.Log(want)
+ want := &OverviewResult{
+ Overview: llm.EchoResponse(promptParts...),
+ Prompt: promptParts,
+ }
+ if diff := cmp.Diff(got, want); diff != "" {
+ t.Errorf("Overview() mismatch (-got +want):\n%s", diff)
+ }
+}
+
+func TestPostOverview(t *testing.T) {
+ ctx := context.Background()
+ g := llm.EchoTextGenerator()
+ d1 := &Doc{URL: "https://example.com", Author: "rsc", Title: "title", Text: "some text"}
+ d2 := &Doc{Text: "some text 2"}
+ got, err := PostOverview(ctx, g, d1, []*Doc{d2})
+ if err != nil {
+ t.Fatal(err)
+ }
+ promptParts := []string{
+ `{"url":"https://example.com","author":"rsc","title":"title","text":"some text"}`,
+ `{"text":"some text 2"}`,
+ postAndComments.instructions(),
+ }
+ want := &OverviewResult{
+ Overview: llm.EchoResponse(promptParts...),
+ Prompt: promptParts,
+ }
+ if diff := cmp.Diff(got, want); diff != "" {
+ t.Errorf("PostOverview() mismatch (-got +want):\n%s", diff)
+ }
}
func TestInstructions(t *testing.T) {
@@ -37,7 +64,7 @@
wantPost := "post" // only in PostAndComments
t.Run("Documents", func(t *testing.T) {
- di := Documents.instructions()
+ di := documents.instructions()
if !strings.Contains(di, wantAll) {
t.Errorf("Documents.instructions(): does not contain %q", wantAll)
}
@@ -47,7 +74,7 @@
})
t.Run("PostAndComments", func(t *testing.T) {
- pi := PostAndComments.instructions()
+ pi := postAndComments.instructions()
if !strings.Contains(pi, wantAll) {
t.Fatalf("PostAndComments.instructions(): does not contain %q", wantAll)
}
