-
Notifications
You must be signed in to change notification settings - Fork 64
Add multiple checkpoint directories optioning #58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
3313cdf
0404d6a
5c766cd
b66aa43
0a8348f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -3,20 +3,62 @@ | |||||
| import os | ||||||
| from dataclasses import dataclass | ||||||
| from pathlib import Path | ||||||
| from typing import Iterable, List | ||||||
|
|
||||||
| import dotenv | ||||||
|
|
||||||
| def get_default_checkpoint_dir() -> Path: | ||||||
| """Get the default checkpoint directory. | ||||||
| DEFAULT_CHECKPOINT_DIR = Path.home() / ".foundry" / "checkpoints" | ||||||
|
|
||||||
|
|
||||||
| def _normalize_paths(paths: Iterable[Path]) -> list[Path]: | ||||||
| """Return absolute, deduplicated paths in order.""" | ||||||
| seen = set() | ||||||
| normalized: List[Path] = [] | ||||||
|
||||||
| normalized: List[Path] = [] | |
| normalized: list[Path] = [] |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation-implementation mismatch: The docstring states "Always starts with the default ~/.foundry/checkpoints directory" but the implementation on line 36 returns _normalize_paths([*extra_dirs, DEFAULT_CHECKPOINT_DIR]), which places environment variable directories first. Either update the documentation to reflect the actual behavior (env dirs first, then default), or fix the implementation to match the documentation (default first, then env dirs).
| return _normalize_paths([*extra_dirs, DEFAULT_CHECKPOINT_DIR]) | |
| return _normalize_paths([DEFAULT_CHECKPOINT_DIR, *extra_dirs]) |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing docstring: The method lacks documentation explaining its search behavior. Add a docstring clarifying that it searches through checkpoint directories and returns the first existing match, or the primary directory path if none exist.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,7 +6,7 @@ | |||||||||||||||||||||||||||||||
| from urllib.request import urlopen | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| import typer | ||||||||||||||||||||||||||||||||
| from dotenv import find_dotenv, load_dotenv, set_key | ||||||||||||||||||||||||||||||||
| from dotenv import load_dotenv | ||||||||||||||||||||||||||||||||
| from rich.console import Console | ||||||||||||||||||||||||||||||||
| from rich.progress import ( | ||||||||||||||||||||||||||||||||
| BarColumn, | ||||||||||||||||||||||||||||||||
|
|
@@ -20,7 +20,8 @@ | |||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| from foundry.inference_engines.checkpoint_registry import ( | ||||||||||||||||||||||||||||||||
| REGISTERED_CHECKPOINTS, | ||||||||||||||||||||||||||||||||
| get_default_checkpoint_dir, | ||||||||||||||||||||||||||||||||
| append_checkpoint_to_env, | ||||||||||||||||||||||||||||||||
| get_default_checkpoint_dirs, | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| load_dotenv(override=True) | ||||||||||||||||||||||||||||||||
|
|
@@ -29,11 +30,25 @@ | |||||||||||||||||||||||||||||||
| console = Console() | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| def _resolve_checkpoint_dir(checkpoint_dir: Optional[Path]) -> Path: | ||||||||||||||||||||||||||||||||
| """Return user-specified checkpoint dir or fall back to default.""" | ||||||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||||||
| checkpoint_dir if checkpoint_dir is not None else get_default_checkpoint_dir() | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
| def _resolve_checkpoint_dirs(checkpoint_dir: Optional[Path]) -> list[Path]: | ||||||||||||||||||||||||||||||||
| """Return checkpoint search path with defaults first.""" | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
| """Return checkpoint search path with defaults first.""" | |
| """ | |
| Return the checkpoint search path, prioritizing user-specified directories. | |
| If `checkpoint_dir` is provided, it is expanded, resolved, and moved to the front | |
| of the search path. The function then attempts to persist the updated checkpoint | |
| directories to the `.env` file (if possible), so future runs will use the same | |
| configuration. | |
| Args: | |
| checkpoint_dir: Optional user-specified checkpoint directory. | |
| Returns: | |
| List of checkpoint directories, with the user-specified directory first if provided. | |
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outdated comment: The comment still refers to "Foundry install dir" (singular) but the variable name is now plural (
FOUNDRY_CHECKPOINT_DIRS). Update the comment to clarify that this is a colon-separated list of checkpoint directories.