Skip to content

fix(profile): avoid pinning dynamic language classes - #93

Merged
yisding merged 2 commits into
mainfrom
codex/fix-cached-languageprofile-memory-retention-issue
Jul 11, 2026
Merged

fix(profile): avoid pinning dynamic language classes#93
yisding merged 2 commits into
mainfrom
codex/fix-cached-languageprofile-memory-retention-issue

Conversation

@yisding

@yisding yisding commented Jun 29, 2026

Copy link
Copy Markdown
Owner

Motivation

  • The profile cache used a WeakKeyDictionary keyed by language class but each cached LanguageProfile held a strong language reference back to the key, defeating weak-key cleanup for dynamically generated language classes.
  • This could cause unbounded memory retention in long-running services that create and discard language classes, so the cache must not strongly reference its keys.

Description

  • Removed the language: type field from LanguageProfile so cached values no longer strongly reference the language class stored as the weak-key in _PROFILE_CACHE (sentencesplit/language_profile.py).
  • Kept the runtime language available on the per-call Processor instance by storing self.lang = lang in Processor.__init__ and passing self.lang to the abbreviation replacer instead of self.profile.language (sentencesplit/processor.py).
  • Added a regression test test_language_profile_cache_does_not_pin_dynamic_language_classes that builds a profile from a dynamically defined language class, drops local references, forces GC, and asserts the dynamic class is collectible (tests/unit/test_language_profile.py).

Testing

  • Ran the focused unit tests with uv run pytest tests/unit/test_language_profile.py -q and they passed.
  • Ran a targeted cross-section with uv run pytest tests/unit/test_language_profile.py tests/contract/test_processor.py tests/lang/test_deutsch.py tests/lang/test_kazakh.py tests/regression/test_kazakh_multiperiod_sentinel.py tests/regression/test_german_standalone_i.py -q and the run succeeded (all tests in that selection passed).
  • Ran the full test suite with uv run pytest --cov=sentencesplit tests/ --color yes which completed successfully (test run reported all tests passed with expected skips/xfails: 10508 passed, 14 skipped, 113 xfailed).
  • Ran lint/format checks with uv run ruff check . and uv run ruff format --check . and both checks passed.

Codex Task

Summary by CodeRabbit

  • Bug Fixes

    • Improved language handling during sentence processing and abbreviation replacement.
    • Prevented cached language information from being unnecessarily retained, improving memory cleanup for dynamically configured languages.
    • Updated language profiles to avoid exposing redundant language metadata.
  • Tests

    • Added coverage to verify language configurations can be released correctly after processing.

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 236f17b458

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".


def replace_abbreviations(self, text: str) -> str:
return self.profile.abbreviation_replacer_cls(text, self.profile.language, split_mode=self.split_mode).replace()
return self.profile.abbreviation_replacer_cls(text, self.lang, split_mode=self.split_mode).replace()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid pinning dynamic languages through abbreviation caching

When a dynamic language is actually used for segmentation, this still passes the language class into AbbreviationReplacer; on first classification, AbbreviationReplacer._period_classifier() caches a PeriodClassifier whose reference replacer is built with that same lang (abbreviation_replacer.py:257) and stores it on .lang, so _data._classifier_cache keeps the dynamic class alive after Processor(...).process() and the weak profile key cannot be reclaimed. The new GC test only calls LanguageProfile.from_language(), so it misses the long-running service scenario this fix is meant to cover.

Useful? React with 👍 / 👎.

@charliecreates charliecreates Bot 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.

Reviewed the latest changes, and I do not have actionable feedback to address.

@coderabbitai

coderabbitai Bot commented Jul 11, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 233b0a15-8016-4a2b-9ba4-bf1bfa56ce5a

📥 Commits

Reviewing files that changed from the base of the PR and between d198fe2 and 47f6e5d.

📒 Files selected for processing (4)
  • sentencesplit/abbreviation_replacer.py
  • sentencesplit/language_profile.py
  • sentencesplit/processor.py
  • tests/unit/test_language_profile.py
💤 Files with no reviewable changes (1)
  • sentencesplit/language_profile.py

📝 Walkthrough

Walkthrough

LanguageProfile no longer stores language classes. Processor retains its language argument for abbreviation replacement, and cached classifier references clear language state. Tests verify profile fields and ensure dynamically defined language classes can be garbage-collected.

Changes

Language cache ownership

Layer / File(s) Summary
Profile and cached reference ownership
sentencesplit/language_profile.py, sentencesplit/abbreviation_replacer.py
Removes the language field from LanguageProfile and clears language state on cached classifier reference replacers.
Processor wiring and retention test
sentencesplit/processor.py, tests/unit/test_language_profile.py
Stores the language on Processor, uses it during abbreviation replacement, and tests profile updates plus dynamic-class collection.

Estimated code review effort: 2 (Simple) | ~10 minutes

Poem

A rabbit hops through cached code,
Unpinning classes from their load,
Profiles shed a borrowed name,
Replacers clear their language claim,
And weakrefs cheer: “GC’s the road!”

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description covers motivation, implementation, and testing, but it omits the template's Summary, Type of change, Linked issues, and Notes sections. Rewrite the PR description to match the template, adding Summary, Type of change, Linked issues, Test evidence checkboxes, and Notes for reviewers.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main fix: preventing dynamic language classes from being pinned.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-cached-languageprofile-memory-retention-issue

Comment @coderabbitai help to get the list of available commands.

@yisding
yisding merged commit 8e73a33 into main Jul 11, 2026
10 checks passed
@yisding
yisding deleted the codex/fix-cached-languageprofile-memory-retention-issue branch July 11, 2026 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant