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

renamed MetadataDenormalizer into ShreddingTransformer #132

Merged
merged 1 commit into from
Feb 7, 2025
Merged

Conversation

epinzur
Copy link
Collaborator

@epinzur epinzur commented Feb 7, 2025

No description provided.

@epinzur epinzur requested a review from bjchambers February 7, 2025 10:30
@coveralls
Copy link

coveralls commented Feb 7, 2025

Pull Request Test Coverage Report for Build 13197993119

Details

  • 29 of 29 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-7.0%) to 93.016%

Totals Coverage Status
Change from base Build 13188782162: -7.0%
Covered Lines: 1493
Relevant Lines: 1580

💛 - Coveralls

Copy link

github-actions bot commented Feb 7, 2025

Test Results

    8 files  ±0    8 suites  ±0   2m 28s ⏱️ -3s
  400 tests ±0  400 ✅ ±0    0 💤 ±0  0 ❌ ±0 
1 600 runs  ±0  876 ✅ ±0  724 💤 ±0  0 ❌ ±0 

Results for commit 2ddcf0a. ± Comparison against base commit c61b19f.

This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.
packages.langchain-graph-retriever.tests.transformers.test_metadata_denormalizer ‑ test_revert_documents
packages.langchain-graph-retriever.tests.transformers.test_metadata_denormalizer ‑ test_restore_documents

@epinzur epinzur merged commit ec5b35f into main Feb 7, 2025
10 of 11 checks passed
@epinzur epinzur deleted the shredding branch February 7, 2025 10:35
@@ -15,7 +15,7 @@ __Supported__

__Collection Support__

: Indicates whether the store supports lists in metadata values or not. Stores which do not support it directly (:material-alert-circle:{.yellow}) can be used by applying the [MetadataDenormalizer][langchain_graph_retriever.transformers.metadata_denormalizer.MetadataDenormalizer] document transformer to documents before writing, which spreads the items of the collection into multiple metadata keys.
: Indicates whether the store supports lists in metadata values or not. Stores which do not support it directly (:material-alert-circle:{.yellow}) can be used by applying the [ShreddingTransformer][langchain_graph_retriever.transformers.ShreddingTransformer] document transformer to documents before writing, which spreads the items of the collection into multiple metadata keys.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be formatted as code (backticks)? Or should we not use them elsewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes I can add 'em

The [Adapter][graph_retriever.adapters.Adapter] interface may be implemented directly. For LangChain [VectorStores][langchain_core.vectorstores.base.VectorStore], [LangchainAdapter][langchain_graph_retriever.adapters.langchain.LangchainAdapter] and [ShreddedLangchainAdapter][langchain_graph_retriever.adapters.langchain.ShreddedLangchainAdapter] provide much of the necessary functionality.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It may make sense to be consistent on ShreddingTrannsformer vs. ShreddedLangchainAdapter (eg., ShreddingLangchainAdapter?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the adapters use already shredded documents. That is why I used Shredded instead of Shredding.

But I'll change to make consistent if you feel that is better.

from langchain_community.document_transformers.metadata_denormalizer import (
MetadataDenormalizer,
)
from langchain_community.document_transformers import ShreddingTransformer
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not believe this is part of langchain_community.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oops, good catch

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.

3 participants