Skip to content

Conversation

@caike
Copy link

@caike caike commented Dec 5, 2024

📥 Pull Request

Fixes #98

📘 Description
Not sure if this is the most elegant solution but it fixes an issue where the template environment variable values "..." would persist and cause an error on agenstack run due to Client not being populated with the proper values defined in .env such as AGENTOPS_API_KEY.

Loading the envs before loading the module and favoring .env over system vars (which I suspect should be the recommended behavior: local config > system config) ensures that Client is properly configured with values from .env

🧪 Testing
The agentstack run command is able to successfully connect to my AgentOps account using the API key found on .env

@bboynton97
Copy link
Contributor

Hi @caike! thank you for your contribution!

this is a great addition, i think the one thing we need to make this work though, is to add variables to .env commented out. that way if a user adds a tool, it inserts say OPENAI_API_KEY=... to the .env, then that doesn't override the valid key the user may already have in their environment variables.

If you can make that change, @tcdent or I can merge this PR!

caike added 2 commits December 6, 2024 17:19
This avoids overriding any predefined ENVs the user
may already have set in their system.
@caike caike force-pushed the load-envs-before-agentops branch from d3c31a7 to 4991922 Compare December 6, 2024 22:21
We don't want to override values the user may already have set
in their environment variables so we add a new line commented out.
"""
f.write(f"\n# {key}={value}")
Copy link
Contributor

Choose a reason for hiding this comment

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

beautiful 🔥

@tcdent
Copy link
Collaborator

tcdent commented Dec 9, 2024

While I do agree that there was a problem with the logic of loading environment variables, I don't think this is the correct solution. #105 addresses this in a more correct way.

Thank you for your contribution @caike and feel free to re-open this if you think I missed anything.

@tcdent tcdent closed this Dec 9, 2024
@caike
Copy link
Author

caike commented Dec 9, 2024

While I do agree that there was a problem with the logic of loading environment variables, I don't think this is the correct solution. #105 addresses this in a more correct way.

Thank you for your contribution @caike and feel free to re-open this if you think I missed anything.

Right on, that does make sense 👍

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.

Error "API Key is invalid: {...}."

3 participants