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

feat(db): refactor db to support redis #783

Merged
merged 20 commits into from
Oct 27, 2024

Conversation

shreypandey
Copy link
Contributor

@shreypandey shreypandey commented Oct 19, 2024

Closes #782

πŸ“‘ Description

βœ… Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed
  • Branch name follows type/descript (e.g. feature/add-llm-agents)
  • Ready for code review

β„Ή Additional Information

@shreypandey
Copy link
Contributor Author

Hi @lwaekfjlk ,
I have created this PR for refactoring code for database clients as discussed in #428.
This adds the abstract class for database client, which can be extended by other clients such as redis in subsequent PRs.

Please review this PR once and let me know your thoughts on this.

@lwaekfjlk
Copy link
Member

thanks, I will check later today

@shreypandey
Copy link
Contributor Author

Hi @lwaekfjlk ,
I have updated the documentation to run the code. Please review the updated README.md for that.

@lwaekfjlk lwaekfjlk changed the title DB Refactor feat(db): refactor db to support redis Oct 20, 2024
@lwaekfjlk
Copy link
Member

@shreypandey wow, your PR is amazing! Let's work together on more things!

@lwaekfjlk
Copy link
Member

I just leave some little comments, generally looks amazingly good to me.

.env.template Outdated
@@ -0,0 +1,5 @@
# openai keys
OPENAI_API_KEY=
Copy link
Member

Choose a reason for hiding this comment

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

what do you think is the advantage for using env compared with using export ENV_NAME.

For env, maybe add something like OPENAI_API_KEY="xxx"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you think is the advantage for using env compared with using export ENV_NAME.

Hey @lwaekfjlk ,
When we integrate redis(or some other vectorDB), they generally have their set of ENV variables we need to provide.
This is similar to the scenario when we want to use any other llm provider(such as azure etc) apart from default openai in litellm.

So, to accommodate the increasing number of env variables, it'll be easier to set the env variables by sourcing the env file rather than individually setting each variable. In shell, the command to source the env file looks something like this:

$ set -a
$ source .env
$ set +a

Let me know your thoughts on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For env, maybe add something like OPENAI_API_KEY="xxx"?

The idea behind this is, that .env.template file is a placeholder file which specify all the env variable names the project is using. Users will create a .env file based on this template(they can just copy the entire template file).

Putting empty value denotes that the variable wasn't set by the user and can be handled appropriately in python code. Something like

api_key = os.getenv("OPENAI_API_KEY") # given empty string if variable is not set
asset api_key # Assertion error if API_KEY is not set, similar to not setting the value

Let me know your thoughts.

@shreypandey
Copy link
Contributor Author

@shreypandey wow, your PR is amazing! Let's work together on more things!

Sure @lwaekfjlk ! Would love to work on more things!

@shreypandey shreypandey marked this pull request as draft October 21, 2024 15:16
@shreypandey
Copy link
Contributor Author

Converting this to draft as waiting for #785 to merge. I will resolve the conflicts post merging #785 and reopen this.

@lwaekfjlk
Copy link
Member

lwaekfjlk commented Oct 22, 2024

@shreypandey thanks for your efforts, I merged #785

@shreypandey shreypandey marked this pull request as ready for review October 23, 2024 16:30
@shreypandey
Copy link
Contributor Author

shreypandey commented Oct 23, 2024

Hi @lwaekfjlk,
I have resolved the conflicts and reopened the PR.
Please review the PR and let me know your thoughts on this. The PR is ready to merge now.

I have tested the code by running the test cases, examples and demo.

@lwaekfjlk
Copy link
Member

lwaekfjlk commented Oct 23, 2024

sure. This is a big PR so I need to take some time and confirm on my side everything in bench works as well.

@shreypandey
Copy link
Contributor Author

Sure @lwaekfjlk! Let me know your feedback and incase of any issues.

Copy link
Member

@lwaekfjlk lwaekfjlk left a comment

Choose a reason for hiding this comment

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

This PR basically add an abstract layer to link to multiple types of basic support of DB (local / redis). The concrete Redis support needs to be implemented later.

@lwaekfjlk lwaekfjlk merged commit c5c0dca into ulab-uiuc:main Oct 27, 2024
14 of 17 checks passed
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

Successfully merging this pull request may close these issues.

[ORG]: Refactor DB client
2 participants