Skip to content

Commit

Permalink
Fix empty doc generation. (#308)
Browse files Browse the repository at this point in the history
* In 14 of our 424 examples in evaluation the input document sent to the
model ends up being the empty string

* This is the result of how our doc tailer works. Our doc tailer imposes
a length cap on the tail of the document. There was a bug in the tailer
where if the last cell in the document exceeded the cap (currently 1110
characters) then an empty string would be returned.

* This PR fixes that. If the last cell exceeds the length then we take
the tail of that cell.

* This PR also checks in completer that the tail of the document is non
empty; if its not empty then we fail the completion rather than
continuing to generate the completion.

* Add a level1 assertion so we can measure how often we include less
than 1 full cell in the prompt.

* Fix #305
  • Loading branch information
jlewi authored Oct 17, 2024
1 parent 1988d6c commit bd6dd6d
Show file tree
Hide file tree
Showing 9 changed files with 306 additions and 137 deletions.
8 changes: 7 additions & 1 deletion app/pkg/agent/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ func (a *Agent) completeWithRetries(ctx context.Context, req *v1alpha1.GenerateR
log := logs.FromContext(ctx)

cells := preprocessDoc(req)
t := docs.NewTailer(cells, MaxDocChars)
t := docs.NewTailer(ctx, cells, MaxDocChars)

exampleArgs := make([]Example, 0, len(examples))
for _, example := range examples {
Expand All @@ -158,6 +158,10 @@ func (a *Agent) completeWithRetries(ctx context.Context, req *v1alpha1.GenerateR
Examples: exampleArgs,
}

if len(strings.TrimSpace(docText)) == 0 {
return nil, errors.New("Unable to generate a completion because the document is empty")
}

var sb strings.Builder
if err := promptTemplate.Execute(&sb, args); err != nil {
return nil, errors.Wrapf(err, "Failed to execute prompt template")
Expand All @@ -178,6 +182,8 @@ func (a *Agent) completeWithRetries(ctx context.Context, req *v1alpha1.GenerateR
}

// Level1 assertion that docText is a non-empty string
// TODO(jeremy): This should be redundant now that we are checking for an empty document before calling the
// completer and throw an error if we have an empty document. So we could probably remove this assertion.
assertion := &v1alpha1.Assertion{
Name: v1alpha1.Assertion_NON_EMPTY_DOC,
Result: v1alpha1.AssertResult_PASSED,
Expand Down
51 changes: 49 additions & 2 deletions app/pkg/docs/tailer.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
package docs

import (
"context"
"strings"

"github.com/jlewi/foyle/app/pkg/logs"
"github.com/jlewi/foyle/app/pkg/runme/ulid"

"github.com/jlewi/foyle/protos/go/foyle/v1alpha1"
)

Expand All @@ -17,20 +21,37 @@ type Tailer struct {
firstBlock int
}

func NewTailer(blocks []*v1alpha1.Block, maxCharLen int) *Tailer {
func NewTailer(ctx context.Context, blocks []*v1alpha1.Block, maxCharLen int) *Tailer {
log := logs.FromContext(ctx)
mdBlocks := make([]string, len(blocks))

length := 0
firstBlock := len(blocks) - 1

assertion := &v1alpha1.Assertion{
Name: v1alpha1.Assertion_AT_LEAST_ONE_FULL_INPUT_CELL,
Result: v1alpha1.AssertResult_PASSED,
Detail: "",
Id: ulid.GenerateID(),
}
for ; firstBlock >= 0; firstBlock-- {
block := blocks[firstBlock]
md := BlockToMarkdown(block)
if length+len(md) > maxCharLen {
break
if length > 0 {
// If adding the block would exceed the max length and we already have at least one block then, break
break
} else {
// Since we haven't added any blocks yet, we need to add a truncated version of the last block
assertion.Result = v1alpha1.AssertResult_FAILED
md = tailLines(md, maxCharLen)
}
}
length += len(md)
mdBlocks[firstBlock] = md
}

log.Info(logs.Level1Assertion, "assertion", assertion)
return &Tailer{
mdBlocks: mdBlocks,
}
Expand All @@ -57,3 +78,29 @@ func (p *Tailer) Shorten() bool {
p.firstBlock += 1
return true
}

// tailLines should always return a non-empty string if the input is non-empty.
// s is longer than maxLen then it will attempt to return the last n lines of s.
func tailLines(s string, maxLen int) string {
lines := strings.Split(s, "\n")

startIndex := len(lines) - 1
//if startIndex < 0 {}

length := len(lines[len(lines)-1])

for ; startIndex >= 1; startIndex-- {
nextIndex := startIndex - 1
if len(lines[nextIndex])+length > maxLen {
break
}

length += len(lines[nextIndex])
}

if startIndex < 0 {
startIndex = 0
}

return strings.Join(lines[startIndex:], "\n")
}
105 changes: 105 additions & 0 deletions app/pkg/docs/tailer_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
package docs

import (
"context"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/jlewi/foyle/protos/go/foyle/v1alpha1"
)

func Test_Tailer(t *testing.T) {
type testCase struct {
name string
Doc *v1alpha1.Doc
MaxChars int
Expected string
}

cases := []testCase{
{
name: "cell-longer-than-max-chars",
Doc: &v1alpha1.Doc{
Blocks: []*v1alpha1.Block{
{
Kind: v1alpha1.BlockKind_MARKUP,
Contents: "Cell1",
},
{
Kind: v1alpha1.BlockKind_MARKUP,
Contents: "Cell2\nCell3",
},
},
},
MaxChars: 5,
Expected: "Cell3\n",
},
{
name: "multiple-cells",
Doc: &v1alpha1.Doc{
Blocks: []*v1alpha1.Block{
{
Kind: v1alpha1.BlockKind_MARKUP,
Contents: "Cell1",
},
{
Kind: v1alpha1.BlockKind_MARKUP,
Contents: "Cell2",
},
{
Kind: v1alpha1.BlockKind_MARKUP,
Contents: "Cell3",
},
},
},
MaxChars: 12,
Expected: "Cell2\nCell3\n",
},
}

for _, c := range cases {
tailer := NewTailer(context.Background(), c.Doc.Blocks, c.MaxChars)
actual := tailer.Text()
if d := cmp.Diff(c.Expected, actual); d != "" {
t.Fatalf("Expected text to be %s but got %s; diff:\n%v", c.Expected, tailer.Text(), d)
}
}
}

func Test_tailLines(t *testing.T) {
type testCase struct {
name string
Contents string
MaxChars int
Expected string
}

cases := []testCase{
{
name: "last-line-exceeds-max-chars",
Contents: "line1\nline2",
MaxChars: 2,
Expected: "line2",
},
{
name: "all-lines",
Contents: "line1\nline2",
MaxChars: 30,
Expected: "line1\nline2",
},
{
name: "some-lines",
Contents: "line1\nline2\nline3",
MaxChars: 10,
Expected: "line2\nline3",
},
}

for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
if tailLines(c.Contents, c.MaxChars) != c.Expected {
t.Fatalf("Expected text to be %s but got %s", c.Expected, tailLines(c.Contents, c.MaxChars))
}
})
}
}
4 changes: 4 additions & 0 deletions protos/foyle/v1alpha1/eval.proto
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,10 @@ message Assertion {
AT_LEAST_ONE_BLOCK = 5;
// AT_LEAST_ONE_POST_PROCESSED asserts that at at least one block is returned after post-processing
AT_LEAST_ONE_BLOCK_POST_PROCESSED = 6;

// AT_LEAST_ONE_FULL_INPUT_CELL asserts that at at least one cell is included without truncation in the input
// prompt.
AT_LEAST_ONE_FULL_INPUT_CELL = 7;
}
// Name of the assertion
Name name = 1;
Expand Down
2 changes: 1 addition & 1 deletion protos/go/foyle/logs/blocks.zap.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion protos/go/foyle/logs/conversion.zap.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion protos/go/foyle/logs/sessions.zap.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion protos/go/foyle/logs/traces.zap.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit bd6dd6d

Please sign in to comment.