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

Lancedb Integration #1444

Merged
merged 5 commits into from
Sep 18, 2024
Merged

Lancedb Integration #1444

merged 5 commits into from
Sep 18, 2024

Conversation

PrashantDixit0
Copy link
Contributor

This PR adds LanceDB as a retriever to handle large datasets.

@PrashantDixit0
Copy link
Contributor Author

@okhat Please approve the workflow. If any changes are required let me know.

@CShorten
Copy link
Collaborator

Hey @PrashantDixit0, happy to help with this! 👋

  • Can you please take a look at the conflicts in the poetry setup and let me know what you think?

  • It looks like the ruff linter is unhappy with unused variables not related to this PR. @PrashantDixit0 you can fix this with ruff check . --unsafe-fixes --fix, cc @isaacbmiller

@PrashantDixit0
Copy link
Contributor Author

Sure I'll fix these conflits and linting asap

@PrashantDixit0
Copy link
Contributor Author

@CShorten While running ruff check . --fix-only I am getting error

error: TOML parse error at line 216, column 1
    |
216 | indent-width = 4
    | ^^^^^^^^^^^^
unknown field `indent-width`, expected one of `allowed-confusables`, `builtins`, `cache-dir`, `dummy-variable-rgx`, `exclude`, `extend`, `extend-exclude`, `extend-include`, `extend-ignore`, `extend-select`, `extend-fixable`, `extend-unfixable`, `external`, `fix`, `fix-only`, `fixable`, `format`, `force-exclude`, `ignore`, `ignore-init-module-imports`, `include`, `line-length`, `tab-size`, `required-version`, `respect-gitignore`, `select`, `show-source`, `show-fixes`, `src`, `namespace-packages`, `target-version`, `task-tags`, `typing-modules`, `unfixable`, `flake8-annotations`, `flake8-bandit`, `flake8-bugbear`, `flake8-builtins`, `flake8-comprehensions`, `flake8-errmsg`, `flake8-quotes`, `flake8-self`, `flake8-tidy-imports`, `flake8-type-checking`, `flake8-gettext`, `flake8-implicit-str-concat`, `flake8-import-conventions`, `flake8-pytest-style`, `flake8-unused-arguments`, `isort`, `mccabe`, `pep8-naming`, `pycodestyle`, `pydocstyle`, `pylint`, `per-file-ignores`, `extend-per-file-ignores`

@CShorten
Copy link
Collaborator

Thanks @PrashantDixit0, maybe for now let's revert the commit of running the linter on all the files. I noticed this when I took a look this morning as well. I'm surprised this changed as many files as it did though, for me it was about ~8 this morning and I was able to manually inspect that it was mostly unused variables, etc. as mentioned in my original comment.

@isaacbmiller @insop can you please advise us on this?

@insop
Copy link
Contributor

insop commented Sep 18, 2024

Thanks @PrashantDixit0, maybe for now let's revert the commit of running the linter on all the files. I noticed this when I took a look this morning as well. I'm surprised this changed as many files as it did though, for me it was about ~8 this morning and I was able to manually inspect that it was mostly unused variables, etc. as mentioned in my original comment.

@isaacbmiller @insop can you please advise us on this?

With @CShorten 's nudge, I went over the change.
I tend to think that if all the changes are related to the PR? many of them look like style changes...

@PrashantDixit0
Copy link
Contributor Author

I'll relock poetry file

@PrashantDixit0
Copy link
Contributor Author

@isaacbmiller @CShorten Done 👍, Now conflicts are resolved

@CShorten
Copy link
Collaborator

Thank you so much @isaacbmiller @insop @PrashantDixit0! 🔥 🚀

@CShorten CShorten merged commit 02b0dc9 into stanfordnlp:main Sep 18, 2024
7 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.

4 participants