feat: add keywords CLI tool for text vectorization (#122) - #163
Conversation
Greptile SummaryThis PR adds a
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([keywords CLI invoked]) --> B[parse_options]
B --> C{early exit?\n-h / -v}
C -- yes --> D([output & exit 0])
C -- no --> E[execute_command]
E --> F{command?}
F -- fit --> G[command_fit]
F -- extract --> H[command_extract]
F -- info --> I[command_info]
F -- bare text / default --> J[command_keywords]
G --> G1{args empty?}
G1 -- yes --> G2[stdin / StringIO]
G1 -- no --> G3[File.open each path]
G2 & G3 --> G4[MultiIO]
G4 --> G5[TFIDF#fit_from_stream]
G5 --> G6[save_to_file]
H --> H1{args empty?}
H1 -- yes --> H2[stdin text]
H1 -- no --> H3{file exists?}
H3 -- yes --> H4[File.read]
H3 -- no --> H5[use arg as text]
H2 & H4 & H5 --> TF
J --> J1{no args\nno stdin\ntty?}
J1 -- yes --> J2([show_getting_started])
J1 -- no --> J3[build document string]
J3 --> TF
I --> I1[TFIDF#load_from_file]
I1 --> I2([format & output stats])
TF[transform document] --> T1[stem_to_word_hash]
T1 --> T2[TFIDF#load_from_file]
T2 --> T3[TFIDF#transform → sort]
T3 --> T4([output term:score pairs])
Reviews (3): Last reviewed commit: "fix: greptile comments (#122)" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @output << 'General Options:' | ||
| @output << ' -m, --model FILE Model file (default: ./keywords.json)' | ||
| @output << ' -n, --top N Show top N terms only (e.g. keywords -n 5 "text...")' | ||
| @output << ' -q Quiet mode (clean output for scripting/pipelines)' |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [95/80]
| @output << '' | ||
| @output << 'General Options:' | ||
| @output << ' -m, --model FILE Model file (default: ./keywords.json)' | ||
| @output << ' -n, --top N Show top N terms only (e.g. keywords -n 5 "text...")' |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [98/80]
| @output << ' # Max DF: 1.0' | ||
| @output << '' | ||
| @output << 'General Options:' | ||
| @output << ' -m, --model FILE Model file (default: ./keywords.json)' |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [83/80]
| end | ||
|
|
||
| def show_getting_started | ||
| @output << 'Keywords - Keyword extraction and term analysis using TF-IDF' |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [81/80]
| transform(document) | ||
| end | ||
|
|
||
| def show_getting_started |
There was a problem hiding this comment.
Metrics/AbcSize: Assignment Branch Condition size for show_getting_started is too high. [41/15]
Metrics/MethodLength: Method has too many lines. [41/10]
| end | ||
| end | ||
|
|
||
| def execute_command |
There was a problem hiding this comment.
Metrics/MethodLength: Method has too many lines. [12/10]
|
|
||
| opts.on('--ngram MIN,MAX', Array, 'N-gram range (default: 1,1)') do |range| | ||
| invalid_range = range.count != 2 || !range.all? { |n| n =~ /\d+/ } | ||
| raise OptionParser::InvalidArgument, 'must have only 2 integers' if invalid_range |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [93/80]
| @options[:max_df] = n | ||
| end | ||
|
|
||
| opts.on('--ngram MIN,MAX', Array, 'N-gram range (default: 1,1)') do |range| |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [85/80]
| @options[:min_df] = n | ||
| end | ||
|
|
||
| opts.on('--max-df N', Float, 'Maximum document frequency ratio (default: 1.0)') do |n| |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [96/80]
| @options[:top] = n | ||
| end | ||
|
|
||
| opts.on('--min-df N', Integer, 'Minimum document frequency (default: 1)') do |n| |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [90/80]
| opts.separator '' | ||
| opts.separator 'Options:' | ||
|
|
||
| opts.on('-m', '--model FILE', 'Model file (default: ./keywords.json)') do |file| |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [90/80]
| opts.banner = 'Usage: keywords [text] [options] [command] [arguments]' | ||
| opts.separator '' | ||
| opts.separator 'Commands:' | ||
| opts.separator ' fit <files...> Fit the model from files or stdin' |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [81/80]
| private | ||
|
|
||
| def parse_options | ||
| @parser = OptionParser.new do |opts| |
There was a problem hiding this comment.
Metrics/BlockLength: Block has too many lines. [39/25]
|
|
||
| private | ||
|
|
||
| def parse_options |
There was a problem hiding this comment.
Metrics/AbcSize: Assignment Branch Condition size for parse_options is too high. [33.9/15]
Metrics/MethodLength: Method has too many lines. [44/10]
| rescue StandardError => e | ||
| @error << "Error: #{e.message}" | ||
| @exit_code = 1 | ||
| { output: @output.join("\n"), error: @error.join("\n"), exit_code: @exit_code } |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [87/80]
| # @rbs @exit_code: Integer | ||
| # @rbs @parser: OptionParser | ||
|
|
||
| def initialize(args, stdin: nil) |
There was a problem hiding this comment.
Metrics/MethodLength: Method has too many lines. [13/10]
|
|
||
| module Classifier | ||
| module Keywords | ||
| class CLI |
There was a problem hiding this comment.
Metrics/ClassLength: Class has too many lines. [200/100]
Style/Documentation: Missing top-level class documentation comment.
| @@ -0,0 +1,252 @@ | |||
| # rbs_inline: enabled | |||
There was a problem hiding this comment.
Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.
|
|
||
| module Classifier | ||
| module Streaming | ||
| # A utility class that wraps multiple IO-like streams and treats them as a single, |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [86/80]
| @@ -0,0 +1,42 @@ | |||
| # frozen_string_literal: true | |||
| # rbs_inline: enabled | |||
There was a problem hiding this comment.
Layout/EmptyLineAfterMagicComment: Add an empty line after magic comments.
| end | ||
|
|
||
| def test_keywords_without_args | ||
| skip( |
There was a problem hiding this comment.
Style/MultilineIfModifier: Favor a normal unless-statement over a modifier clause in a multiline statement.
| require 'classifier/keywords/cli' | ||
|
|
||
| module Keywords | ||
| class CLITest < Minitest::Test |
There was a problem hiding this comment.
Metrics/ClassLength: Class has too many lines. [122/100]
| require 'classifier/keywords/cli' | ||
|
|
||
| module Keywords | ||
| class CLITest < Minitest::Test |
There was a problem hiding this comment.
Metrics/ClassLength: Class has too many lines. [124/100]
|
@cardmagic can you take a look, please? |
cardmagic
left a comment
cardmagic
left a comment
There was a problem hiding this comment.
Deep review
Solid, well-tested implementation that tracks the issue closely: separate tool, transform-as-default, stdin-friendly, reuses TFIDF, clean command dispatch. MultiIO is minimal and correct, the LineReader integration is sound (it consumes via each_line, and estimate_line_count is guarded so multi-file fit degrades gracefully), rubocop is clean, and CI/typecheck are green. The earlier Greptile items all landed in commit 2. Verified locally by building + installing the gem and driving the CLI through ~25 scenarios.
One blocking issue and a ring of robustness/spec-fidelity gaps:
🔴 Blocking — keywords doesn't install. classifier.gemspec (not touched by this PR) still has s.executables = ['classifier']. exe/keywords ships inside the gem via s.files, but RubyGems only generates a binstub for names in executables. I built and installed the gem into an isolated GEM_HOME: only classifier appears in bin/, no keywords. So after gem install classifier, the tool the issue asks for isn't on PATH. Fix: s.executables = %w[classifier keywords]. The tests miss this because they call Classifier::Keywords::CLI directly instead of the installed binary. (Inline note left on exe/keywords.)
Robustness (details inline): fit silently saves an empty model (exit 0) when a glob matches nothing, when an explicitly-named file doesn't exist, or on empty stdin; --ngram validation accepts garbage like 1abc,2xyz; usage errors split between exit 1 and exit 2; raw Ruby exceptions leak to users (missing model, directory arg, negative -n).
Spec deviations vs #122: -q is effectively a no-op for the primary transform output; output tokens are Porter-stemmed (rubi, eleg, program) rather than the whole words the issue's examples show; fit granularity is one-document-per-line, which drives IDF and diverges from "build vocabulary from files".
Docs: README has a ## Command Line section for classifier but nothing for the new keywords command — it ships undiscoverable.
None of this is architectural; all are small, localized fixes. Minimum before merge: the gemspec executables line and the silent-empty-model guard.
| def mapping_stem_to_word_for_words(words, min_word_length) | ||
| h = {} | ||
| words.map { _1.tap(&:downcase!) }.tally.each do |word, count| | ||
| next unless !CORPUS_SKIP_WORDS.include?(word) && word.length >= min_word_length |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [85/80]
| end | ||
|
|
||
| # @rbs (Array[String], Integer) -> Hash[Symbol, Integer] | ||
| def mapping_stem_to_word_for_words(words, min_word_length) |
There was a problem hiding this comment.
Metrics/AbcSize: Assignment Branch Condition size for mapping_stem_to_word_for_words is too high. [17.97/15]
| word_hash_for_words(gsub(/[^\w\s]/, '').split, min_word_length) | ||
| end | ||
|
|
||
| # Builds a mapping between stemmed roots and their most frequent original words. |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [82/80]
| tfidf = TFIDF.load_from_file(@options[:model]) | ||
| vector = tfidf.transform(document).sort_by { |_, v| v }.reverse | ||
| vector = vector.first(@options[:top]) if @options[:top] | ||
| @output << vector.map { |k, v| "#{stem_map[k]}:#{v.round(2)}" }.join(' ') |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [81/80]
| @output << 'Run "keywords --help" for full usage.' | ||
| end | ||
|
|
||
| def transform(document) |
There was a problem hiding this comment.
Metrics/AbcSize: Assignment Branch Condition size for transform is too high. [17.58/15]
| end | ||
|
|
||
| opts.on('--ngram MIN,MAX', Array, 'N-gram range (default: 1,1)') do |range| | ||
| raise OptionParser::InvalidArgument, 'requires exactly two values' if range.count != 2 |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [98/80]
| end | ||
|
|
||
| opts.on('-n', '--top N', Integer, 'Show top N terms only') do |n| | ||
| raise OptionParser::InvalidArgument, 'must be positive' unless n.positive? |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [86/80]
| opts.banner = 'Usage: keywords [text] [options] [command] [arguments]' | ||
| opts.separator '' | ||
| opts.separator 'Commands:' | ||
| opts.separator ' fit <files...> Fit the model from files or stdin (each line is treated as a separate document)' |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [127/80]
| private | ||
|
|
||
| def parse_options | ||
| @parser = OptionParser.new do |opts| |
There was a problem hiding this comment.
Metrics/BlockLength: Block has too many lines. [42/25]
|
|
||
| private | ||
|
|
||
| def parse_options |
There was a problem hiding this comment.
Metrics/AbcSize: Assignment Branch Condition size for parse_options is too high. [40.61/15]
Metrics/CyclomaticComplexity: Cyclomatic complexity for parse_options is too high. [7/6]
Metrics/MethodLength: Method has too many lines. [47/10]
|
Hi @Yegorov could you please iterate on the greptile comments until greptile gives this a 5/5 rating. Thank you! |
| @exit_code = 0 | ||
| end | ||
|
|
||
| def run |
There was a problem hiding this comment.
Metrics/MethodLength: Method has too many lines. [12/10]
|
|
||
| module Classifier | ||
| module Keywords | ||
| class CLI |
There was a problem hiding this comment.
Metrics/ClassLength: Class has too many lines. [211/100]
Style/Documentation: Missing top-level class documentation comment.
| def test_stem_to_word_hash | ||
| hash = { rubi: 'ruby', program: 'programming', eleg: 'elegance', mean: 'means', defin: 'defines' } | ||
|
|
||
| assert_equal(hash, 'Ruby programming is elegant. Ruby means elegance. Elegance defines Ruby'.stem_to_word_hash) |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [115/80]
| end | ||
|
|
||
| def test_stem_to_word_hash | ||
| hash = { rubi: 'ruby', program: 'programming', eleg: 'elegance', mean: 'means', defin: 'defines' } |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [102/80]
|
|
||
| def test_keywords_command_output_original_words | ||
| make_model | ||
| result = run_cli('-m', @model_path, 'Dogs and cats are great. Large dog. Smart dog') |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [90/80]
|
|
||
| def test_fit_command_with_invalid_ngram_value_not_integers | ||
| result = run_cli( | ||
| '-m', @model_path, '--min-df', '2', '--max-df', '0.5', '--ngram', '1abc,2xyz', |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [86/80]
|
|
||
| def test_fit_command_with_invalid_ngram_three_value | ||
| result = run_cli( | ||
| '-m', @model_path, '--min-df', '2', '--max-df', '0.5', '--ngram', '1,2,3', |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [82/80]
| require 'classifier/keywords/cli' | ||
|
|
||
| module Keywords | ||
| class CLITest < Minitest::Test |
There was a problem hiding this comment.
Metrics/ClassLength: Class has too many lines. [189/100]
|
@greptile-apps review this PR, please. |
| if @args.empty? | ||
| streams = [@stdin ? StringIO.new(@stdin.to_s) : $stdin] | ||
| else | ||
| files = @args.map { |arg| Dir.glob(arg).map { |f| File.expand_path(f) } }.flatten.uniq |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [96/80]
| end | ||
| end | ||
|
|
||
| def command_fit |
There was a problem hiding this comment.
Metrics/AbcSize: Assignment Branch Condition size for command_fit is too high. [30.68/15]
Metrics/MethodLength: Method has too many lines. [19/10]
|
|
||
| module Classifier | ||
| module Keywords | ||
| class CLI |
There was a problem hiding this comment.
Metrics/ClassLength: Class has too many lines. [210/100]
Style/Documentation: Missing top-level class documentation comment.
|
@greptile-apps can you write summary with confidence score, please. |
|
Hello @cardmagic, all greptile comments fixed. Can you take a look, please? |
cardmagic
left a comment
cardmagic
left a comment
There was a problem hiding this comment.
Follow-up QA pass
Re-verified every item from my earlier review against the current head (38d85a5) by building the gem and installing it into an isolated GEM_HOME, then driving the real keywords binary through ~35 scenarios.
All previously-raised items are fixed — the gemspec executables blocker (binstub confirmed present after gem install), the silent-empty-model saves, --ngram validation, exit-code consistency, the raw-exception leaks, and the README section. Output is now de-stemmed (elegant:0.62 programming:0.51 ruby:0.43), which matches the examples in #122. Tests pass (697 runs, 0 failures) and rubocop is clean. Also withdrawing my earlier -q note — suppressing chatter while keeping data output is the correct Unix semantics.
One new regression turned up, introduced by the de-stemming fix in a85bfae, plus two smaller inconsistencies. Details inline.
Minimum before merge: the label_for fix on transform.
| tfidf = TFIDF.load_from_file(@options[:model]) | ||
| vector = tfidf.transform(document).sort_by { |_, v| v }.reverse | ||
| vector = vector.first(@options[:top]) if @options[:top] | ||
| @output << vector.map { |k, v| "#{stem_map[k]}:#{v.round(2)}" }.join(' ') |
There was a problem hiding this comment.
🔴 Blocking — n-gram output has empty labels. This is a regression from a85bfae, which changed "#{k}" to "#{stem_map[k]}" to fix the Porter-stemming feedback.
stem_to_word_hash only builds unigram stem→word entries, but n-gram vocabulary keys are underscore-joined stems (machin_learn, learn_neural). Every n-gram key misses the map and stem_map[k] returns nil, rendering as an empty label:
$ keywords fit --ngram 1,2 -m ng.json corpus/*.txt
$ keywords -m ng.json "machine learning neural networks"
:0.43 :0.43 networks:0.43 :0.35 neural:0.35 machine:0.35 learning:0.3
^^^^^ ^^^^^ ^^^^^Before the fix this printed machin_learn:0.43 — ugly, but usable. It matters because --ngram MIN,MAX is in the #122 spec and this PR's own README advertises keywords fit --min-df 2 --max-df 0.85 --ngram 1,2 corpus/*.txt, so a documented workflow now produces unusable output.
Same root cause bites a second case: the label map is built from the document with a hardcoded min_word_length of 3, rather than derived from the model. A model saved with a different min_word_length (legal via the Ruby API and loadable through -m) also yields empty labels:
$ ruby -e 'require "classifier"; t=Classifier::TFIDF.new(min_word_length: 2); t.fit(["go to db", "db is an elegant store", "go build web apps"]); t.save_to_file("mwl.json")'
$ keywords -m mwl.json "go to db elegant store"
store:0.56 elegant:0.56 :0.43 :0.43Suggested fix — split the key on _ and map each component back through the stem map, falling back to the raw part:
def label_for(key, stem_map)
key.to_s.split('_').map { |part| stem_map[part.to_sym] || part }.join(' ')
endthen @output << vector.map { |k, v| "#{label_for(k, stem_map)}:#{v.round(2)}" }.join(' ').
I verified this locally against all three cases, with the existing suite still green:
bigram model: neural networks:0.43 learning neural:0.43 networks:0.43 machine learning:0.35 ...
min_word_len2: store:0.56 elegant:0.56 db:0.43 go:0.43
unigram model: elegant:0.62 programming:0.51 language:0.43 ruby:0.43 (unchanged)
Worth adding a test that transforms against an n-gram model — the current --ngram tests only cover fit, which is why this slipped through.
| @stdin ? @stdin.to_s : $stdin.read | ||
| else | ||
| file = File.expand_path(@args.first) | ||
| File.exist?(file) ? File.read(file) : @args.first |
There was a problem hiding this comment.
Non-blocking: extract silently swallows a typo'd filename. When the path doesn't exist, the argument falls through to being analyzed as literal text. Its tokens aren't in the vocabulary, so the user gets no output and exit 0:
$ keywords extract corpus/a.txtt # note the typo
$ echo $?
0For a command documented as extract <file> Extract keywords from a file, a nonexistent path should be a UsageError rather than silence. The text-fallback is what the bare keywords <text> form is already for, so I don't think extract needs it.
| def command_info | ||
| @args.shift | ||
|
|
||
| tfidf = TFIDF.load_from_file(@options[:model]) |
There was a problem hiding this comment.
Non-blocking: info misses the model guard that transform has. Same user error, two different treatments:
$ keywords -m /nope/missing.json "ruby programming"
Error: No model found; run 'keywords fit' first or pass correct model using the '-m' option. # exit 2
$ keywords info -m /nope/missing.json
Error: No such file or directory @ rb_sysopen - /nope/missing.json # exit 1Hoisting the File.exist? check from transform into a shared helper would make both paths give the friendly message and exit 2.
Related, a directory argument still leaks a raw errno through the StandardError rescue:
$ keywords fit -m dir.json corpus
Error: Is a directory @ io_fillbuf - fd:6 /path/to/corpus
$ keywords extract corpus
Error: Is a directory @ io_fread - /path/to/corpus3 participants
Closes #122