Skip to content
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

Adding official agent protocol test to ci + github badge #698

Merged
merged 26 commits into from
Sep 26, 2023

Conversation

ATheorell
Copy link
Collaborator

No description provided.

@ATheorell
Copy link
Collaborator Author

ATheorell commented Sep 15, 2023

Is there an obvious reason why neither the ci.yaml, nor validate_agent_protocol.yml are run in the checks? They are the whole purpose with this PR. Again asking for help, @k1lgor, since you've been involved in setting up the ci.

@ATheorell
Copy link
Collaborator Author

I can reproduce this failure locally and it is because the OPENAI_API_KEY is not defined. Long term, it would be nice to have some kind of dummy model that could be used.

For now, the easy fix is to add the api key to github as a secret, once that is done, the action script should already be reading from it. Would be great if you could do this @AntonOsika https://docs.github.com/en/actions/security-guides/using-secrets-in-github-actions (I don't have permission). Call the secret OPENAI_API_KEY

And while your at it, feel free to add a codecov.io token as a secret too, which will enable us to monitor test coverage.

@ATheorell
Copy link
Collaborator Author

This is currently failing because pip install agent-protocol is down. Waiting til they get it up again.

@ATheorell ATheorell requested a review from pbharrin September 26, 2023 17:06
@ATheorell
Copy link
Collaborator Author

Now all tests are passing. I took the freedom to make a further update and use tempdir as the default for hosting the tasks. Let me know if this has any clear disadvantages.
We bypass the lack of a api key in the online test by catching the authentication error and printing a message. Not sure if this is agood long term solution.

Copy link
Contributor

@pbharrin pbharrin left a comment

Choose a reason for hiding this comment

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

LGTM, very professional cleanup.

@pbharrin pbharrin merged commit d854f2a into AntonOsika:main Sep 26, 2023
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.

2 participants