Skip to content

[adapter] refactor: tidy _resolve_checkpoint_path call sites#161

Merged
Jayce-Ping merged 2 commits into
X-GenGroup:mainfrom
Jayce-Ping:refactor/resolve-checkpoint-cleanup
May 17, 2026
Merged

[adapter] refactor: tidy _resolve_checkpoint_path call sites#161
Jayce-Ping merged 2 commits into
X-GenGroup:mainfrom
Jayce-Ping:refactor/resolve-checkpoint-cleanup

Conversation

@Jayce-Ping
Copy link
Copy Markdown
Collaborator

Summary

Two small follow-up refactors to BaseAdapter._resolve_checkpoint_path and BaseAdapter.load_checkpoint, both prompted by re-reading the code after PR #160 was merged. No behavior change.

What changes

A. Hoist os.path.expanduser into _resolve_checkpoint_path (commit 1)

Currently load_checkpoint does:

path = os.path.expanduser(path)
path = self._resolve_checkpoint_path(path)

The two normalization steps belong together. _resolve_checkpoint_path is the one entry point that takes a raw user-provided spec (local path OR HF repo spec, with or without hf://) and returns a usable local directory; making it own expanduser too means callers no longer have to remember to normalize ~ separately.

Safe across all three input branches:

  • Local path with ~: expanded correctly (unchanged behavior).
  • Local path without ~: no-op.
  • HF spec (owner/repo or hf://...): no-op since expanduser only acts on a leading ~.

ModelArguments.__post_init__ still calls expanduser(resume_path) defensively at config-parse time, so other code that displays/logs model_args.resume_path keeps seeing the normalized form. The double call is idempotent.

B. Drop dead os.path.exists guard + collapse spec local (commit 2)

After the un-gating in PR #160, _resolve_checkpoint_path has these exit paths:

  1. Returns a local path it has just confirmed with os.path.exists.
  2. Returns a downloaded path that download_hf_checkpoint has verified with os.path.isdir.
  3. Raises FileNotFoundError with the actual root cause.

So the post-resolve check in load_checkpoint was unreachable AND its error message lied about the failure mode (it said "not found locally or on HF Hub", but _resolve_checkpoint_path would already have raised the real reason). Removed:

path = self._resolve_checkpoint_path(path)
if not os.path.exists(path):
    raise FileNotFoundError(...)   # unreachable; misleading message

Same commit also collapses the redundant spec local in _resolve_checkpoint_path. parse_hf_checkpoint_path already strips the hf:// prefix internally, and the local-path os.path.exists check is gated on not force_hf, so passing path directly (with the prefix still attached on the HF branch) is safe. One fewer variable for the reader to track.

Diff

Net: -9/+8 in src/flow_factory/models/abc.py. No other files touched.

Test plan

  • Pre-existing parser unit cases (8 happy + 5 error + 6 path-traversal) still pass — these refactors don't touch the parser.
  • No new imports, no public API change, no config change.
  • Single-GPU smoke: load LoRA from resume_path: "owner/repo" (will exercise the cleaned-up flow).

Out of scope

  • ModelArguments.__post_init__'s expanduser call — kept; defensive normalization at config-parse time, idempotent with the resolver, removing it could surprise other readers of model_args.resume_path.

Made with Cursor

Jayce-Ping and others added 2 commits May 17, 2026 10:40
…_path

Move the `path = os.path.expanduser(path)` call out of `load_checkpoint`
and into `_resolve_checkpoint_path` itself. The resolver is now the
single entry point that takes a raw user-provided spec and returns a
usable local directory; callers no longer need to remember to normalize
`~` separately.

Safe in all three input branches:
- Local path with `~`: expanded correctly (unchanged behavior).
- Local path without `~`: no-op.
- HF spec ('owner/repo' or 'hf://...'): no-op since `expanduser`
  only acts on a leading `~`.

Co-authored-by: Cursor <cursoragent@cursor.com>
Two small cleanups in the HF-checkpoint resolve path, both follow-ups
to the un-gating change:

A. Remove the dead `if not os.path.exists(path)` guard in
   `load_checkpoint`. `_resolve_checkpoint_path` now either:
     - returns a path it has just confirmed via `os.path.exists`, or
     - returns a downloaded path that `download_hf_checkpoint` has
       verified with `os.path.isdir`, or
     - raises `FileNotFoundError` with the actual root cause.
   The guard was useful pre-ungating (the old post-barrier
   `snapshot_download` call could return a non-existent path on
   cache-miss-after-failure); now it's unreachable AND its error
   message would lie about the failure mode.

B. Collapse the redundant `spec` local in `_resolve_checkpoint_path`.
   `parse_hf_checkpoint_path` already strips the `hf://` prefix
   internally, and the local-path `os.path.exists` check is gated on
   `not force_hf` so passing `path` (with the prefix still attached on
   the HF branch) is safe. Removes one variable from the reader's
   mental model.

Pure refactor: no behavior change. The 8 happy-path + 5 original-error
+ 6 path-traversal parser cases all still pass.

Co-authored-by: Cursor <cursoragent@cursor.com>
Copilot AI review requested due to automatic review settings May 17, 2026 02:41
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Small follow-up refactor to BaseAdapter checkpoint path handling. Moves os.path.expanduser into _resolve_checkpoint_path, removes a now-unreachable os.path.exists guard with a misleading error message in load_checkpoint, and collapses a redundant spec local variable since parse_hf_checkpoint_path strips the hf:// prefix internally. No behavioral change.

Changes:

  • Hoist expanduser into _resolve_checkpoint_path so callers don't need to remember to normalize ~ separately.
  • Drop the unreachable post-resolve os.path.exists check in load_checkpoint whose error message lied about the failure mode.
  • Pass path directly to parse_hf_checkpoint_path (which already handles the hf:// prefix) and remove the redundant spec variable.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Jayce-Ping Jayce-Ping merged commit 27a20cb into X-GenGroup:main May 17, 2026
4 checks passed
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