Add context to BaseSummaryEngine #907
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
vachillo
force-pushed
the
context_prompt_summary
branch
from
959b8b5
to
5158a57
Compare
vachillo
force-pushed
the
context_prompt_summary
branch
from
5158a57
to
2aca7db
Compare
| @@ -9,7 +9,7 @@ | |||
| class TestPromptSummaryEngine: | |||
There was a problem hiding this comment.
Would be nice if there was at least one test that used the context in the template too (currently added tests seem to just supply the context init param, but never test that it effects the output).
| from griptape.artifacts import TextArtifact, ListArtifact | ||
| from griptape.rules import Ruleset | ||
|
|
||
|
|
||
| @define | ||
| class BaseSummaryEngine(ABC): | ||
| context: dict[str, Any] = field(factory=dict, kw_only=True) |
There was a problem hiding this comment.
I'm wondering if additional template parameters makes sense in the engine or not. Do you have a specific use case that would make it easier to deduce the requirements on possible solutions? Is the use case dependent on adding the template parameters to the engine? For example, do you need to be able to use the engine with the context outside of a task/structure?
If we do indeed want to supply the additional template parameters in the engine, I think it should at least be named something other than context, to make it less confusing with the existing context in BaseTask.context. This existing context gets consumed (though indirectly) by TextSummaryTask (via BaseTask.full_context and BaseTextInputTask.input).
There was a problem hiding this comment.
What if we added a context field to the J2 class instead?
There was a problem hiding this comment.
it is meant to mimic the init parameter on BaseTask. yes the use case is custom template rendering. right now in the summary engine (or really any class that uses templates), it is really hard to know what parameters are accessible when overriding a template. see #872.
all of the existing uses of full_context are only for input rendering (which this class does not have), not system/user prompt rendering. i can follow the same context init and full_context property pattern if that would be better here? context field on J2 might make sense too, but that would be a much bigger change.
There was a problem hiding this comment.
Is this PR intended to solve #872?
context field on J2 might make sense too, but that would be a much bigger change.
Would it be a big change though? The change would be isolated to J2, and it would benefit all uses of J2 rather than just this Engine.
|
closing to reimplement on J2 |
No description provided.