-
Notifications
You must be signed in to change notification settings - Fork 81
Feat/310 default agent conf #316
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Casper Nielsen <[email protected]>
…time if found Signed-off-by: Casper Nielsen <[email protected]>
Signed-off-by: Casper Nielsen <[email protected]>
Signed-off-by: Casper Nielsen <[email protected]>
Signed-off-by: Casper Nielsen <[email protected]>
Signed-off-by: Casper Nielsen <[email protected]>
|
@sicoyle before I get too far it would be good to align on the assumptions I take on naming: Secrets:
Conf:
I was thinking we could also allow users to specify this via env variables on equal terms. |
Signed-off-by: Casper Nielsen <[email protected]>
Signed-off-by: Casper Nielsen <[email protected]>
Looks great so far! Instead of an env var equivalent what are your thoughts on if we add an otelConfig dataclass instead as an optional secondary setup to reduce the amt of env vars we support. |
sicoyle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few comments so far for ya :)
dapr_agents/agents/components.py
Outdated
| pubsub = AgentPubSubConfig( | ||
| pubsub_name="agent-pubsub", | ||
| agent_topic=f"{name.replace(' ', '-').lower()}.topic", | ||
| broadcast_topic="agents.broadcast", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if someone has set agent-pubsub and we're picking it up, but they set a diff broadcast_topic? Same for the state components... then, we would be overriding their value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uf, this is a good point. Let me try to handle this scenario better!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, shouldn't that be covered by the conditional:
if (
"pubsub" in component.type
and component.name == "agent-pubsub"
and pubsub is None
):
...If they've added:
apiVersion: dapr.io/v1alpha1
kind: Component
metadata:
name: agent-pubsub
spec:
type: pubsub.redis
version: v1
metadata:
- name: redisHost
value: localhost:6379But they have not added it in the agent instantiation then this does exactly what we want it to, no?
"pubsub" in component.type -> We found the component
and component.name == "agent-pubsub" -> It has the right default naming
and pubsub is None -> And they didn't pass any during instantiation
Then the agent would pick it up as the default pubsub
Co-authored-by: Sam <[email protected]> Signed-off-by: Casper Nielsen <[email protected]>
@sicoyle I think this is a better approach. We'd still have to support the "usual suspects" (i.e., endpoint & token) but it would substantially lower the amount! |
Signed-off-by: Casper Nielsen <[email protected]>
Signed-off-by: Casper Nielsen <[email protected]>
Signed-off-by: Casper Nielsen <[email protected]>
Co-authored-by: Sam <[email protected]> Signed-off-by: Casper Nielsen <[email protected]>
Co-authored-by: Sam <[email protected]> Signed-off-by: Casper Nielsen <[email protected]>
Co-authored-by: Sam <[email protected]> Signed-off-by: Casper Nielsen <[email protected]>
Signed-off-by: Casper Nielsen <[email protected]>
Signed-off-by: Casper Nielsen <[email protected]>
Signed-off-by: Casper Nielsen <[email protected]>
Signed-off-by: Casper Nielsen <[email protected]>
…chromadb Signed-off-by: Casper Nielsen <[email protected]>
Signed-off-by: Casper Nielsen <[email protected]>
… is fetched and used Signed-off-by: Casper Nielsen <[email protected]>
sicoyle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few comments so far and will complete my review in my morning. Thank you!!
Co-authored-by: Sam <[email protected]> Signed-off-by: Casper Nielsen <[email protected]>
Co-authored-by: Sam <[email protected]> Signed-off-by: Casper Nielsen <[email protected]>
Co-authored-by: Sam <[email protected]> Signed-off-by: Casper Nielsen <[email protected]>
…r_client Signed-off-by: Casper Nielsen <[email protected]>
Signed-off-by: Casper Nielsen <[email protected]>
sicoyle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one final comment and then g2g - thank you!
Co-authored-by: Sam <[email protected]> Signed-off-by: Casper Nielsen <[email protected]>
Signed-off-by: Casper Nielsen <[email protected]>
Signed-off-by: Casper Nielsen <[email protected]>
Signed-off-by: Casper Nielsen <[email protected]>
… avoid hanging process Signed-off-by: Casper Nielsen <[email protected]>
Signed-off-by: Casper Nielsen <[email protected]>
…ssed through the payload Signed-off-by: Casper Nielsen <[email protected]>
Signed-off-by: Casper Nielsen <[email protected]>
…ld + include possible system_prompt overwrite Signed-off-by: Casper Nielsen <[email protected]>
Signed-off-by: Casper Nielsen <[email protected]>
Signed-off-by: Casper Nielsen <[email protected]>
Signed-off-by: Casper Nielsen <[email protected]>
sicoyle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, pls fix the linter issue (flake8 I believe) and then this is g2g 🚀
Signed-off-by: Casper Nielsen <[email protected]>
Description
This PR allows default configuration and default secrets to be passed to the agent through
agent-secretstoreandagent-runtime. This PR also covers automatically setting up OTel tracing if the appropriate configuration parameters are passed throughagent-runtimestatestore.This allows operators to automate configuration setup as well as externalize the lifecycle of these configurations.
NB: This carries a breaking change with dropping support for Python 3.10.
Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR closes: #310
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list:
Note: We expect contributors to open a corresponding documentation PR in the dapr/docs repository. As the implementer, you are the best person to document your work! Implementation PRs will not be merged until the documentation PR is opened and ready for review.