Add LLM-guided embedding adaptation module; polish granularity calibration docs - #3
Merged
nevil-mathew merged 5 commits intoJul 4, 2026
Conversation
- run_benchmark.py: reproduces the README's TriTopic vs BERTopic/NMF/LDA benchmark table (20ng, BBC News, AG News, Arxiv) with a --quick synthetic smoke-test mode for CI. The file was previously referenced by the README but never committed because a blanket `run*` .gitignore rule silently swallowed it; added a `!run_benchmark.py` exception. - Verify quoted phrases in LLM-generated report-theme narratives against the source documents shown to the LLM (tritopic/utils/quote_verification.py), flagging quotes that can't be traced back to a real document so a fabricated participant quote doesn't silently ship in a qualitative research report. Surfaced via ReportTheme.unverified_quotes and an inline warning in export_report()'s Markdown output. - Add GitHub Actions CI: pytest on push/PR (py3.10/3.12), a fast benchmark harness smoke test, and a manually-triggered full benchmark reproduction job. - Fix README code samples using the nonexistent `n_topics_target` kwarg; the actual TriTopic constructor param is `n_topics`.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds a new ChangesLLM-Guided Embedding Adaptation
Benchmark Reproduction Script and CI Workflow
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant TriTopic
participant adapt_and_refit
participant EmbeddingAdapter
participant TripletBank
participant LLMLabeler
User->>TriTopic: adapt_embeddings_with_llm(labeler, config)
TriTopic->>adapt_and_refit: adapt_and_refit(model, labeler, config)
adapt_and_refit->>EmbeddingAdapter: collect_triplets(documents, embeddings, labels)
EmbeddingAdapter->>TripletBank: collect(labeler, documents, embeddings, labels)
TripletBank->>LLMLabeler: call_structured(prompt, schema)
LLMLabeler-->>TripletBank: judgment answers
TripletBank-->>EmbeddingAdapter: train/holdout judgments
adapt_and_refit->>EmbeddingAdapter: finetune(documents, embeddings, bank)
EmbeddingAdapter-->>adapt_and_refit: adapted embeddings
adapt_and_refit->>TriTopic: fit new TriTopic on adapted embeddings
adapt_and_refit-->>TriTopic: (new_model, report)
TriTopic-->>User: self (updated with adaptation_diagnostics_)
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
nevil-mathew
changed the base branch from
batch-clustering
to
llm-embedding-adaptation
July 4, 2026 09:27
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
1-81: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd an explicit
permissions:block (least privilege).No
permissions:block is set at the workflow or job level, so all jobs run with the default (potentially broad, e.g. read/write)GITHUB_TOKENpermissions. Since none of these jobs need to write to the repo or open PRs/issues, scope this down.🔒 Proposed fix
name: CI +permissions: + contents: read + on: push: branches: [main]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 1 - 81, Add an explicit least-privilege permissions block to the CI workflow so the default GITHUB_TOKEN scope is not broader than needed. Update the workflow-level configuration in ci.yml to set read-only access for the jobs, since the test, benchmark-smoke, and full-benchmark jobs only check out code, install dependencies, run commands, and upload artifacts; refer to the jobs test, benchmark-smoke, and full-benchmark when verifying nothing needs write access.Source: Linters/SAST tools
🧹 Nitpick comments (4)
.github/workflows/ci.yml (1)
56-81: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a
timeout-minutesguard onfull-benchmark.This job downloads real datasets (HF Hub) and sentence-transformer models with no timeout, so a network hang or slow HF fetch would let the job run indefinitely (up to the GitHub-imposed 6h default), consuming Actions minutes.
Proposed addition
full-benchmark: name: Full benchmark reproduction (manual) runs-on: ubuntu-latest + timeout-minutes: 60 needs: test🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 56 - 81, The full-benchmark job is missing a timeout guard, so a slow or stalled HF download can run far too long. Add a reasonable timeout-minutes setting to the full-benchmark job in ci.yml so the manual benchmark reproduction cannot consume Actions time indefinitely. Use the existing full-benchmark job definition as the place to apply it, keeping the rest of the steps unchanged.tritopic/adaptation/keyphrase.py (1)
150-152: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMerge the two encode calls to cut API round-trips in half.
- doc_emb = np.asarray(encoder.encode(documents), dtype=np.float64) - phrase_texts = ["; ".join(kws) if kws else "" for kws in keyphrases] - phrase_emb = np.asarray(encoder.encode(phrase_texts), dtype=np.float64) + phrase_texts = ["; ".join(kws) if kws else "" for kws in keyphrases] + all_emb = np.asarray(encoder.encode(documents + phrase_texts), dtype=np.float64) + doc_emb, phrase_emb = all_emb[: len(documents)], all_emb[len(documents) :]Useful for API-based encoders where each
.encode()call has fixed per-request overhead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tritopic/adaptation/keyphrase.py` around lines 150 - 152, The keyphrase adaptation flow makes two separate encoder.encode calls for documents and phrase_texts, which doubles request overhead for API-backed encoders. Update the logic in the keyphrase adaptation path to batch both inputs into a single encode invocation, then split the returned embeddings back into doc_emb and phrase_emb while preserving the existing np.asarray dtype handling.tritopic/adaptation/correction.py (1)
109-121: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy lift
batch_sizedoesn't actually batch LLM calls.Unlike
generate_keyphrases(which packsbatch_sizedocuments into a single prompt/call), herelabeler.call_structuredis invoked once per document inside the inner loop —batch_sizeonly affects iteration chunking, not the number of LLM calls. Formax_docs=200this means up to 200 individual calls regardless ofbatch_size, which is easy to misread as a cost/latency lever.Consider either implementing real multi-doc batching (like
generate_keyphrases) or renaming the parameter to avoid the misleading implication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tritopic/adaptation/correction.py` around lines 109 - 121, The current `batch_size` in `correction.py` only chunks iteration in `correct_topics` but still calls `labeler.call_structured` once per document, so it does not batch LLM requests. Either refactor this path to perform true multi-document batching similar to `generate_keyphrases`, or rename/remove `batch_size` in `correct_topics` (and any related call sites) so the API matches the actual per-document behavior and avoids misleading cost/latency expectations.tests/test_adaptation_keyphrase.py (1)
48-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for non-zero weight and empty-keyphrase docs.
test_keyphrase_expand_embeddings_weight_zero_reproduces_baseonly exercisesweight=0.0, which trivially returns the base embedding regardless of the blending logic. Consider adding a case withweight>0and a doc whosekeyphraseslist is empty, to catch the dilution issue flagged inkeyphrase.py.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_adaptation_keyphrase.py` around lines 48 - 64, The current tests only cover the weight=0.0 path in keyphrase_expand_embeddings, so add coverage that exercises the real blending logic with weight>0 and an input document whose keyphrases list is empty. Update test_keyphrase_expand_embeddings_weight_zero_reproduces_base or add a new test in tests/test_adaptation_keyphrase.py that calls keyphrase_expand_embeddings with a non-zero weight and verifies the empty-keyphrase document still matches the expected normalized base embedding, using the keyphrase_expand_embeddings helper and _DuckEncoder to locate the behavior in keyphrase.py.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 23: The checkout steps in the CI workflow currently persist the
GITHUB_TOKEN in git config for the rest of each job; update every
actions/checkout@v4 usage in the workflow to set persist-credentials to false.
Apply this change to each checkout step in the jobs that use it, keeping the
rest of the job logic unchanged.
In `@run_benchmark.py`:
- Around line 161-176: The embedding cache key in embed() only uses the dataset
key and text count, so reruns with different sample seeds can reuse embeddings
for a different sampled subset. Update the cache_path construction to
incorporate sample_seed (and any other sampling-discriminating inputs) so cached
embeddings always match the current docs/labels. Make sure the embed() call site
in run_benchmark() passes the seed-based identifier consistently, and keep the
cache naming stable so repeated runs with the same seed still hit the cache.
In `@tritopic/adaptation/adapter.py`:
- Around line 188-214: The explicit adapter_mode="finetune" path in
_resolve_mode currently bypasses the sentence-transformers Trainer-API
dependency check, so it can fail later with a raw import error instead of the
intended actionable message. Update _resolve_mode in the Adapter class to run
the same accelerate/datasets/sentence_transformers version check before
returning finetune, and raise or fall back with the same user-facing guidance
used by the auto branch.
- Around line 324-334: The linear path in EmbeddingAdapter.encode ignores the
normalize argument and always returns normalized vectors because it delegates to
LinearAdapter.transform() without passing the flag. Update encode() (and, if
needed, LinearAdapter.transform()) so the normalize parameter is threaded
through consistently in linear mode, or explicitly document in the
EmbeddingAdapter and linear adapter methods that linear mode always normalizes
regardless of the caller’s request.
In `@tritopic/adaptation/correction.py`:
- Around line 124-143: The reassignment flow in correction should not recompute
topic statistics when a topic becomes empty. In the logic that updates labels
and then calls model._compute_topic_centroids() and
model._compute_probabilities(), add a guard in the correction/applied path to
detect topics with no assigned documents (for example by checking each topic
mask before recomputing) and skip or drop those empty topics instead of
averaging an empty slice. Use the existing symbols rows, applied, dry_run, and
the model._compute_topic_centroids()/model._compute_probabilities() calls to
locate and update this behavior.
In `@tritopic/adaptation/keyphrase.py`:
- Around line 85-91: The cache-loading block in generate_keyphrases is too
brittle: a single malformed JSONL record will abort the whole read. Update the
loop that opens cache_path and parses each line so it skips invalid or partial
entries instead of raising, while still loading the rest of the cache; use the
existing cache[rec["hash"]] assignment path and add per-line error handling
around json.loads for resilience.
- Around line 150-159: The blending logic in the keyphrase embedding path
applies `weight` even when `keyphrases[i]` is empty, which causes `encode("")`
to distort those documents. Update the keyphrase composition in
`tritopic/adaptation/keyphrase.py` so that the `combined` embedding only blends
`doc_emb` with `phrase_emb` for rows where `keyphrases` is non-empty, and leaves
`doc_emb` unchanged when the parsed keyphrase list is empty. Use the existing
`encoder.encode`, `phrase_texts`, and `combined` flow to locate the fix.
In `@tritopic/adaptation/pipeline.py`:
- Around line 123-126: The refit path in adapt_and_refit is dropping the
metadata view because TriTopic.fit only consumes metadata for that invocation
and does not persist it on the model instance. Update the TriTopic.fit call on
new_model to pass through the original metadata frame when use_metadata_view was
enabled, using the existing model state or adaptation inputs to locate it; if
metadata cannot be recovered, add an explicit warning or error instead of
silently refitting without it.
In `@tritopic/core/model.py`:
- Around line 1930-1934: The `AdaptationConfig` type used in
`TriTopic.adapt_embeddings_with_llm` is only a quoted annotation, so Ruff and
type checkers cannot resolve it. Add a `TYPE_CHECKING` import near the top of
`tritopic/core/model.py` and import `AdaptationConfig` from
`tritopic.adaptation.config` inside that block so the annotation can be resolved
without introducing a runtime import. Keep the existing
`adapt_embeddings_with_llm` signature unchanged apart from making the type
available for static analysis.
- Around line 1980-1993: Preserve the explicit topic count during the refit flow
in adapt_embeddings_with_llm()/adapt_and_refit(), since rebuilding TriTopic from
model.config alone drops model.n_topics back to "auto". Update the
adapt_and_refit path to carry n_topics through when constructing the new model,
either by passing model.n_topics explicitly or by copying it into the refit
config before creating the replacement TriTopic, so the returned new_model keeps
the intended topic target.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 1-81: Add an explicit least-privilege permissions block to the CI
workflow so the default GITHUB_TOKEN scope is not broader than needed. Update
the workflow-level configuration in ci.yml to set read-only access for the jobs,
since the test, benchmark-smoke, and full-benchmark jobs only check out code,
install dependencies, run commands, and upload artifacts; refer to the jobs
test, benchmark-smoke, and full-benchmark when verifying nothing needs write
access.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 56-81: The full-benchmark job is missing a timeout guard, so a
slow or stalled HF download can run far too long. Add a reasonable
timeout-minutes setting to the full-benchmark job in ci.yml so the manual
benchmark reproduction cannot consume Actions time indefinitely. Use the
existing full-benchmark job definition as the place to apply it, keeping the
rest of the steps unchanged.
In `@tests/test_adaptation_keyphrase.py`:
- Around line 48-64: The current tests only cover the weight=0.0 path in
keyphrase_expand_embeddings, so add coverage that exercises the real blending
logic with weight>0 and an input document whose keyphrases list is empty. Update
test_keyphrase_expand_embeddings_weight_zero_reproduces_base or add a new test
in tests/test_adaptation_keyphrase.py that calls keyphrase_expand_embeddings
with a non-zero weight and verifies the empty-keyphrase document still matches
the expected normalized base embedding, using the keyphrase_expand_embeddings
helper and _DuckEncoder to locate the behavior in keyphrase.py.
In `@tritopic/adaptation/correction.py`:
- Around line 109-121: The current `batch_size` in `correction.py` only chunks
iteration in `correct_topics` but still calls `labeler.call_structured` once per
document, so it does not batch LLM requests. Either refactor this path to
perform true multi-document batching similar to `generate_keyphrases`, or
rename/remove `batch_size` in `correct_topics` (and any related call sites) so
the API matches the actual per-document behavior and avoids misleading
cost/latency expectations.
In `@tritopic/adaptation/keyphrase.py`:
- Around line 150-152: The keyphrase adaptation flow makes two separate
encoder.encode calls for documents and phrase_texts, which doubles request
overhead for API-backed encoders. Update the logic in the keyphrase adaptation
path to batch both inputs into a single encode invocation, then split the
returned embeddings back into doc_emb and phrase_emb while preserving the
existing np.asarray dtype handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43154418-103e-401d-8a36-944ccb1d6142
📒 Files selected for processing (27)
.github/workflows/ci.yml.gitignoreREADME.mdbenchmarks/adaptation_quality_report.pynotebooks/embedding_adaptation_demo.ipynbpyproject.tomlrun_benchmark.pytests/test_adaptation_correction.pytests/test_adaptation_evaluation.pytests/test_adaptation_keyphrase.pytests/test_adaptation_linear.pytests/test_adaptation_pipeline.pytests/test_adaptation_triplets.pytests/test_quote_verification.pytests/test_report_themes.pytritopic/adaptation/__init__.pytritopic/adaptation/_compat.pytritopic/adaptation/adapter.pytritopic/adaptation/config.pytritopic/adaptation/correction.pytritopic/adaptation/evaluation.pytritopic/adaptation/keyphrase.pytritopic/adaptation/pipeline.pytritopic/adaptation/triplets.pytritopic/core/model.pytritopic/utils/__init__.pytritopic/utils/quote_verification.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
1-81: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd an explicit
permissions:block (least privilege).No
permissions:block is set at the workflow or job level, so all jobs run with the default (potentially broad, e.g. read/write)GITHUB_TOKENpermissions. Since none of these jobs need to write to the repo or open PRs/issues, scope this down.🔒 Proposed fix
name: CI +permissions: + contents: read + on: push: branches: [main]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 1 - 81, Add an explicit least-privilege permissions block to the CI workflow so the default GITHUB_TOKEN scope is not broader than needed. Update the workflow-level configuration in ci.yml to set read-only access for the jobs, since the test, benchmark-smoke, and full-benchmark jobs only check out code, install dependencies, run commands, and upload artifacts; refer to the jobs test, benchmark-smoke, and full-benchmark when verifying nothing needs write access.Source: Linters/SAST tools
🧹 Nitpick comments (4)
.github/workflows/ci.yml (1)
56-81: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a
timeout-minutesguard onfull-benchmark.This job downloads real datasets (HF Hub) and sentence-transformer models with no timeout, so a network hang or slow HF fetch would let the job run indefinitely (up to the GitHub-imposed 6h default), consuming Actions minutes.
Proposed addition
full-benchmark: name: Full benchmark reproduction (manual) runs-on: ubuntu-latest + timeout-minutes: 60 needs: test🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 56 - 81, The full-benchmark job is missing a timeout guard, so a slow or stalled HF download can run far too long. Add a reasonable timeout-minutes setting to the full-benchmark job in ci.yml so the manual benchmark reproduction cannot consume Actions time indefinitely. Use the existing full-benchmark job definition as the place to apply it, keeping the rest of the steps unchanged.tritopic/adaptation/keyphrase.py (1)
150-152: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMerge the two encode calls to cut API round-trips in half.
- doc_emb = np.asarray(encoder.encode(documents), dtype=np.float64) - phrase_texts = ["; ".join(kws) if kws else "" for kws in keyphrases] - phrase_emb = np.asarray(encoder.encode(phrase_texts), dtype=np.float64) + phrase_texts = ["; ".join(kws) if kws else "" for kws in keyphrases] + all_emb = np.asarray(encoder.encode(documents + phrase_texts), dtype=np.float64) + doc_emb, phrase_emb = all_emb[: len(documents)], all_emb[len(documents) :]Useful for API-based encoders where each
.encode()call has fixed per-request overhead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tritopic/adaptation/keyphrase.py` around lines 150 - 152, The keyphrase adaptation flow makes two separate encoder.encode calls for documents and phrase_texts, which doubles request overhead for API-backed encoders. Update the logic in the keyphrase adaptation path to batch both inputs into a single encode invocation, then split the returned embeddings back into doc_emb and phrase_emb while preserving the existing np.asarray dtype handling.tritopic/adaptation/correction.py (1)
109-121: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy lift
batch_sizedoesn't actually batch LLM calls.Unlike
generate_keyphrases(which packsbatch_sizedocuments into a single prompt/call), herelabeler.call_structuredis invoked once per document inside the inner loop —batch_sizeonly affects iteration chunking, not the number of LLM calls. Formax_docs=200this means up to 200 individual calls regardless ofbatch_size, which is easy to misread as a cost/latency lever.Consider either implementing real multi-doc batching (like
generate_keyphrases) or renaming the parameter to avoid the misleading implication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tritopic/adaptation/correction.py` around lines 109 - 121, The current `batch_size` in `correction.py` only chunks iteration in `correct_topics` but still calls `labeler.call_structured` once per document, so it does not batch LLM requests. Either refactor this path to perform true multi-document batching similar to `generate_keyphrases`, or rename/remove `batch_size` in `correct_topics` (and any related call sites) so the API matches the actual per-document behavior and avoids misleading cost/latency expectations.tests/test_adaptation_keyphrase.py (1)
48-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for non-zero weight and empty-keyphrase docs.
test_keyphrase_expand_embeddings_weight_zero_reproduces_baseonly exercisesweight=0.0, which trivially returns the base embedding regardless of the blending logic. Consider adding a case withweight>0and a doc whosekeyphraseslist is empty, to catch the dilution issue flagged inkeyphrase.py.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_adaptation_keyphrase.py` around lines 48 - 64, The current tests only cover the weight=0.0 path in keyphrase_expand_embeddings, so add coverage that exercises the real blending logic with weight>0 and an input document whose keyphrases list is empty. Update test_keyphrase_expand_embeddings_weight_zero_reproduces_base or add a new test in tests/test_adaptation_keyphrase.py that calls keyphrase_expand_embeddings with a non-zero weight and verifies the empty-keyphrase document still matches the expected normalized base embedding, using the keyphrase_expand_embeddings helper and _DuckEncoder to locate the behavior in keyphrase.py.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 23: The checkout steps in the CI workflow currently persist the
GITHUB_TOKEN in git config for the rest of each job; update every
actions/checkout@v4 usage in the workflow to set persist-credentials to false.
Apply this change to each checkout step in the jobs that use it, keeping the
rest of the job logic unchanged.
In `@run_benchmark.py`:
- Around line 161-176: The embedding cache key in embed() only uses the dataset
key and text count, so reruns with different sample seeds can reuse embeddings
for a different sampled subset. Update the cache_path construction to
incorporate sample_seed (and any other sampling-discriminating inputs) so cached
embeddings always match the current docs/labels. Make sure the embed() call site
in run_benchmark() passes the seed-based identifier consistently, and keep the
cache naming stable so repeated runs with the same seed still hit the cache.
In `@tritopic/adaptation/adapter.py`:
- Around line 188-214: The explicit adapter_mode="finetune" path in
_resolve_mode currently bypasses the sentence-transformers Trainer-API
dependency check, so it can fail later with a raw import error instead of the
intended actionable message. Update _resolve_mode in the Adapter class to run
the same accelerate/datasets/sentence_transformers version check before
returning finetune, and raise or fall back with the same user-facing guidance
used by the auto branch.
- Around line 324-334: The linear path in EmbeddingAdapter.encode ignores the
normalize argument and always returns normalized vectors because it delegates to
LinearAdapter.transform() without passing the flag. Update encode() (and, if
needed, LinearAdapter.transform()) so the normalize parameter is threaded
through consistently in linear mode, or explicitly document in the
EmbeddingAdapter and linear adapter methods that linear mode always normalizes
regardless of the caller’s request.
In `@tritopic/adaptation/correction.py`:
- Around line 124-143: The reassignment flow in correction should not recompute
topic statistics when a topic becomes empty. In the logic that updates labels
and then calls model._compute_topic_centroids() and
model._compute_probabilities(), add a guard in the correction/applied path to
detect topics with no assigned documents (for example by checking each topic
mask before recomputing) and skip or drop those empty topics instead of
averaging an empty slice. Use the existing symbols rows, applied, dry_run, and
the model._compute_topic_centroids()/model._compute_probabilities() calls to
locate and update this behavior.
In `@tritopic/adaptation/keyphrase.py`:
- Around line 85-91: The cache-loading block in generate_keyphrases is too
brittle: a single malformed JSONL record will abort the whole read. Update the
loop that opens cache_path and parses each line so it skips invalid or partial
entries instead of raising, while still loading the rest of the cache; use the
existing cache[rec["hash"]] assignment path and add per-line error handling
around json.loads for resilience.
- Around line 150-159: The blending logic in the keyphrase embedding path
applies `weight` even when `keyphrases[i]` is empty, which causes `encode("")`
to distort those documents. Update the keyphrase composition in
`tritopic/adaptation/keyphrase.py` so that the `combined` embedding only blends
`doc_emb` with `phrase_emb` for rows where `keyphrases` is non-empty, and leaves
`doc_emb` unchanged when the parsed keyphrase list is empty. Use the existing
`encoder.encode`, `phrase_texts`, and `combined` flow to locate the fix.
In `@tritopic/adaptation/pipeline.py`:
- Around line 123-126: The refit path in adapt_and_refit is dropping the
metadata view because TriTopic.fit only consumes metadata for that invocation
and does not persist it on the model instance. Update the TriTopic.fit call on
new_model to pass through the original metadata frame when use_metadata_view was
enabled, using the existing model state or adaptation inputs to locate it; if
metadata cannot be recovered, add an explicit warning or error instead of
silently refitting without it.
In `@tritopic/core/model.py`:
- Around line 1930-1934: The `AdaptationConfig` type used in
`TriTopic.adapt_embeddings_with_llm` is only a quoted annotation, so Ruff and
type checkers cannot resolve it. Add a `TYPE_CHECKING` import near the top of
`tritopic/core/model.py` and import `AdaptationConfig` from
`tritopic.adaptation.config` inside that block so the annotation can be resolved
without introducing a runtime import. Keep the existing
`adapt_embeddings_with_llm` signature unchanged apart from making the type
available for static analysis.
- Around line 1980-1993: Preserve the explicit topic count during the refit flow
in adapt_embeddings_with_llm()/adapt_and_refit(), since rebuilding TriTopic from
model.config alone drops model.n_topics back to "auto". Update the
adapt_and_refit path to carry n_topics through when constructing the new model,
either by passing model.n_topics explicitly or by copying it into the refit
config before creating the replacement TriTopic, so the returned new_model keeps
the intended topic target.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 1-81: Add an explicit least-privilege permissions block to the CI
workflow so the default GITHUB_TOKEN scope is not broader than needed. Update
the workflow-level configuration in ci.yml to set read-only access for the jobs,
since the test, benchmark-smoke, and full-benchmark jobs only check out code,
install dependencies, run commands, and upload artifacts; refer to the jobs
test, benchmark-smoke, and full-benchmark when verifying nothing needs write
access.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 56-81: The full-benchmark job is missing a timeout guard, so a
slow or stalled HF download can run far too long. Add a reasonable
timeout-minutes setting to the full-benchmark job in ci.yml so the manual
benchmark reproduction cannot consume Actions time indefinitely. Use the
existing full-benchmark job definition as the place to apply it, keeping the
rest of the steps unchanged.
In `@tests/test_adaptation_keyphrase.py`:
- Around line 48-64: The current tests only cover the weight=0.0 path in
keyphrase_expand_embeddings, so add coverage that exercises the real blending
logic with weight>0 and an input document whose keyphrases list is empty. Update
test_keyphrase_expand_embeddings_weight_zero_reproduces_base or add a new test
in tests/test_adaptation_keyphrase.py that calls keyphrase_expand_embeddings
with a non-zero weight and verifies the empty-keyphrase document still matches
the expected normalized base embedding, using the keyphrase_expand_embeddings
helper and _DuckEncoder to locate the behavior in keyphrase.py.
In `@tritopic/adaptation/correction.py`:
- Around line 109-121: The current `batch_size` in `correction.py` only chunks
iteration in `correct_topics` but still calls `labeler.call_structured` once per
document, so it does not batch LLM requests. Either refactor this path to
perform true multi-document batching similar to `generate_keyphrases`, or
rename/remove `batch_size` in `correct_topics` (and any related call sites) so
the API matches the actual per-document behavior and avoids misleading
cost/latency expectations.
In `@tritopic/adaptation/keyphrase.py`:
- Around line 150-152: The keyphrase adaptation flow makes two separate
encoder.encode calls for documents and phrase_texts, which doubles request
overhead for API-backed encoders. Update the logic in the keyphrase adaptation
path to batch both inputs into a single encode invocation, then split the
returned embeddings back into doc_emb and phrase_emb while preserving the
existing np.asarray dtype handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43154418-103e-401d-8a36-944ccb1d6142
📒 Files selected for processing (27)
.github/workflows/ci.yml.gitignoreREADME.mdbenchmarks/adaptation_quality_report.pynotebooks/embedding_adaptation_demo.ipynbpyproject.tomlrun_benchmark.pytests/test_adaptation_correction.pytests/test_adaptation_evaluation.pytests/test_adaptation_keyphrase.pytests/test_adaptation_linear.pytests/test_adaptation_pipeline.pytests/test_adaptation_triplets.pytests/test_quote_verification.pytests/test_report_themes.pytritopic/adaptation/__init__.pytritopic/adaptation/_compat.pytritopic/adaptation/adapter.pytritopic/adaptation/config.pytritopic/adaptation/correction.pytritopic/adaptation/evaluation.pytritopic/adaptation/keyphrase.pytritopic/adaptation/pipeline.pytritopic/adaptation/triplets.pytritopic/core/model.pytritopic/utils/__init__.pytritopic/utils/quote_verification.py
🛑 Comments failed to post (10)
.github/workflows/ci.yml (1)
23-23: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Set
persist-credentials: falseon checkout steps.None of the three
actions/checkout@v4steps setpersist-credentials: false, so theGITHUB_TOKENremains persisted in the local git config for the remainder of each job. Since none of these jobs push to the repo, disabling persistence reduces the blast radius if a later step in the job is compromised (e.g., via a malicious dependency pulled duringpip install).🔒 Proposed fix
- uses: actions/checkout@v4 + with: + persist-credentials: falseAlso applies to: 42-42, 62-62
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 23-23: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 23, The checkout steps in the CI workflow currently persist the GITHUB_TOKEN in git config for the rest of each job; update every actions/checkout@v4 usage in the workflow to set persist-credentials to false. Apply this change to each checkout step in the jobs that use it, keeping the rest of the job logic unchanged.Source: Linters/SAST tools
run_benchmark.py (1)
161-176: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Embedding cache key ignores
--sample-seed, risking stale/mismatched embeddings.
cache_pathis keyed only by datasetkeyandlen(texts)(Line 164). If a user reruns with a different--sample-seedbut the dataset happens to subsample to the same document count,embed()will silently return embeddings computed for the previous (different) subset of documents — whiledocs/true_labelsreflect the new subset. This desyncs embeddings from documents/labels and corrupts the benchmark's NMI/coherence numbers without any warning, undermining the stated purpose of "reproduc[ing] the headline numbers" (Lines 5-6).🛠️ Proposed fix: include sample_seed in the cache key
-def embed(key: str, texts: list[str]) -> np.ndarray: +def embed(key: str, texts: list[str], sample_seed: int) -> np.ndarray: """all-MiniLM-L6-v2 embeddings, cached per dataset to benchmarks/.cache/.""" CACHE_DIR.mkdir(parents=True, exist_ok=True) - cache_path = CACHE_DIR / f"{key}_n{len(texts)}_emb.npy" + cache_path = CACHE_DIR / f"{key}_n{len(texts)}_seed{sample_seed}_emb.npy"- embeddings = embed(key, docs) + embeddings = embed(key, docs, sample_seed)Also applies to: 280-280
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@run_benchmark.py` around lines 161 - 176, The embedding cache key in embed() only uses the dataset key and text count, so reruns with different sample seeds can reuse embeddings for a different sampled subset. Update the cache_path construction to incorporate sample_seed (and any other sampling-discriminating inputs) so cached embeddings always match the current docs/labels. Make sure the embed() call site in run_benchmark() passes the seed-based identifier consistently, and keep the cache naming stable so repeated runs with the same seed still hit the cache.tritopic/adaptation/adapter.py (2)
188-214: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard explicit
adapter_mode="finetune"with the Trainer-API dependency check
mode == "finetune"returns before thesentence-transformers >= 3guard, so explicit finetune can still fall through to a raw import error later. Mirror the same check here so it fails with the same actionable message asauto.🧰 Tools
🪛 Ruff (0.15.20)
[warning] 208-208: No explicit
stacklevelkeyword argument foundSet
stacklevel=2(B028)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tritopic/adaptation/adapter.py` around lines 188 - 214, The explicit adapter_mode="finetune" path in _resolve_mode currently bypasses the sentence-transformers Trainer-API dependency check, so it can fail later with a raw import error instead of the intended actionable message. Update _resolve_mode in the Adapter class to run the same accelerate/datasets/sentence_transformers version check before returning finetune, and raise or fall back with the same user-facing guidance used by the auto branch.
324-334: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## adapter outline\n' ast-grep outline tritopic/adaptation/adapter.py --view expanded || true printf '\n## relevant slices\n' sed -n '100,150p' tritopic/adaptation/adapter.py printf '\n---\n' sed -n '300,360p' tritopic/adaptation/adapter.py printf '\n## search for normalize=False and encode call sites\n' rg -n "normalize\s*=\s*False|\.encode\(" tritopic -g '!**/__pycache__/**' || true printf '\n## search for EmbeddingAdapter usage\n' rg -n "EmbeddingAdapter|adapt_embeddings_with_llm|base_encoder" tritopic -g '!**/__pycache__/**' || trueRepository: nevil-mathew/topic-extraction-poc
Length of output: 10713
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## pipeline slice\n' sed -n '70,170p' tritopic/adaptation/pipeline.py printf '\n## adapt_embeddings_with_llm slice\n' sed -n '1930,2005p' tritopic/core/model.py printf '\n## adapter docs / type hints around encode\n' sed -n '1,40p' tritopic/adaptation/adapter.py printf '\n---\n' sed -n '130,170p' tritopic/adaptation/evaluation.py printf '\n## any normalize=False mentions in repo\n' rg -n "normalize\s*=\s*False" . -g '!**/__pycache__/**' || trueRepository: nevil-mathew/topic-extraction-poc
Length of output: 10275
normalize=Falseis ignored in linear mode
EmbeddingAdapter.encode(..., normalize=False)still returns unit-length vectors on the linear path becauseLinearAdapter.transform()always normalizes. Either threadnormalizethrough that backend or document that linear mode always normalizes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tritopic/adaptation/adapter.py` around lines 324 - 334, The linear path in EmbeddingAdapter.encode ignores the normalize argument and always returns normalized vectors because it delegates to LinearAdapter.transform() without passing the flag. Update encode() (and, if needed, LinearAdapter.transform()) so the normalize parameter is threaded through consistently in linear mode, or explicitly document in the EmbeddingAdapter and linear adapter methods that linear mode always normalizes regardless of the caller’s request.tritopic/adaptation/correction.py (1)
124-143: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n -B3 -A20 'def _compute_topic_centroids' tritopic/core/model.pyRepository: nevil-mathew/topic-extraction-poc
Length of output: 1315
🏁 Script executed:
#!/bin/bash sed -n '1022,1065p' tritopic/core/model.pyRepository: nevil-mathew/topic-extraction-poc
Length of output: 1892
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the reassignment flow and any topic cleanup around it. sed -n '1,220p' tritopic/adaptation/correction.py printf '\n---\n' # Find where topics are added/removed or labels are normalized. rg -n "remove.*topic|drop.*topic|empty topic|topics_|labels_\[doc_idx\]|_compute_topic_centroids|_compute_probabilities" tritopic/core tritopic/adaptationRepository: nevil-mathew/topic-extraction-poc
Length of output: 12844
🏁 Script executed:
#!/bin/bash python3 - <<'PY' import numpy as np import warnings x = np.empty((0, 3)) print("mean:") with warnings.catch_warnings(record=True) as w: warnings.simplefilter("always") try: y = x.mean(axis=0) print("result", y.tolist()) except Exception as e: print(type(e).__name__, str(e)) print("warnings", [str(wi.message) for wi in w]) print("average:") with warnings.catch_warnings(record=True) as w: warnings.simplefilter("always") try: y = np.average(x, axis=0) print("result", y.tolist()) except Exception as e: print(type(e).__name__, str(e)) print("warnings", [str(wi.message) for wi in w]) PYRepository: nevil-mathew/topic-extraction-poc
Length of output: 376
🏁 Script executed:
#!/bin/bash sed -n '828,920p' tritopic/core/model.pyRepository: nevil-mathew/topic-extraction-poc
Length of output: 3979
Guard against empty topics before refreshing centroids
If this reassignment removes the last document from a topic,
_compute_topic_centroids()will average an empty slice and producenancentroids;_compute_probabilities()then propagates invalid values through the model. Skip/drop empty topics, or guardmask.any()before recomputing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tritopic/adaptation/correction.py` around lines 124 - 143, The reassignment flow in correction should not recompute topic statistics when a topic becomes empty. In the logic that updates labels and then calls model._compute_topic_centroids() and model._compute_probabilities(), add a guard in the correction/applied path to detect topics with no assigned documents (for example by checking each topic mask before recomputing) and skip or drop those empty topics instead of averaging an empty slice. Use the existing symbols rows, applied, dry_run, and the model._compute_topic_centroids()/model._compute_probabilities() calls to locate and update this behavior.tritopic/adaptation/keyphrase.py (2)
85-91: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Cache load isn't resilient to a corrupted/partial line.
A single malformed JSONL line (e.g. from a prior interrupted write) will raise and abort
generate_keyphrasesentirely instead of just skipping that record.🛡️ Proposed fix
line = line.strip() if line: - rec = json.loads(line) - cache[rec["hash"]] = rec["keyphrases"] + try: + rec = json.loads(line) + cache[rec["hash"]] = rec["keyphrases"] + except (json.JSONDecodeError, KeyError): + continue📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if cache_path and Path(cache_path).exists(): with open(cache_path) as f: for line in f: line = line.strip() if line: try: rec = json.loads(line) cache[rec["hash"]] = rec["keyphrases"] except (json.JSONDecodeError, KeyError): continue🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 85-85: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(cache_path)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').(open-filename-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tritopic/adaptation/keyphrase.py` around lines 85 - 91, The cache-loading block in generate_keyphrases is too brittle: a single malformed JSONL record will abort the whole read. Update the loop that opens cache_path and parses each line so it skips invalid or partial entries instead of raising, while still loading the rest of the cache; use the existing cache[rec["hash"]] assignment path and add per-line error handling around json.loads for resilience.
150-159: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Docs with empty keyphrases get diluted toward
encode("").When
keyphrases[i]is empty,phrase_texts[i]becomes"", yetcombined = (1-weight)*doc_emb + weight*phrase_embstill applies the fullweight— corrupting the embedding for exactly the docs where the LLM lever contributed nothing. This can happen systematically ifgenerate_keyphrasesparsing fails for a batch (see_parse_keyphrase_responsefallback to[]).🐛 Proposed fix — skip blending when a doc has no keyphrases
doc_emb = np.asarray(encoder.encode(documents), dtype=np.float64) phrase_texts = ["; ".join(kws) if kws else "" for kws in keyphrases] phrase_emb = np.asarray(encoder.encode(phrase_texts), dtype=np.float64) - combined = (1 - weight) * doc_emb + weight * phrase_emb + has_phrases = np.array([bool(kws) for kws in keyphrases], dtype=bool)[:, None] + effective_weight = np.where(has_phrases, weight, 0.0) + combined = (1 - effective_weight) * doc_emb + effective_weight * phrase_emb if not normalize: return combined📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.doc_emb = np.asarray(encoder.encode(documents), dtype=np.float64) phrase_texts = ["; ".join(kws) if kws else "" for kws in keyphrases] phrase_emb = np.asarray(encoder.encode(phrase_texts), dtype=np.float64) has_phrases = np.array([bool(kws) for kws in keyphrases], dtype=bool)[:, None] effective_weight = np.where(has_phrases, weight, 0.0) combined = (1 - effective_weight) * doc_emb + effective_weight * phrase_emb if not normalize: return combined norms = np.linalg.norm(combined, axis=1, keepdims=True) norms = np.where(norms == 0, 1.0, norms) return combined / norms🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tritopic/adaptation/keyphrase.py` around lines 150 - 159, The blending logic in the keyphrase embedding path applies `weight` even when `keyphrases[i]` is empty, which causes `encode("")` to distort those documents. Update the keyphrase composition in `tritopic/adaptation/keyphrase.py` so that the `combined` embedding only blends `doc_emb` with `phrase_emb` for rows where `keyphrases` is non-empty, and leaves `doc_emb` unchanged when the parsed keyphrase list is empty. Use the existing `encoder.encode`, `phrase_texts`, and `combined` flow to locate the fix.tritopic/adaptation/pipeline.py (1)
123-126: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Confirm whether TriTopic persists metadata after fit() for reuse. rg -n 'self\.metadata_' tritopic/core/model.py rg -n -A3 'use_metadata_view' tritopic/core/model.py | head -60Repository: nevil-mathew/topic-extraction-poc
Length of output: 770
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the relevant files and inspect the fit/adapt flow around metadata handling. ast-grep outline tritopic/core/model.py --view expanded || true ast-grep outline tritopic/adaptation/pipeline.py --view expanded || true printf '\n--- model.py: fit-related sections ---\n' rg -n -A40 -B20 'def fit\(|def fit_transform\(|use_metadata_view|metadata' tritopic/core/model.py printf '\n--- pipeline.py: adapt/refit sections ---\n' rg -n -A40 -B20 'adapt_and_refit|new_model\.fit\(|metadata' tritopic/adaptation/pipeline.pyRepository: nevil-mathew/topic-extraction-poc
Length of output: 46021
Forward metadata into the refit
TriTopic.fit()only usesmetadatafor that call; it isn’t kept on the instance. As written,adapt_and_refit()will drop the metadata view on refit whenuse_metadata_view=Truewas part of the original fit. Thread the metadata frame through this path, or warn/error when it’s unavailable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tritopic/adaptation/pipeline.py` around lines 123 - 126, The refit path in adapt_and_refit is dropping the metadata view because TriTopic.fit only consumes metadata for that invocation and does not persist it on the model instance. Update the TriTopic.fit call on new_model to pass through the original metadata frame when use_metadata_view was enabled, using the existing model state or adaptation inputs to locate it; if metadata cannot be recovered, add an explicit warning or error instead of silently refitting without it.tritopic/core/model.py (2)
1930-1934: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
AdaptationConfigforward reference is unresolved in this file.Ruff flags
AdaptationConfigas undefined (F821) — it's only referenced as a quoted string annotation and never imported (not even underTYPE_CHECKING), so static type checkers/IDEs can't resolve it even though it's harmless at runtime.🔧 Suggested fix (add near the top-level imports)
from typing import TYPE_CHECKING if TYPE_CHECKING: from tritopic.adaptation.config import AdaptationConfig🧰 Tools
🪛 Ruff (0.15.20)
[error] 1933-1933: Undefined name
AdaptationConfig(F821)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tritopic/core/model.py` around lines 1930 - 1934, The `AdaptationConfig` type used in `TriTopic.adapt_embeddings_with_llm` is only a quoted annotation, so Ruff and type checkers cannot resolve it. Add a `TYPE_CHECKING` import near the top of `tritopic/core/model.py` and import `AdaptationConfig` from `tritopic.adaptation.config` inside that block so the annotation can be resolved without introducing a runtime import. Keep the existing `adapt_embeddings_with_llm` signature unchanged apart from making the type available for static analysis.Source: Linters/SAST tools
1980-1993: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n "new_model = TriTopic" tritopic/adaptation/pipeline.py -A3 -B3 rg -n "n_topics" tritopic/adaptation/pipeline.pyRepository: nevil-mathew/topic-extraction-poc
Length of output: 422
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the TriTopic constructor and any topic-count resolution logic. ast-grep outline tritopic/core/model.py --view expanded | sed -n '1,220p' # Read the constructor and nearby helpers around n_topics handling. sed -n '1,260p' tritopic/core/model.py # Find where n_topics is stored, resolved, or passed through config. rg -n "n_topics|_auto_resolve_topic_count|config\.n_topics|setattr\(.*n_topics|self\.n_topics" tritopic/core/model.py tritopic/adaptation/pipeline.py -A3 -B3Repository: nevil-mathew/topic-extraction-poc
Length of output: 31149
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the refit helper and its call path. sed -n '1,220p' tritopic/adaptation/pipeline.py # Show the adapt_embeddings_with_llm call site for context. sed -n '1930,2005p' tritopic/core/model.pyRepository: nevil-mathew/topic-extraction-poc
Length of output: 9092
Preserve
n_topicswhen refitting adapted models.adapt_and_refit()rebuildsTriTopicfrommodel.configonly, butn_topicslives on the model, so an explicit topic target gets reset to"auto"duringadapt_embeddings_with_llm(). Passmodel.n_topicsthrough the refit path, or include it in the copied config.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tritopic/core/model.py` around lines 1980 - 1993, Preserve the explicit topic count during the refit flow in adapt_embeddings_with_llm()/adapt_and_refit(), since rebuilding TriTopic from model.config alone drops model.n_topics back to "auto". Update the adapt_and_refit path to carry n_topics through when constructing the new model, either by passing model.n_topics explicitly or by copying it into the refit config before creating the replacement TriTopic, so the returned new_model keeps the intended topic target.
…ules
CI (.github/workflows/ci.yml):
- persist-credentials: false on all checkout steps
- add a read-only top-level permissions block
- timeout-minutes on the full-benchmark job
run_benchmark.py:
- embedding cache key now includes sample_seed, so a rerun with a different
subsampling seed can't silently reuse embeddings for a different set of docs
tritopic/adaptation/adapter.py:
- _resolve_mode now runs the same accelerate/datasets/sentence-transformers
version check for explicit adapter_mode="finetune" that the "auto" branch
already ran, raising the same actionable message instead of failing later
with a raw ImportError out of _finetune_sentence_transformer
- EmbeddingAdapter.encode()'s normalize flag is now threaded through to
LinearAdapter.transform() in linear mode instead of being ignored
tritopic/adaptation/correction.py:
- reassign_low_confidence drops any topic that a reassignment empties out
entirely before recomputing centroids/probabilities, instead of averaging
an empty embedding slice (was a real NaN-propagation crash, reproduced and
confirmed fixed with a new regression test)
- clarify batch_size's docstring: it chunks iteration only, doesn't batch
LLM requests (kept the signature as-is; a real multi-doc-per-call rewrite
is a bigger change than this pass warrants)
tritopic/adaptation/keyphrase.py:
- generate_keyphrases' cache loader now skips malformed/partial JSONL lines
instead of aborting the whole read
- keyphrase_expand_embeddings no longer blends in encode("") for documents
with an empty keyphrase list; also merges the doc+keyphrase encode calls
into one, halving request overhead for API-backed encoders
tritopic/adaptation/pipeline.py:
- adapt_and_refit carries the original model's explicit n_topics through to
the refit model instead of silently resetting it to "auto"
- warns when use_metadata_view was enabled, since the fit-time metadata
DataFrame isn't persisted on the model and can't be recovered for the refit
tritopic/core/model.py:
- AdaptationConfig is now imported under TYPE_CHECKING so the annotation on
adapt_embeddings_with_llm resolves for static analysis (was a bare
"name not defined" for mypy/Ruff)
Skipped (not still valid / not worth the risk-to-value tradeoff):
- Renaming/removing correction.py's batch_size outright — no call site or
doc example depends on the "real batching" framing; docstring fix covers
the misleading-cost-expectation concern without an API break.
All 232 tests pass (224 pre-existing + 8 new regression tests), including a
reproduction of the correction.py crash against the pre-fix code to confirm
the new test actually exercises the bug.
HF's datasets/huggingface_hub stopped resolving the legacy script-based "ag_news" repo id (HfUriError: repo id must be namespace/name), breaking the full benchmark CI job. Point at fancyzhx/ag_news, the parquet-based mirror with the same text/label schema.
The manual full-benchmark job (4 datasets x 3 seeds x k-grid x 4 models, including BERTopic) was hitting the 45-minute timeout and getting cancelled before finishing, not failing outright.
…mark-error-juhs58 Update AG News dataset source to fancyzhx/ag_news
This was referenced Jul 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
New tritopic.adaptation subpackage implementing ClusterLLM-style triplet
fine-tuning: sample LLM-judged (anchor, positive, negative) triplets from a
fitted model, adapt the embedder to them via a pure-numpy linear transform
or a real sentence-transformers fine-tune, and refit TriTopic on the result.
Also includes the cheaper Few-Shot-Clustering extras (LLM keyphrase
expansion, low-confidence correction) and a compare_embedders evaluation
harness for judging whether adaptation actually helped on a given corpus.
EmbeddingAdapter, evaluation harness, keyphrase/correction extras,
adapt_and_refit() pipeline
tune_resolution_with_llm's ergonomics
(synthetic + real 20 Newsgroups scenarios)
notebook with inline assertions, real 20NG data
adaptationextra (datasets, accelerate)with two-stage/bias-mitigation details and a diagnostics example
Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation