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

Allow to retry a model when it errors #125

Merged
merged 1 commit into from
May 17, 2024
Merged

Allow to retry a model when it errors #125

merged 1 commit into from
May 17, 2024

Conversation

ruiAzevedo19
Copy link
Collaborator

Part of #123

@ruiAzevedo19 ruiAzevedo19 marked this pull request as draft May 17, 2024 11:30
@ruiAzevedo19 ruiAzevedo19 force-pushed the 123-model-retry branch 3 times, most recently from b5129de to 3decf21 Compare May 17, 2024 11:52
@bauersimon bauersimon marked this pull request as ready for review May 17, 2024 12:27
@bauersimon bauersimon self-requested a review May 17, 2024 12:27
@bauersimon bauersimon added this to the v0.5.0 milestone May 17, 2024
@bauersimon bauersimon added the enhancement New feature or request label May 17, 2024
cmd/eval-dev-quality/cmd/evaluate.go Outdated Show resolved Hide resolved
cmd/eval-dev-quality/cmd/evaluate_test.go Outdated Show resolved Hide resolved
cmd/eval-dev-quality/cmd/evaluate_test.go Outdated Show resolved Hide resolved
cmd/eval-dev-quality/cmd/evaluate_test.go Show resolved Hide resolved
cmd/eval-dev-quality/cmd/evaluate_test.go Outdated Show resolved Hide resolved
model/llm/llm.go Outdated Show resolved Hide resolved
Copy link
Member

@bauersimon bauersimon left a comment

Choose a reason for hiding this comment

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

Awesome!

@bauersimon bauersimon merged commit 181d97c into main May 17, 2024
3 checks passed
@bauersimon bauersimon deleted the 123-model-retry branch May 17, 2024 14:15
)
},
After: func(t *testing.T, logger *log.Logger, resultPath string) {
delete(provider.Providers, "testing-provider")
Copy link
Member

Choose a reason for hiding this comment

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

This is a source of problems, when we register a provider temporarily, we can run into concurrency problems. Better to refactor the code, make the provider a parameter and mock that parameter instead. Easier to type. Easier to understand. Less code. Cleaner.

Name: "Single try fails",

Before: func(t *testing.T, logger *log.Logger, resultPath string) {
queryMock.On("Query", mock.Anything, "testing-provider/testing-model", mock.Anything).Return("", errors.New("empty response from model"))
Copy link
Member

Choose a reason for hiding this comment

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

Do not copy errors, create a variable.

Copy link
Member

Choose a reason for hiding this comment

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

If you copy, don't. Create an API for it.

@@ -124,3 +147,8 @@ func (m *Model) GenerateTestsForFile(logger *log.Logger, language language.Langu

return assessment, nil
}

// SetAttempts sets the number of attempts to perform when a model errors in the process of solving a task.
Copy link
Member

@zimmski zimmski May 18, 2024

Choose a reason for hiding this comment

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

When we implement an interface, we always explicitly check that the interface is implemented it in the code. Even if it is just one method.

@bauersimon bauersimon mentioned this pull request Jun 3, 2024
45 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants