Skip to content

fix: keep manual bucket when autotuning from Python - #23

Open
bact wants to merge 3 commits into
munlicode:mainfrom
bact:fix/autotune-bucket-crash
Open

bact wants to merge 3 commits into
munlicode:mainfrom
bact:fix/autotune-bucket-crash

Conversation

@bact

@bact bact commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor
  • Don't zero a user-set bucket when autotune is on
  • Autotune's n-gram trials used to hash modulo 0 and crash
  • A manual bucket=0 means no n-grams, so don't sample them

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Manual zero buckets can still crash autotuning, and the regression test may not reliably exercise the failing path.

Review effort: Lite
Findings: 1 High severity

Open (1)
What changed in this PR

This PR preserves manually configured buckets during Python autotuning and adds regression coverage. Non-positive buckets can still cause modulo-by-zero crashes, and the test depends on a timing-sensitive trial.

Changes:

  • Retains user-specified buckets during autotuning.
  • Adds regression coverage for manual bucket autotuning.
  • Requires validation or substitution for non-positive buckets and deterministic trial coverage.
File Summary
python/​fasttext_module/​fasttext/​FastText.py Preserves manual bucket configuration during autotuning.
python/​fasttext_module/​fasttext/​tests/​test_autotune_bucket.py Adds regression coverage for autotune bucket handling.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread python/fasttext_module/fasttext/FastText.py Outdated
@bact

bact commented Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

Fixed as suggested in review. A manual bucket=0 under autotune now means no n-gram trials (no wordNgrams/minn sampling, maxn=0).

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