langchain-ai / langchain-cohere

MIT License
16 stars 15 forks source link

Cohere client.tokenize() now requires model argument #19

Closed znwilkins closed 2 months ago

znwilkins commented 2 months ago

Using a LangChain ConversationalRetrievalChain, we fail on a call to tokenize because of a missing model parameter:

> Entering new ConversationalRetrievalChain chain...
INFO:httpx:HTTP Request: POST https://api.cohere.ai/v1/embed "HTTP/1.1 200 OK"
INFO:httpx:HTTP Request: POST http://localhost:6333/collections/my.collection/points/search "HTTP/1.1 200 OK"
ERROR:root:Exception raised by task = <Task finished name='Task-522' coro=<Chain.ainvoke() done, defined at /home/zachary/.cache/pypoetry/virtualenvs/MY_VENV/lib/python3.10/site-packages/langchain/chains/base.py:170> exception=TypeError("Client.tokenize() missing 1 required keyword-only argument: 'model'")>
Traceback (most recent call last):
  File "/home/zachary/PycharmProjects/my_project/somewhere/in/my/code/utils.py", line 108, in _handle_task_result
    task.result()
  File "/home/zachary/.cache/pypoetry/virtualenvs/MY_VENV/lib/python3.10/site-packages/langchain/chains/base.py", line 212, in ainvoke
    raise e
  File "/home/zachary/.cache/pypoetry/virtualenvs/MY_VENV/lib/python3.10/site-packages/langchain/chains/base.py", line 203, in ainvoke
    await self._acall(inputs, run_manager=run_manager)
  File "/home/zachary/.cache/pypoetry/virtualenvs/MY_VENV/lib/python3.10/site-packages/langchain/chains/conversational_retrieval/base.py", line 207, in _acall
    docs = await self._aget_docs(new_question, inputs, run_manager=_run_manager)
  File "/home/zachary/.cache/pypoetry/virtualenvs/MY_VENV/lib/python3.10/site-packages/langchain/chains/conversational_retrieval/base.py", line 333, in _aget_docs
    return self._reduce_tokens_below_limit(docs)
  File "/home/zachary/.cache/pypoetry/virtualenvs/MY_VENV/lib/python3.10/site-packages/langchain/chains/conversational_retrieval/base.py", line 298, in _reduce_tokens_below_limit
    tokens = [
  File "/home/zachary/.cache/pypoetry/virtualenvs/MY_VENV/lib/python3.10/site-packages/langchain/chains/conversational_retrieval/base.py", line 299, in <listcomp>
    self.combine_docs_chain.llm_chain._get_num_tokens(doc.page_content)
  File "/home/zachary/.cache/pypoetry/virtualenvs/MY_VENV/lib/python3.10/site-packages/langchain/chains/llm.py", line 384, in _get_num_tokens
    return _get_language_model(self.llm).get_num_tokens(text)
  File "/home/zachary/.cache/pypoetry/virtualenvs/MY_VENV/lib/python3.10/site-packages/langchain_cohere/chat_models.py", line 366, in get_num_tokens
    return len(self.client.tokenize(text=text).tokens)
TypeError: Client.tokenize() missing 1 required keyword-only argument: 'model'

Background

This PR made the model: str parameter to tokenize() mandatory, and is included starting in cohere-python v5.2.0.

I was using cohere-python v5.2.5, langchain-core v0.1.42, langchain-community v0.0.32.

langchain-cohere is calling the function without the argument (link), which then throws an error:

    def get_num_tokens(self, text: str) -> int:
        """Calculate number of tokens."""
        return len(self.client.tokenize(text=text).tokens)

Suggested Fix

The Cohere documentation still says that this argument is optional, even though it isn't. If it is indeed mandatory, then the fix is probably to choose a reasonable default while allowing the user to override.

Thanks, I'm happy to provide more info as needed.

harry-cohere commented 2 months ago

Thanks @znwilkins, and apologies for the issue.

To clarify things: the model parameter is now marked as required in the API specification and 5.2.x versions of the SDK. However, 5.1.x versions of the SDK still work as the API itself will be backwards compatible for a short while.

We're actively working on supporting 5.2.x versions in langchain-cohere - ETA is next week (we plan to use the model parameter if set, otherwise we'll fetch the default from our list models endpoint.

If you'd like support in the mean time, downgrading to cohere-python 5.1.x is hopefully an acceptable short term solution.

znwilkins commented 2 months ago

Thanks for the explanation @harry-cohere! Sounds like you folks are on the ball. Downgrading to 5.1x does indeed work for now. 👍

harry-cohere commented 2 months ago

Hello @znwilkins

This is now fixed in langchain-cohere version 0.1.3 - it supports Cohere SDK versions 5.3+

Migrating from the Cohere integration in langchain-community is hopefully straightforward, there's a migration guide here.

Thanks for your patience and please reach out to let us know how it goes

znwilkins commented 2 months ago

Hi @harry-cohere, thanks for the update! I can confirm that using Cohere SDK v5.3.2 and langchain-cohere v0.1.3 this issue has been resolved.

I do notice this log message:

/home/zachary/.cache/pypoetry/virtualenvs/MY_VENV/lib/python3.10/site-packages/cohere/client.py:217: RuntimeWarning: coroutine 'local_tokenize' was never awaited
  opts["additional_headers"]["sdk-api-warning-message"] = "offline_tokenizer_failed"
RuntimeWarning: Enable tracemalloc to get the object allocation traceback

Breaking and printing the exception at that point shows:

Traceback (most recent call last):
  File "/home/zachary/.cache/pypoetry/virtualenvs/MY_VENV/lib/python3.10/site-packages/cohere/client.py", line 213, in tokenize
    tokens = asyncio.run(local_tokenizers.local_tokenize(self, text=text, model=model))
  File "/usr/lib/python3.10/asyncio/runners.py", line 33, in run
    raise RuntimeError(
RuntimeError: asyncio.run() cannot be called from a running event loop

I am calling my chain within a FastAPI application, so looks like a nested eventloop situation. It doesn't affect my LCEL chains though, and that's the way we're moving long-term.

In any case, this is just a warning, so I'm a happy camper! Thanks again!

abdullahkady commented 2 months ago

@znwilkins thank you for the detailed reply. very helpful! working on a fix for the warning now. this should be non-blocking, as it's falling back to call the API to count the tokens.

abdullahkady commented 2 months ago

@znwilkins This is now fixed in 5.3.4 Thank you for reporting:)