Description
CodeRabbit flagged that PR #272/#273 (sys.path import cleanup) mixed two
unrelated concerns:
-
Import refactor: removing sys.path.insert hacks, adding
__init__.py, normalizing imports — the stated objective of the PR.
-
Classifier client rework: adding get_classifier_client() with
dedicated httpx client, moving api_base from hardcoded URL to
os.environ/LLAMA_CLASSIFIER_URL, adding classifier env var to
.env and config.
Impact
The classifier changes were functional and needed, but they landed in what
was labeled as a chore/refactor PR. This:
- Makes git blame and history harder to follow
- Increases review surface (reviewers must assess import cleanup AND a
new HTTP client pattern simultaneously)
- Risk: if the classifier client had a bug, bisecting would point at the
import cleanup PR, confusing diagnosis
Suggested Action
Since the PR already merged (#275), this issue tracks the retrospective
concern. Going forward:
- Keep PRs scoped to one concern
- New infrastructure (HTTP clients, config changes) should be its own PR
- Refactors should be pure refactors unless the functional change is a
direct prerequisite
Related
Description
CodeRabbit flagged that PR #272/#273 (sys.path import cleanup) mixed two
unrelated concerns:
Import refactor: removing
sys.path.inserthacks, adding__init__.py, normalizing imports — the stated objective of the PR.Classifier client rework: adding
get_classifier_client()withdedicated httpx client, moving
api_basefrom hardcoded URL toos.environ/LLAMA_CLASSIFIER_URL, adding classifier env var to.envand config.Impact
The classifier changes were functional and needed, but they landed in what
was labeled as a chore/refactor PR. This:
new HTTP client pattern simultaneously)
import cleanup PR, confusing diagnosis
Suggested Action
Since the PR already merged (#275), this issue tracks the retrospective
concern. Going forward:
direct prerequisite
Related