Refactor to remove QueryRequest entity
#799
Conversation
dosubot
bot
added
the
size:L
jamesbraza
force-pushed
the
no-query-request
branch
2 times, most recently
from
618e857 to
22aee3c
Compare
| self._docs = docs | ||
| self._llm_model = llm_model | ||
| self._summary_llm_model = summary_llm_model | ||
| self._embedding_model = embedding_model | ||
| self._session_id = session_id |
There was a problem hiding this comment.
One minor feature we lose here is that he query_request had the session id, which you could then match to a given response. So if you built out a bunch of queries you could match to answers after.
Obviously the AnswerResponse object stores both, so I don't think this would cause an issue, but I'm trying to think through any edge cases.
There was a problem hiding this comment.
I think it's good to have less duplication of the ID within paper-qa itself. Ideally we have one source of truth
Let me know if you'd like any unit tests added
|
Also we need to make a couple minor changes to Readme I think |
jamesbraza
force-pushed
the
no-query-request
branch
from
22aee3c to
eb324aa
Compare
Done 👌 nice catch |
There was a problem hiding this comment.
I'm irrationally worried about this change because of the potential size of the affected interfaces, but I can't think of a rational reason not to approve 😄. Thanks @jamesbraza.
jamesbraza
force-pushed
the
no-query-request
branch
from
eb324aa to
6df0de9
Compare
The
QueryRequestentity here served a few purposes oriented around a PaperQA web service infrastructure:Docswith a given runQueryRequestalso comes with a few tradeoffs:deepcopysinceQueryRequestis stateful(query, settings), a session ID is irrelevantSince this repo is really more about CLI usage and the core algorithm, we don't need this entity here, so this PR removes it.