fix: Refactor run handling to use run object instead of runId #531
+515
−385
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CI Failure Feedback 🧐(Checks updated until commit f4632e7)
✨ CI feedback usage guide:The CI feedback tool (
In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR: where Configuration options
See more information about the |
johnjcsmith
reviewed
Jan 9, 2025
|
|
||
| if (!run) { | ||
| throw new Error("Failed to create run"); | ||
| } | ||
|
|
||
| // Insert tags if provided | ||
| if (tags) { | ||
| await db.insert(runTags).values( |
There was a problem hiding this comment.
Should still be possible to do this within the transaction?
There was a problem hiding this comment.
What do you think about not doing a transaction here?
The tags would only be used in a small minority, whereas applying the transaction would apply to all.
We can do a if (tags) transaction, but that's a bit too much code?
There was a problem hiding this comment.
Is there any disadvantage to doing this transactionally though?
There was a problem hiding this comment.
I guess there would be some performance difference but that would be pretty marginal?
There was a problem hiding this comment.
I think the run row is extremely contentious right now, especially in the middle of a run. (asserting run ready etc). So, it's more to do with deadlocks and lags right now.
I think it's best to stay away from database transactions as much as you could, and instead rely on the database schema for atomic updates.
In hindsight, I think there's an opportunity here to reintegrate run tags into the workflow table.
* fix: Remove knowledge artifact related code and components * fix: Minor changes 2025-01-09-22:23:43 * fix: Minor changes 2025-01-09-23:20:58 * fix: Minor changes 2025-01-09-23:20:58 * fix: Minor changes 2025-01-09-23:20:58 * fix: Minor changes 2025-01-09-23:20:58
|
Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here. PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here. PR Code Suggestions ✨Explore these optional code suggestions:
|
johnjcsmith
reviewed
Jan 10, 2025
johnjcsmith
approved these changes
Jan 10, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Type
Bug fix, Enhancement, Tests
Description
Refactored
Runhandling to replacerunIdwithrunobject.Improved type safety by replacing
Runtype with explicit object definitions.Enhanced database schema with stricter constraints and improved default values.
Updated and expanded test cases to align with refactored logic.
Changes walkthrough 📝
8 files
Refactored test cases forextractAuthStateandextractCustomAuthState.Added `modelIdentifier` and other properties to test `run` object.Updated test cases for `postStartEdge` and `postModelEdge`.Refactored test cases for `handleModelCall`.Updated `buildModelSchema` tests with refactored `run` object.Refactored `handleToolCalls` tests with updated `run` object.Updated `findRelevantTools` and `buildMockTools` tests.Refactored `assertRunReady` and related tests.10 files
Enhanced database schema for `runs` table.Updated router logic to use `run` object.Refactored `createRunGraph` to use explicit `run` object.Refactored `handleToolCall` to use explicit `run` object.RefactoredprocessRunand related functions to use explicitrunobject.
Updated `RunGraphState` to use explicit `run` object.Refactored core `Run` handling logic and database operations.Updated `notifyStatusChange` to use explicit `run` object.Refactored router logic to align with `run` object changes.Updated `generateTitle` to use explicit `run` object.