Skip to content

Conversation

@cezarc1
Copy link
Contributor

@cezarc1 cezarc1 commented Feb 28, 2025

Fixes and removes the now non-existent dependency on dsp.SentenceTransformersVectorizer from KNN and indirectly from KNNFewShot.

Also:

  • Re-enables and fixes all relevant tests
  • Removes trainset and valset params from the KNNFewShot.compile call as they were unused.
  • Updates documentation to reflect new usage.

Copy link
Collaborator

@chenmoneygithub chenmoneygithub 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 the PR, it's a very solid work!

Left some comments for readability improvements, the functionality looks good to me!

@cezarc1
Copy link
Contributor Author

cezarc1 commented Mar 1, 2025

Thanks! Should have resolved all comments. PTAL.

@cezarc1 cezarc1 requested a review from chenmoneygithub March 1, 2025 01:11
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran the linter but it seems to have run it against the whole file. It did find lots of fixes.... I can revert if need be but I think this should be done regardless.

Copy link
Collaborator

Choose a reason for hiding this comment

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

no worries, I will push a commit to fix it

Copy link
Collaborator

@chenmoneygithub chenmoneygithub left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

Choose a reason for hiding this comment

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

no worries, I will push a commit to fix it

@chenmoneygithub chenmoneygithub merged commit 21b21e8 into stanfordnlp:main Mar 1, 2025
6 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.

2 participants