Skip to content

Add incremental TF-IDF learning fix for chatbot sample (issue #157) - #189

Open
Hardikrepo wants to merge 1 commit into
microsoft:masterfrom
Hardikrepo:fix/chatbot-incremental-tfidf-157
Open

Add incremental TF-IDF learning fix for chatbot sample (issue #157)#189
Hardikrepo wants to merge 1 commit into
microsoft:masterfrom
Hardikrepo:fix/chatbot-incremental-tfidf-157

Conversation

@Hardikrepo

Copy link
Copy Markdown

Summary

  • Adds a community sample fixing the O(n^2) growth issue raised in Chatbot #157: the original chatbot's learn_from_pair refit the entire TF-IDF vectorizer on every call.
  • The fix reuses the already-fitted vectorizer's transform() for new examples and appends via scipy.sparse.vstack (O(1) amortized per call), with a periodic full rebuild (default every 20 additions) to resync vocabulary/IDF weights.

Context

See discussion in #157 for the original script and review comments identifying the O(n^2) issue and suggesting an incremental approach.

Test plan

  • Run community-samples/tfidf-chatbot-incremental-fix/simple_chatbot.py interactively and confirm learn: commands work and responses remain correct after several incremental learns.

Avoids O(n^2) growth from refitting the vectorizer on every
learn_from_pair call by appending via scipy.sparse.vstack and
rebuilding only periodically.
@Hardikrepo

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

@ManyaS-Git Manya Sharma (ManyaS-Git) left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for this contribution — a clear, well-commented fix for the O(n²) refit in issue #157. The incremental transform() + vstack approach is a genuine improvement over refitting every time, and the periodic full rebuild is a sensible trade-off. A few things to address before merge:

1. The README's "O(1) amortized per call" claim is inaccurate (README.md, last paragraph).
scipy.sparse.vstack does not append in place — it builds a brand-new combined matrix each call and copies all existing data. So each learn_from_pair is still O(n) in the current corpus size, and a full learning session remains O(n²) in copy cost (just with a much smaller constant factor than refitting, since it skips re-tokenization and fit_transform). Please update the README to state the actual behaviour (reduces constant factor, avoids repeated refit) rather than O(1) amortized. If true O(1)-amortized appends are desired, accumulate the transformed row blocks in a list and only vstack once at query time, or pre-allocate the CSR arrays.

2. import readline breaks on Windows (simple_chatbot.py:5).
readline only exists on Unix. I verified ModuleNotFoundError: No module named 'readline' on Windows, which kills the whole sample at startup. Since it's only used for interactive history on Unix-like terminals, wrap it:

try:
    import readline  # Unix interactive history (optional)
except ImportError:
    pass

3. New terms are silently dropped until the next rebuild (learn_from_pair).
Because transform() reuses the already-fitted vocabulary, any n-gram in a newly learned example that wasn't in the original corpus is discarded (with ngram_range=(1,2)). Until the periodic rebuild fires, those examples are effectively unretrievable even when highly relevant. This is documented in spirit, but consider noting it in the README more explicitly, or triggering an early rebuild when a transformed row is empty/all-zero for a non-empty input.

4. respond(top_k>1) can return the lowest-scoring candidate (respond).
np.argsort returns indices ascending, so top_indices = np.argsort(sims)[-top_k:] — a uniform np.random.choice over these can pick the weakest of the top-k, potentially one scoring below the 0.25 threshold (only best_score is threshold-checked). The randomization is fun but undocumented in the README; consider sampling proportional to score, or at least documenting the behaviour.

5. Demo seed pairs are discarded if a memory file exists (demo()).
SimpleChatBot(initial_pairs) is immediately followed by load_from_file(...), which replaces self.pairs wholesale when my_bot_memory.txt exists. A user who runs the sample once, then re-runs it, silently loses the six built-in examples. Consider merging the loaded pairs with the seeds (dedupe), or documenting the behaviour.

6. Minor: learn_from_pair doesn't guard against empty user_text/bot_answer (an empty query row is all-zeros, harmless but adds noise), and np.random.choice uses the global RNG — fine for a demo, but worth a seed for reproducibility.

Overall this is a solid, minimal change that directly addresses the reported issue. Fixing items 1 and 2 (README claim + readline import) would make it merge-ready.

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