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

Fix sqlite busy errors #347

Merged
merged 9 commits into from
Nov 25, 2024
Merged

Fix sqlite busy errors #347

merged 9 commits into from
Nov 25, 2024

Conversation

jlewi
Copy link
Owner

@jlewi jlewi commented Nov 24, 2024

Fix sqlite busy errors with session based learning

There were a couple different errors

  • We were still enqueing block ids and not sessions in some places
  • When we call Enqueue we should filter out sessions that aren't suitable for learning; this should minimize DB contention
  • Improve logging

By default sqlite doesn't have any retries for SQLITE_BUSY errors. We could configure a timeout for retries using a PRAGMA; we should probably do that but I want to hold off to see if this PR fixes the contention.

Use ko to create docker images as part of the development process

  • This is convenient when we want to build a docker image from our latest changes without doing a relase.
  • Change the Dockerfile to not include any arguments so that it is compatible with the image ko builds

Copy link

netlify bot commented Nov 24, 2024

Deploy Preview for foyle canceled.

Name Link
🔨 Latest commit fa84ee6
🔍 Latest deploy log https://app.netlify.com/sites/foyle/deploys/674402b8f7f4350009456316

Comment on lines +373 to +401
// isLearnable returns true if the session is eligible for learning and false otherwise
func isLearnable(session *logspb.Session) bool {
// Make sure there is a cell execution log event.
// Right now we rely on the cell execution log event to get the actual cell content.
// So we can't learn from sessions without cell execution events. TN013 has some ideas for how we could
// learn in the event of non cell execution events.
execEvent := getLastExecEvent(session)

if execEvent == nil {
// Since the cell wasn't successfully executed we don't learn from it
sessProcessed.WithLabelValues("noexec").Inc()
return false
}

log := zapr.NewLogger(zap.L())
if session.GetFullContext() == nil {
sessProcessed.WithLabelValues("nocontext").Inc()
log.Error(errors.New("Session missing fullcontext"), "contextId", session.GetContextId())
return false
}

if session.GetFullContext().GetNotebook() == nil {
sessProcessed.WithLabelValues("nonotebook").Inc()
log.Error(errors.New("Session missing notebook"), "contextId", session.GetContextId())
return false
}

if session.GetFullContext().GetSelected() == 0 {
// If its the first cell we can't learn from it because what would we use as context to predict it?
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// isLearnable returns true if the session is eligible for learning and false otherwise
func isLearnable(session *logspb.Session) bool {
// Make sure there is a cell execution log event.
// Right now we rely on the cell execution log event to get the actual cell content.
// So we can't learn from sessions without cell execution events. TN013 has some ideas for how we could
// learn in the event of non cell execution events.
execEvent := getLastExecEvent(session)
if execEvent == nil {
// Since the cell wasn't successfully executed we don't learn from it
sessProcessed.WithLabelValues("noexec").Inc()
return false
}
log := zapr.NewLogger(zap.L())
if session.GetFullContext() == nil {
sessProcessed.WithLabelValues("nocontext").Inc()
log.Error(errors.New("Session missing fullcontext"), "contextId", session.GetContextId())
return false
}
if session.GetFullContext().GetNotebook() == nil {
sessProcessed.WithLabelValues("nonotebook").Inc()
log.Error(errors.New("Session missing notebook"), "contextId", session.GetContextId())
return false
}
if session.GetFullContext().GetSelected() == 0 {
// If its the first cell we can't learn from it because what would we use as context to predict it?
// isLearnable determines if the session is learnable based on the presence of execution events and contextual information.
func isLearnable(session *logspb.Session) bool {
// Make sure there is a cell execution log event.
// Right now we rely on the cell execution log event to get the actual cell content.
// So we can't learn from sessions without cell execution events. TN013 has some ideas for how we could
// learn in the event of non cell execution events.
execEvent := getLastExecEvent(session)
if execEvent == nil {
// Since the cell wasn't successfully executed we don't learn from it
sessProcessed.WithLabelValues("noexec").Inc()
return false
}
log := zapr.NewLogger(zap.L())
if session.GetFullContext() == nil {
sessProcessed.WithLabelValues("nocontext").Inc()
log.Error(errors.New("session missing full context"), "contextId", session.GetContextId())
return false
}
if session.GetFullContext().GetNotebook() == nil {
sessProcessed.WithLabelValues("nonotebook").Inc()
log.Error(errors.New("session missing notebook"), "contextId", session.GetContextId())
return false
}
if session.GetFullContext().GetSelected() == 0 {
// If it's the first cell we can't learn from it because what would we use as context to predict it?

add comment to isLearnable function, fix typos in error messages

Comment on lines +408 to +410
// getLasExecEvent returns the last successful execution event in the session
// or null if there isn't one.
func getLastExecEvent(session *logspb.Session) *v1alpha1.LogEvent {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// getLasExecEvent returns the last successful execution event in the session
// or null if there isn't one.
func getLastExecEvent(session *logspb.Session) *v1alpha1.LogEvent {
// getLastExecEvent retrieves the last successful execution log event from a session.
func getLastExecEvent(session *logspb.Session) *v1alpha1.LogEvent {

add comment to getLastExecEvent function

@jlewi jlewi enabled auto-merge (squash) November 25, 2024 04:53
@jlewi jlewi merged commit 2c1180c into main Nov 25, 2024
5 checks passed
@jlewi jlewi deleted the jlewi/dbretries branch November 25, 2024 04:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant