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.
| end | ||
|
|
||
| def label_for(key, stem_map) | ||
| key.to_s.split('_').map { |part| stem_map[part.to_sym] || part }.join(' ') |
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| "#{label_for(k, stem_map)}:#{v.round(2)}" }.join(' ') |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [92/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. [15.56/15]
| transform(document) | ||
| end | ||
|
|
||
| def command_info |
There was a problem hiding this comment.
Metrics/MethodLength: Method has too many lines. [12/10]
| @stdin ? @stdin.to_s : $stdin.read | ||
| else | ||
| file = File.expand_path(@args.first) | ||
| raise UsageError, "File #{file.inspect} does not exists" unless File.exist?(file) |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [93/80]
|
|
||
| module Classifier | ||
| module Keywords | ||
| class CLI |
There was a problem hiding this comment.
Metrics/ClassLength: Class has too many lines. [217/100]
Style/Documentation: Missing top-level class documentation comment.
| assert_match('must be positive', result[:error]) | ||
| end | ||
|
|
||
| def test_keywords_command_with_bigram_model |
There was a problem hiding this comment.
Metrics/AbcSize: Assignment Branch Condition size for test_keywords_command_with_bigram_model is too high. [16.03/15]
| require 'classifier/keywords/cli' | ||
|
|
||
| module Keywords | ||
| class CLITest < Minitest::Test |
There was a problem hiding this comment.
Metrics/ClassLength: Class has too many lines. [246/100]
cardmagic
left a comment
cardmagic
left a comment
There was a problem hiding this comment.
QA pass on 351f570
Re-verified by building the gem and installing it into an isolated GEM_HOME, then driving the real keywords binary through ~45 scenarios.
The blocking label_for regression is fixed. Confirmed across all three cases from my last review:
$ keywords -m ng.json "machine learning neural networks" # bigram model
learning neural:0.48 machine learning:0.4 machine:0.4 neural networks:0.34 networks:0.34 ...
$ keywords -m mwl.json "go to db elegant store" # model with min_word_length: 2
store:0.56 elegant:0.56 db:0.43 go:0.43
$ keywords -m uni.json "Ruby programming is elegant" # unigram, unchanged
elegant:0.58 programming:0.58 ruby:0.58Thanks for adding test_keywords_command_with_bigram_model. The extract typo guard and the info model guard are both fixed and now exit 2 with the friendly message. Everything from the earlier rounds still holds under regression check: gemspec binstub, empty-model guards, --ngram validation, -n positivity, README. Suite is 701 runs / 0 failures, rubocop clean, CI green.
Two new findings, both in command_fit, details inline. The first is the one I would like fixed before merge.
Also still open, lower priority:
- The directory-argument errno leak from my earlier
infocomment was marked fixed but only the model guard half landed.keywords fit -m dir.json corpusstill printsError: Is a directory @ io_fillbuf - fd:6 ...andkeywords extract corpusprintsError: Is a directory @ io_fread - ..., both exit 1. --min-df -5and--max-df 9.9are usage errors but exit 1, since the message surfaces fromTFIDF.new.--ngrambounds errors correctly exit 2. Same split we just fixed for--ngram, relocated.label_forsplits on_before trying the whole key, so a literal snake_case token in the source text renders mangled:machine_learningprints asmachine learn. One-line fix is to trystem_map[key.to_sym]first and fall back to the split.- Typo:
"File #{file.inspect} does not exists"should bedoes not exist. - My own suggestion has a wrinkle worth a conscious decision: joining n-gram parts with a space puts a space inside labels (
machine learning:0.32) in an otherwise space-separatedterm:scorestream, which is ambiguous for scripting. Not asking you to change it, just flagging that we chose it.
| 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.
Nonexistent paths are silently dropped whenever any other file is valid. Dir.glob returns [] for a path that does not exist, and the zero-document guard on line 155 only fires when nothing at all was read. So a typo in one of several arguments produces a smaller model and reports success:
$ keywords fit -m mix.json corpus/a.txt corpus/NOPE.txt
Saved to "/path/mix.json"
$ echo $?
0
$ keywords info -m mix.json
Documents: 2 # a.txt only; a+b+c would be 6keywords fit corpus/*.txt archive/*.txt with a wrong second path silently fits half the corpus, exit 0. Since IDF depends on the document set, the resulting model is quietly wrong rather than obviously broken.
This also makes the tool internally inconsistent as of this commit: command_extract now hard-fails with a UsageError on a nonexistent path (which is the right call), while command_fit ignores the identical typo. Same user error, two behaviors in one CLI.
Suggested fix, raise per argument rather than only on the aggregate:
files = @args.flat_map do |arg|
matches = Dir.glob(arg)
raise UsageError, "No files matched #{arg.inspect}" if matches.empty?
matches.map { |f| File.expand_path(f) }
end.uniqThat keeps the exit-2 behavior for the all-empty case too, so the guard on line 155 then only needs to cover genuinely empty input (empty stdin, empty files).
| streams = [@stdin ? StringIO.new(@stdin.to_s) : $stdin] | ||
| else | ||
| files = @args.map { |arg| Dir.glob(arg).map { |f| File.expand_path(f) } }.flatten.uniq | ||
| files.each { |f| streams << File.open(f) } |
There was a problem hiding this comment.
Every input file is opened at once, so large corpora hit the file-descriptor limit. command_fit materializes the full streams array before MultiIO reads a single line, so peak open descriptors equals the corpus size:
$ ( ulimit -n 256; keywords fit -m many.json many/*.txt ) # 600 files
Error: Too many open files @ rb_sysopen - /path/many/d324.txt
$ echo $?
1Stock Linux ulimit -n is 1024, so a corpus of roughly a thousand files breaks the workflow this PR's own README leads with (keywords fit corpus/*.txt). macOS masks it because the default limit is much higher, which is probably why it has not shown up.
The fix is the one Greptile originally gestured at: give MultiIO the paths and let it open and close one stream at a time. Something like:
class MultiIO
def initialize(sources)
@sources = sources.dup
end
def each_line
return enum_for(:each_line) unless block_given?
@sources.each do |source|
if source.respond_to?(:each_line)
source.each_line { |line| yield line }
else
File.open(source) { |io| io.each_line { |line| yield line } }
end
end
end
endThen command_fit passes paths for the file case and the single $stdin / StringIO object for the stdin case, and the ensure block on line 160 goes away entirely, since File.open with a block closes on its own and on exception. That also resolves the handle-leak thread above more robustly than the current manual bookkeeping.
3 participants
Closes #122