zilliztech / GPTCache

Semantic cache for LLMs. Fully integrated with LangChain and llama_index.
https://gptcache.readthedocs.io
MIT License
6.96k stars 490 forks source link

[Bug]: Failed to import gptcache due to missing redis dependency #521

Open jprafael opened 11 months ago

jprafael commented 11 months ago

Current Behavior

Importing gptcache fails in environments where redis is not installed (because its not declared as a required dependency) and it can't be installed (e.g. read only filesystem such as AWS lambda).

import guidance
  File "/var/task/guidance/__init__.py", line 7, in <module>
    from ._program import Program
  File "/var/task/guidance/_program.py", line 17, in <module>
    from .llms import _openai
  File "/var/task/guidance/llms/__init__.py", line 1, in <module>
    from ._openai import OpenAI, MSALOpenAI, AzureOpenAI
  File "/var/task/guidance/llms/_openai.py", line 15, in <module>
    from ._llm import LLM, LLMSession, SyncSession
  File "/var/task/guidance/llms/_llm.py", line 5, in <module>
    from .caches import Cache, DiskCache
  File "/var/task/guidance/llms/caches/__init__.py", line 3, in <module>
    from ._gptcache import GPTCache
  File "/var/task/guidance/llms/caches/_gptcache.py", line 8, in <module>
    from gptcache.adapter.api import get, put, init_similar_cache
  File "/var/task/gptcache/__init__.py", line 5, in <module>
    from gptcache.core import Cache
  File "/var/task/gptcache/core.py", line 7, in <module>
    from gptcache.manager import get_data_manager
  File "/var/task/gptcache/manager/__init__.py", line 4, in <module>
    from gptcache.manager.factory import get_data_manager, manager_factory
  File "/var/task/gptcache/manager/factory.py", line 6, in <module>
    from gptcache.manager.data_manager import SSDataManager, MapDataManager
  File "/var/task/gptcache/manager/data_manager.py", line 10, in <module>
    from gptcache.manager.eviction.distributed_cache import NoOpEviction
  File "/var/task/gptcache/manager/eviction/distributed_cache.py", line 8, in <module>
    import_redis()
  File "/var/task/gptcache/utils/__init__.py", line 260, in import_redis
    _check_library("redis")
  File "/var/task/gptcache/utils/__init__.py", line 59, in _check_library
    prompt_install(package if package else libname)
  File "/var/task/gptcache/utils/dependency_control.py", line 20, in prompt_install
    raise PipInstallError(package) from e
Could not install packages due to an OSError: [Errno 30] Read-only file system: '/home/sbx_user1051'

Expected Behavior

gptcache shouldn't attempt to install packages on the user's behalf. mandatory packages should be declared in requirements.txt/setup.py and optional features should be enabled only when said packages are available, raising an error at runtime if that functionality is attempted without the required packages

Steps To Reproduce

Create an AWS lambda function that declares gptcache as a dependency.
In the handler import gptcache.
Trigger the lamdba.

Environment

AWS Lambda

Anything else?

No response

a9raag commented 11 months ago

It was because the NoOpEviction class was being imported from the distributed_cahe module which has the implementation for RedisEviction as well I have created a PR where the RedisEviction is put into a separate module to avoid that import.

leio10 commented 8 months ago

Hi @a9raag!

I landed here searching for a solution to this problem, so I guess it's still not solved. :disappointed:

I have not much context about this project, but checking the code I'd say that the problem is in this line, where we are importing the RedisCacheStorage class, but also loads the module and try to install redis.

As the class is only loaded to check a condition, would it be possible to change that condition to avoid importing the class at the very top level? I don't know if it's possible to find another way to decide if the condition is true, or check first if eviction_manager == "redis" and only then doing the import of the class to check the other part in the condition.

Thanks!

leio10 commented 8 months ago

@a9raag I've created this PR trying to solve the issue. As scalar is the name given to the CacheBase class to decide which class it should instance, I think checking it would be equivalent, but without loading classes. But I was not able to test it, what do you think? Could it work?

marioluan commented 8 months ago

@a9raag any chance to merge @leio10's pr to fix this issue?

SimFG commented 8 months ago

@marioluan i have merged it to dev branch

leio10 commented 8 months ago

Thanks @SimFG!! :heart: In order to completely fix the issue we need this change to be released. Are you planning to release a new version soon? :grimacing: