-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Sets up metadata db for every llm class #1401
Conversation
Hi @PranavPuranik Can you please check why tests are failing? |
15a7824
to
22ea8d4
Compare
#1449 will fix |
Nice, I will wait till its merged in. All tests are passing my laptop - for every commit. Funny how they end up failing here in CI... Investigating |
e6a9512
to
96387f0
Compare
tests/llm/conftest.py
Outdated
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.
Hey, what is the need of adding this test in LLM folder?
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.
Original Problem: We couldn't run LLM tests individually.
Initially, I had the Alembic fixture under the original conftest file at tests/conftest.py
. Tests were passing individually and overall on my laptop from the command line, but they failed on CI. I suspected it was an integration test failure because the error indicated a database issue.
To address this, I moved the fixture inside the LLM folder since we don't need to perform Alembic upgrades for LLM testing but might need it somewhere else. For LLMs, we only test assert_called_once
or similar assertions. Moving it inside reduces the scope of the fixture to only run on these LLM tests.
Additionally, PyCharm now picks up this tests/llm/conftest.py
, which is great because it wasn't able to pass individual tests when the fixture was in tests/conftest.py
. Although this could be due to some configuration issue.
@PranavPuranik can you please resolve the conflict here? Changes look good. |
96387f0
to
6eeddbd
Compare
resolving |
1d66d37
to
d90c300
Compare
d90c300
to
77bf1a5
Compare
eceb55b
to
b901c8d
Compare
Description
The metadata database for storing chat history is a sqllite3 db. It is setup up in the app class by using
setup_engine
.But running individual tests in llm folder fails because we don't create any app for tests. Individual llm classes are called independently. Thus,
setup_engine
is never called.This change moves the setup to BaseLlmConfig, which is called every time we load an llm.
Fixes #1400
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Tested individual tests in llm folder, and also tested running my demo app.
Checklist:
Maintainer Checklist