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

Fix: defined default var for langchain cache path to enable custom path (issue 987) #982

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

necjamahim
Copy link

When defining lc_cache_path and enabling cache in config:

# Enable third parties caching (e.g LangChain cache)
cache = true
lc_cache_path = './.custom-cache-path.db'

TypeError is thrown:

TypeError: chainlit.config.ProjectSettings() got multiple values for keyword argument 'lc_cache_path'

@necjamahim necjamahim changed the title defined default var for langchain cache path to enable custom path Fix: defined default var for langchain cache path to enable custom path (issue 987) May 13, 2024
Copy link
Contributor

@tpatel tpatel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your code, I've added two comments.

@@ -420,10 +424,7 @@ def load_settings():
"Your config file is outdated. Please delete it and restart the app to regenerate it."
)

lc_cache_path = os.path.join(config_dir, ".langchain.db")

project_settings = ProjectSettings(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you're missing something to change the project_config["lc_cache_path"] value, like:

Suggested change
project_settings = ProjectSettings(
project_config["lc_cache_path"] = project_config.get("lc_cache_path", lc_cache_path)
project_settings = ProjectSettings(

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the way i understand it and tested it locally, possible custom path will be picked up by toml_dict module here:

project_config = toml_dict.get("project", {})

or will default to default value as proposed by my PR here

# default to "{config_dir}/.langchain.db"
lc_cache_path: Optional[str] = os.path.join(config_dir, ".langchain.db")

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @necjamahim is right on this one. Could you please have another look @tpatel?

@@ -19,7 +19,7 @@ def init_lc_cache():
if config.project.lc_cache_path is not None:
set_llm_cache(SQLiteCache(database_path=config.project.lc_cache_path))

if not os.path.exists(config.project.lc_cache_path):
if os.path.exists(config.project.lc_cache_path):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure to understand why you've removed this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is because previous line st_llm_cache generates sqlite db file at path

set_llm_cache(SQLiteCache(database_path=config.project.lc_cache_path))

consequently in next line we want to inform cache was generated by testing if path exists and not the opposite. I hope I was clear - was trying not to use too many "if", "not" in the explaination

Copy link
Collaborator

@dokterbob dokterbob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution @necjamahim!

I see where you're coming from; current config doesn't allow overriding the langchain cache configured through lc_cache_path, which seems hardcoded to {config_dir}/.langchain.db.

So this patch keeps the previously hardcoded value as a default and makes it configurable.

However, I don't see how your changes to set_chat_profiles come in. Could you please remove this from your patch and resolve any merge conflicts?

As soon as that's done I'll do a final manual test and I think we're good to go!

)
set_chat_profiles: Optional[
Callable[[Optional["User"]], List["ChatProfile"]]
] = None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this relevant to the issue at hand?

@@ -54,6 +54,9 @@
# Enable third parties caching (e.g LangChain cache)
cache = false

# Set langchain cache path (default is .langchain.db inside config dir)
# lc_cache_path = './.my-cache-path.db'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is a default config option, it seems better to uncomment it, in line with other default config options.

@@ -420,10 +424,7 @@ def load_settings():
"Your config file is outdated. Please delete it and restart the app to regenerate it."
)

lc_cache_path = os.path.join(config_dir, ".langchain.db")

project_settings = ProjectSettings(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @necjamahim is right on this one. Could you please have another look @tpatel?

@dokterbob dokterbob added bug Something isn't working enhancement New feature or request and removed bug Something isn't working labels Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants