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

Refactoring of TigerRAG Package #9

Merged
merged 7 commits into from
Nov 5, 2023

Conversation

habichta
Copy link
Contributor

@habichta habichta commented Nov 5, 2023

Refactoring of TigerRAG

Originally meant to do enhancement of issue 5 (#5), hence the branch name. However, turned out to be a refactoring (sorry).

Major Changes:

  • Required dependencies are in setup.py instead of a requirements.txt file. Like this they get installed when the tigerrag package gets installed. Like this the demos can run as is. The idea is that the tigerrag package dependencies should be self contained. If I wanted to use this package I would expect it to work like all other packages (not asking me to install another requirements.txt). Might make sense to go modern and use a pyproject.toml.
  • Refactoring of basic functionality into the tigerrag package. The current package doesn't really do anything. The demos contain the full code which seems weird. Now, the demos don't need to import any other third-party dependencies. All comes package within tigerrag. Yay!
  • Proper typing within the packages. I know pedantic but you will thank me later... I hope
  • I took the liberty to create a structure within the package. May not be what you intended...
  • Also took the liberty to add a standard .gitignore file. Can't live without one to be honest. It's like a asking people to take of their shoes when entering the house. Keeps things cleaner
  • Updated README.md files. Should be simpler to set stuff up. Demos worked (did not test langChain demo, as this one failed for me anyways)
  • The only thing I regret about this pull request is that I added more code than I deleted

This was a "bored sunday" endeavour. Cherry-pick what you like or discard. Up to you. Good luck

@wendyran
Copy link
Collaborator

wendyran commented Nov 5, 2023

Hey @habichta ,

First and foremost, a huge thank you for dedicating your "bored Sunday" to enhancing our project. Your enthusiasm and proactive approach is truly commendable!

  • Re-structuring the packages: Absolutely stellar work here. This was on our roadmap, and you've laid a great foundation for us. It's precisely the direction we wanted to head in.
  • Dependencies in setup.py: Transitioning from requirements.txt to having dependencies in setup.py is a thoughtful change. It indeed makes the package more self-contained and user-friendly. Your suggestion to possibly use pyproject.toml is noted, and we'll be looking into it.
  • Typing & Structure: Your attention to detail with proper typing and structuring is much appreciated. It not only makes the code more readable but also more maintainable in the long run.
  • .gitignore & README updates: Thanks for these housekeeping updates. A standard .gitignore is indeed like asking visitors to take off their shoes—clean and considerate.

Regarding the langChain demo, I've created a new issue to address its challenges. We'll ensure it runs smoothly moving forward.

Lastly, don't worry about adding more code than deleting. Quality over quantity, and you've certainly delivered quality!

Once again, thank you for your invaluable contribution. We're excited to integrate your changes and look forward to any future collaborations!

@wendyran wendyran merged commit e71ae33 into tigerlab-ai:main Nov 5, 2023
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.

2 participants