dagql: generalize and unify around Result (formerly known as Instance)#10620
Merged
Conversation
7ec4563 to
f9afd69
Compare
34 tasks
97e84a5 to
a8eda31
Compare
b15d4b4 to
d8fc82f
Compare
fed3558 to
afc776c
Compare
jedevc
reviewed
Jul 3, 2025
Contributor
|
Maybe the PR should be renamed? |
vito
reviewed
Jul 3, 2025
Result (formerly known as Instance)
jedevc
approved these changes
Jul 4, 2025
vito
approved these changes
Jul 4, 2025
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
9aa31b3 to
30c56b0
Compare
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
30c56b0 to
1bc045c
Compare
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
1bc045c to
f6e7e7f
Compare
This file contains hidden or 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
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.
Another big carve-out from persistent cache work, this change:
dagql.Instanceto be able to wrap any type (whereas previously it could only wrap objects)Instanceis renamed to ->Result, which is a bit more clear IMO especially once it is used in the persistent cache PR for also wrapping "cache results"dagql.Resultanddagql.ObjectResult(which is adagql.Resultplus some more object-specific methods)Result, and various impacted parts of the codebaseThe motivation is that the persistent cache was currently introducing even more wrapper types to support carrying around cache-related metadata (i.e. ID in db, etc.), which was rapidly devolving into insanity mixed in with all our existing types, wrappers, etc.
I decided to centralize everything there around the existing
Instancetypes, which makes sense sinceInstanceis already a wrapper for ID-metadata, which is very related to the cache metadata.However, doing that required very substantial updates, most important support for wrapping any type, not just objects.
String, we still want to be able to cache it and reference it, which in this new world means it needs to be wrapped as anInstance.@jedevc mentioned that he had wanted
Instanceto be able to wrap any type for telemetry-related purposes too, so hopefully this should support that too.