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

remove abstract decorator from initialize in BaseEmbeddingEncoder #3730

Closed

Conversation

mattseddon
Copy link

@mattseddon mattseddon commented Oct 18, 2024

Closes #3731. Please see the issue for details.

Comments inline.

@@ -15,7 +15,6 @@ class EmbeddingConfig(BaseModel):
class BaseEmbeddingEncoder(ABC):
config: EmbeddingConfig

@abstractmethod
Copy link
Author

Choose a reason for hiding this comment

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

@mattseddon mattseddon marked this pull request as ready for review October 18, 2024 04:29
@mattseddon mattseddon force-pushed the remove-abstract-decorator branch 5 times, most recently from 58c50b4 to d8cad90 Compare October 26, 2024 00:47
@mattseddon mattseddon force-pushed the remove-abstract-decorator branch from d8cad90 to deb4c4e Compare November 1, 2024 18:49
@mattseddon
Copy link
Author

@MKhalusova, can you help me get this reviewed?... please 🙏🏻🙏🏻🙏🏻. The DataChain unstructured examples (here and here) are currently stuck on an older version of Unstructured because of #3731 and this change should close that issue.

@mattseddon
Copy link
Author

@cragwolfe would this 👇🏻 be the correct format for the changelog entry?

0.16.5-dev0

Enhancements

Features

Fixes

  • Remove abstract decorator from initialize in BaseEmbeddingEncoder

@MKhalusova MKhalusova requested a review from scanny November 6, 2024 13:10
@MKhalusova
Copy link

@scanny Cam you please take a look?

@scanny scanny requested a review from rbiseck3 November 6, 2024 17:25
@scanny
Copy link
Collaborator

scanny commented Nov 6, 2024

@rbiseck3 Hi Roman, can you take a look at this one-liner? I think you were closer to the choices on this one. I'm not sure whether this is the right fix or whether HuggingFaceEmbeddingEncoder needs to implement a trivial .initialize() method.

@mattseddon mattseddon force-pushed the remove-abstract-decorator branch from deb4c4e to 2acaa3b Compare November 9, 2024 02:06
@mattseddon mattseddon force-pushed the remove-abstract-decorator branch from 2acaa3b to 5763efa Compare December 10, 2024 11:55
@mattseddon
Copy link
Author

Can someone please take a look at this?

@rbiseck3
Copy link
Contributor

@mattseddon all the embedder code was actually migrated to the new ingest repo, and this is no longer an issue there: unstructured_ingest/embed/interfaces.py. This code should be emitting a deprecation warning at the moment.

@mattseddon
Copy link
Author

@mattseddon all the embedder code was actually migrated to the new ingest repo, and this is no longer an issue there: unstructured_ingest/embed/interfaces.py. This code should be emitting a deprecation warning at the moment.

I didn't get the deprecation warning because the code won't run without this change and I had to revert to a previous version.

@mattseddon mattseddon force-pushed the remove-abstract-decorator branch from 5763efa to b47c8ca Compare December 10, 2024 22:09
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.

bug/unable to instantiate HuggingFaceEmbeddingEncoder from unstructured.embed.huggingface
4 participants