-
Notifications
You must be signed in to change notification settings - Fork 25
Persist the corpus and make index loading deterministic #32
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,7 @@ | |
| import logging | ||
| import tempfile | ||
| import os | ||
| from typing import Dict, Any, Optional, Tuple | ||
| from typing import Dict, Any, List, Optional, Tuple | ||
|
|
||
| import torch | ||
| import safetensors | ||
|
|
@@ -37,9 +37,12 @@ | |
| # Constants for file names | ||
| CONFIG_FILE_NAME = "sentinel_local_index_config.json" | ||
| EMBEDDINGS_FILE_NAME = "embeddings.safetensors" | ||
| CORPUS_FILE_NAME = "corpus.json" # Optional; absent in indices saved before corpus support | ||
| # Storage keys - kept as positive/negative for backward compatibility | ||
| POSITIVE_EMBEDDINGS_KEY = "positive_embeddings" # Corresponds to rare class examples | ||
| NEGATIVE_EMBEDDINGS_KEY = "negative_embeddings" # Corresponds to common class examples | ||
| POSITIVE_CORPUS_KEY = "positive_corpus" | ||
| NEGATIVE_CORPUS_KEY = "negative_corpus" | ||
|
|
||
|
|
||
| def create_s3_transport_params( | ||
|
|
@@ -87,12 +90,38 @@ def _join_path(base_path: str, filename: str) -> str: | |
| return os.path.join(base_path, filename) | ||
|
|
||
|
|
||
| def _check_corpus_length( | ||
| name: str, corpus: Optional[List[str]], embeddings: Optional[torch.Tensor] | ||
| ) -> None: | ||
| """Raise if a corpus does not line up row-for-row with its embeddings. | ||
|
|
||
| Args: | ||
| name: Which corpus is being checked, used in the error message. | ||
| corpus: The corpus texts, or None when there is nothing to check. | ||
| embeddings: The embeddings the corpus is supposed to describe. | ||
|
|
||
| Raises: | ||
| ValueError: If the corpus and embeddings have different lengths. | ||
| """ | ||
| if corpus is None or embeddings is None: | ||
| return | ||
| if len(corpus) != embeddings.shape[0]: | ||
| raise ValueError( | ||
| f"{name}_corpus has {len(corpus)} entries but {name}_embeddings has " | ||
| f"{embeddings.shape[0]} rows. Saving a misaligned corpus would make " | ||
| f"explanations name the wrong text, so refusing to write it." | ||
| ) | ||
|
|
||
|
|
||
| def save_index( | ||
| path: str, | ||
| config: SavedIndexConfig, | ||
| positive_embeddings: torch.Tensor, # Represents rare class examples | ||
| negative_embeddings: torch.Tensor, # Represents common class examples | ||
| transport_params: Optional[Dict[str, Any]] = None, | ||
| *, | ||
| positive_corpus: Optional[List[str]] = None, | ||
| negative_corpus: Optional[List[str]] = None, | ||
| ) -> None: | ||
| """ | ||
| Save a SentinelLocalIndex to a path. | ||
|
|
@@ -103,7 +132,18 @@ def save_index( | |
| positive_embeddings: Tensor of positive (rare class) example embeddings. | ||
| negative_embeddings: Tensor of negative (common class) example embeddings. | ||
| transport_params: Optional transport parameters for smart_open. | ||
| positive_corpus: Optional texts behind the positive embeddings. Written to | ||
| an extra ``corpus.json`` so explanations survive a reload. | ||
| negative_corpus: Optional texts behind the negative embeddings. | ||
|
|
||
| Raises: | ||
| ValueError: If a corpus is supplied whose length does not match its embeddings. | ||
| """ | ||
| # Fail before writing anything: a misaligned corpus baked into the artifact is | ||
| # worse than no corpus, and the caller is right here and can fix it. | ||
| _check_corpus_length("positive", positive_corpus, positive_embeddings) | ||
| _check_corpus_length("negative", negative_corpus, negative_embeddings) | ||
|
|
||
| # Ensure directory exists for local paths | ||
| if not path.startswith("s3://") and not os.path.exists(path): | ||
| os.makedirs(path, exist_ok=True) | ||
|
|
@@ -146,6 +186,20 @@ def save_index( | |
| ) as f_out: | ||
| f_out.write(f_in.read()) | ||
|
|
||
| # The corpus is small text rather than a tensor blob, so smart_open handles both | ||
| # local and S3 destinations directly, the same way the config file does. | ||
| if positive_corpus is not None or negative_corpus is not None: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to require positive_corpus and/or negative_corpus? Otherwise, it seems we can overwrite the embeddings, but then if the corpuses aren't provided, then previously written corpuses would be used for explanations. Alternatively, if they are not supplied, we can write null corpuses?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Basically, positive_corpus and/or negative_corpus (text) are both option and it is added in this PR. We want to keep it this way and also work backward comparable. Good catch, this is a real bug. Save embeddings + corpus to a path, then save new embeddings to that same path without a corpus, and I will always write |
||
| corpus_path = _join_path(path, CORPUS_FILE_NAME) | ||
| LOG.info("Saving corpus to %s", corpus_path) | ||
| with smart_open.open(corpus_path, "w", transport_params=transport_params) as f: | ||
| json.dump( | ||
| { | ||
| POSITIVE_CORPUS_KEY: positive_corpus, | ||
| NEGATIVE_CORPUS_KEY: negative_corpus, | ||
| }, | ||
| f, | ||
| ) | ||
|
|
||
| LOG.info("Successfully saved index to %s", path) | ||
|
|
||
|
|
||
|
|
@@ -209,6 +263,48 @@ def _load_embeddings_from_file(file_path: str) -> Tuple[torch.Tensor, torch.Tens | |
| return positive_embeddings, negative_embeddings | ||
|
|
||
|
|
||
| def load_corpus( | ||
| path: str, transport_params: Optional[Dict[str, Any]] = None | ||
| ) -> Tuple[Optional[List[str]], Optional[List[str]]]: | ||
| """ | ||
| Load the corpus texts of an index, if it has any. | ||
|
|
||
| This is a separate function rather than extra return values on | ||
| :func:`load_index` because ``load_index`` is public API that callers unpack as a | ||
| 3-tuple; widening it would break them. The corpus is an optional, separate file, | ||
| so an optional, separate loader mirrors the format honestly. | ||
|
|
||
| Args: | ||
| path: Path where the index is stored. | ||
| transport_params: Optional transport parameters for smart_open. | ||
|
|
||
| Returns: | ||
| Tuple of (positive_corpus, negative_corpus). Either element may be None, and | ||
| both are None for indices saved without a corpus. | ||
|
|
||
| Raises: | ||
| json.JSONDecodeError: If the corpus file exists but is not valid JSON. A | ||
| corrupt artifact is a real problem and is not silently swallowed. | ||
| """ | ||
| corpus_path = _join_path(path, CORPUS_FILE_NAME) | ||
|
|
||
| try: | ||
| with smart_open.open(corpus_path, "r", transport_params=transport_params) as f: | ||
| payload = json.load(f) | ||
| except (FileNotFoundError, OSError, KeyError) as e: | ||
| # Every index saved before corpus support lacks this file. That is the normal | ||
| # case, not an error. smart_open surfaces a missing S3 key as OSError/KeyError. | ||
| LOG.info( | ||
| "No corpus file at %s (%s) - explanations will fall back to row numbers.", | ||
| corpus_path, | ||
| type(e).__name__, | ||
| ) | ||
| return None, None | ||
|
|
||
| LOG.info("Loaded corpus from %s", corpus_path) | ||
| return payload.get(POSITIVE_CORPUS_KEY), payload.get(NEGATIVE_CORPUS_KEY) | ||
|
|
||
|
|
||
| def load_index( | ||
| path: str, transport_params: Optional[Dict[str, Any]] = None | ||
| ) -> Tuple[SavedIndexConfig, torch.Tensor, torch.Tensor]: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to log an error and not write the corpuses if this is the case, rather than failing the entire run? Alternatively, we can at least write embeddings first, and then fail? Worried about calculating all the embeddings for nothing.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The worry about wasted compute is fair, but I'd keep the raise. Two reasons.
This check can only fire on a caller bug. In every build path we document, the corpus is the same list that was encoded:
so the lengths match by construction, and a mismatch means the caller subsampled one side or passed the wrong variable. That's deterministic: it fails identically on rerun. Logging and skipping the corpus would instead write an expensive artifact that has permanently lost its explanations, and we'd likely only notice much later when production explanations show row numbers instead of text. That also undercuts the invariant the rest of the module works to keep (
_corpus_if_alignedon load,_take_rowson subsetting).On writing the embeddings first and then failing, I'd push back harder, because it will cause inconsistent embedding and
corpus.json. It also rarely happens if run correctly.