De-duplicate update_checker: drop the stale copy in src/jabs/utils, keep the jabs-core one - #426
Merged
Merged
Conversation
src/jabs/utils/update_checker.py was a byte-for-byte duplicate of
packages/jabs-core/src/jabs/core/utils/update_checker.py apart from how it
resolved the version string (jabs.version.version_str() vs.
version_str("jabs-behavior-classifier"), which are equivalent). The jabs-core
copy is the one exported by jabs.core.utils, but the GUI was still importing
the stale root copy via jabs.utils.
Delete the root copy and re-export the two helpers from jabs-core in
src/jabs/utils/__init__.py, so jabs.utils.check_for_update and
jabs.utils.is_pypi_install (used by menu_handlers and update_check_dialog)
keep working unchanged.
Move tests/utils/test_update_checker.py to packages/jabs-core/tests/ and
retarget its patch paths, so the surviving implementation stays covered by
the jabs-core suite that CI runs.
Also switch the two log calls in the surviving module to lazy %s formatting,
per the logging conventions in CLAUDE.md.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MhEg7sSojc4dxBh7FbtZjW
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the duplicate update_checker implementation from the root src/jabs/utils tree and standardizes the application on the jabs-core implementation, aligning with the monorepo migration direction while preserving the existing GUI import surface (from jabs.utils import ...).
Changes:
- Deleted the stale duplicate
src/jabs/utils/update_checker.pyand keptpackages/jabs-core/.../update_checker.pyas the single implementation. - Updated
src/jabs/utils/__init__.pyto re-exportcheck_for_update/is_pypi_installfromjabs.core.utilsto maintain backward-compatible imports for the GUI. - Moved the update-checker unit tests under
packages/jabs-core/tests/and updated patch/import targets accordingly; adjusted logging calls in the surviving implementation to use lazy%sformatting.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/jabs/utils/update_checker.py | Removed duplicate PyPI update-checker module from the root package. |
| src/jabs/utils/init.py | Re-export update-check helpers from jabs-core to keep jabs.utils imports stable. |
| packages/jabs-core/src/jabs/core/utils/update_checker.py | Kept the canonical implementation; updated log formatting to lazy %s. |
| packages/jabs-core/tests/test_update_checker.py | Relocated tests to cover the jabs-core implementation and retargeted patches/imports. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reasoning
src/jabs/utils/update_checker.pyandpackages/jabs-core/src/jabs/core/utils/update_checker.pywere the same file twice. The only difference in the whole module was how the version string was obtained:Those two calls are equivalent —
jabs.version.version_str()is itself a one-line shim that callsjabs.core.utils.version.version_str("jabs-behavior-classifier"). Every other line of the two modules was identical.So the
jabs-coremigration for this module had already happened (it's implemented there, exported fromjabs.core.utils.__init__, and reachable asjabs.core.utils.check_for_update), but the old root copy was left in place — and the root copy is the one the GUI was actually importing, viafrom jabs.utils import check_for_updateinmenu_handlers.pyandfrom jabs.utils import is_pypi_installinupdate_check_dialog.py. Two live implementations of PyPI-update-checking, only one of which was reachable from the app, and only one of which was covered by tests (the root one —packages/jabs-core/tests/had no test for its own copy).I picked this over the other candidates I looked at because it is the highest-confidence, lowest-risk item on the list: it is the monorepo-transition cleanup described in the development guide, the destination file already exists so nothing is being moved, and there is no circular-dependency risk (the root package already depends on
jabs-core==0.46.2;jabs-coreimports nothing from the root tree). Other candidates I considered and skipped as either riskier or lower value:VideoLabels.counts()insrc/jabs/project/video_labels.pyis dead code that would raiseAttributeErrorif called (TrackLabelshas nocountsattribute), andProject.load_counts()has a return annotation ofdict[str, tuple[int, int]]where it actually returnsdict[int, dict[str, tuple[int, int]]]. Both are real, but removing a public method and correcting a public signature are judgement calls that deserve their own PRs rather than being bundled here.Why this is safe (net-zero behavior change):
jabs.utils.check_for_updateandjabs.utils.is_pypi_installstill resolve — now to thejabs-corefunctions. Verified:check_for_update is jabs.core.utils.update_checker.check_for_update→True. The two GUI call sites are untouched and need no change.jabs.utils.FINAL_TRAIN_SEEDand__all__are unchanged.One thing worth flagging for the reviewer: the module path
jabs.utils.update_checkerno longer exists (only thejabs.utilspackage-level names do). Nothing insrc/,tests/,packages/,docs/, ordev/imported that path apart from the test file this PR moves — I grepped — but it is technically a narrowing of an internal import surface.Change
Logic changes
src/jabs/utils/update_checker.py— deleted. Duplicate of thejabs-coremodule.src/jabs/utils/__init__.py— importscheck_for_update/is_pypi_installfromjabs.core.utilsinstead of the now-deleted local module. Module docstring notes that these are re-exports, so the next reader doesn't re-add a local copy.packages/jabs-core/src/jabs/core/utils/update_checker.py— the two log calls switched from f-strings to lazy%sformatting (logger.warning("Failed to check for updates: %s", e)), per the logging conventions inCLAUDE.md. Message text is unchanged; both copies had this issue and it seemed worth fixing on the one that survives.Mechanical updates
(no logic change — safe to skim)
tests/utils/test_update_checker.py→packages/jabs-core/tests/test_update_checker.py— the file moves so the surviving implementation is covered by the suite CI runs for that package (_run-tests-action.ymlruns eachpackages/*/testsdirectory separately). The only content edits are ased-style retarget ofjabs.utils.update_checker→jabs.core.utils.update_checkerin the import and in the 10patch(...)targets, plus the module docstring. No test logic, assertions, or fixtures changed.Verification
No
FEATURE_VERSIONbump: nothing here touches feature computation or the cache format.This PR was produced by an automated analysis from Claude Code.
Generated by Claude Code