Skip to content
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

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

Open
jprafael opened this issue Aug 23, 2023 · 6 comments
Open

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

jprafael opened this issue Aug 23, 2023 · 6 comments

Comments

@jprafael
Copy link

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
Copy link
Contributor

a9raag commented Aug 24, 2023

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
Copy link

leio10 commented Nov 23, 2023

Hi @a9raag!

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

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
Copy link

leio10 commented Nov 23, 2023

@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?

@marioplumbarius
Copy link

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

@SimFG
Copy link
Collaborator

SimFG commented Nov 25, 2023

@marioluan i have merged it to dev branch

@leio10
Copy link

leio10 commented Nov 27, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants