-
Notifications
You must be signed in to change notification settings - Fork 46
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
git hub action for the test #202
git hub action for the test #202
Conversation
.github/workflows/tests.yml
Outdated
python -m pip install --upgrade pip | ||
python -m pip install poetry | ||
cd src | ||
pip install numpy==1.25.2 |
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.
Doesn't poetry install
ensure that numpy
is installed?
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.
Please remove the explicit install of numpy here. Running poetry install
should install numpy for us.
.github/workflows/tests.yml
Outdated
echo "SERPER_API_KEY=${{ secrets.SERPAAPIKEY }}" >> .env | ||
echo "GITHUB_AUTH_TOKEN=${{ secrets.GITHUBOATHKEY }}" >> .env | ||
echo "DAILY_TOKEN_LIMIT=${{ secrets.TOKENLIMIT }}" >> .env | ||
echo "TEMPRATURE=${{ secrets.TEMPRATURE }}" >> .env |
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.
Please rename TEMPRATURE
to TEMPERATURE
.github/workflows/tests.yml
Outdated
echo "SLACK_OAUTH_TOKEN=${{ secrets.SLACKOAUTHTOKEN }}" >> .env | ||
echo "SLACK_VERIFICATION_TOKEN=${{ secrets.SLACKVERIFICATIONTOKEN }}" >> .env | ||
echo "OPENAI_API_KEY=${{ secrets.OPENAIKEY }}" >> .env | ||
echo "PINECONE_INDEX=${{ secrets.PINCONEID }}" >> .env |
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.
I can find PINECONE_INDEX
in the code but can't find secrets.PINECONEID
defined anywhere. I suggest making them the same name, PINECONE_INDEX
. (Assuming they represent the same value.)
.github/workflows/tests.yml
Outdated
|
||
- name: Set secrets in .env file | ||
run: | | ||
cd ./src |
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.
I suggest giving the variables in secrets
exactly the same names as the variables we expect in .env
. That will help us avoid typos and confusion.
For example, secrets. SLACK_SIGNING_SECRET
instead of secrets.SLACKSIGNINGSECRET
.github/workflows/tests.yml
Outdated
python -m pip install --upgrade pip | ||
python -m pip install poetry | ||
cd src | ||
pip install numpy==1.25.2 |
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.
Please remove the explicit install of numpy here. Running poetry install
should install numpy for us.
@Eyobyb and @20001LastOrder I'm noting here this PR is related to #224. I suggest we hold off on implementing github actions until we have modified the tests so that, by default, tests don't call 3rd party APIs. After that we should be able to remove this PR's dependencies on environment variables for services like Slack, because the tests will no longer depend on such services. What do you think? |
@amirfz , @Eyobyb , @20001LastOrder and I discussed this further today. We agreed that once we have all our tests running cleanly, here's what we want github actions to do for us:
Next steps:
|
With this pr we merged, we can continue with this PR now. |
c93cd96
to
98c8f24
Compare
98c8f24
to
22c42cf
Compare
…ctical-llms into feature/github-action
6549259
to
1bd5ccd
Compare
1bd5ccd
to
f7e4735
Compare
f7e4735
to
ad64c23
Compare
Sorry for making the commit history a bit messy. I guess we can squeeze all commits. Finally, we have all the ingredients for the offline tests. Should we go ahead and merge it so that all other pr can benefit from the automated tests? @oshoma |
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 - let's go!
GitHub action for the testing. but in order for it to work, these properties
"SLACKSIGNINGSECRET ,SLACKOAUTHTOKEN ,SLACKVERIFICATIONTOKEN ,OPENAIKEY ,PINCONEID ,PINECONEENV ,SLACKPORT ,SERPAAPIKEY ,GITHUBOATHKEY ,TOKENLIMIT, TEMPRATURE " have to be added inside the repo secretes