Repository navigation
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the system's robustness by proactively checking for sufficient disk space before critical operations like building and pulling models. By estimating the required storage and comparing it against available space, the system can provide early warnings to users, preventing potential failures due to full disks without halting the process entirely. This improves user experience by offering visibility into storage constraints. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable disk space pre-check for build and pull operations, which enhances robustness by warning users about potential out-of-space issues. The implementation is mostly solid, but I've identified a high-severity issue in the build size estimation logic where errors are silently ignored, potentially leading to inaccurate calculations. I've also noted a medium-severity portability concern in the diskspace package, as it currently only supports Unix-like systems. Additionally, a large design document for an unrelated feature seems to have been included, which I've flagged for removal to keep the commit history clean.
Check available disk space before writing to local storage. When space is insufficient (with 10% safety margin), log a warning but continue the operation without returning an error. Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
Add debug logging for errors encountered during directory walking in estimateBuildSize, so that inaccessible paths are visible instead of being silently ignored. Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
- Add overflow guard for Bavail * Bsize multiplication (cap at MaxInt64) - Handle negative values in formatBytes (return "0 B") - Add edge case tests for Check (empty path, root dir, deeply nested) - Add unit tests for estimateBuildSize (files, dirs, missing, empty) - Fix copyright year to 2025 in new files Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
1e66306 to
808f641
Compare
|
/gemini review |
|
/codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable pre-check for available disk space before build and pull operations, which helps prevent failures on systems with limited storage. The implementation is well-structured with a new diskspace package and includes relevant unit tests. My review provides feedback on a potential portability issue, a suggestion to improve logging for better error visibility, and a recommendation for maintaining testing consistency across the project.
|
@codex-code-assist review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 808f64149d
ℹ️ 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".
The previous implementation treated every Modelfile entry as a literal filesystem path, causing wildcard patterns (e.g. models/*.bin) and absolute paths to be silently skipped. processor/base.go already handles those patterns via filepath.Glob, so the disk-space pre-check was under-reporting the required size for common Modelfile inputs, defeating the warning. Mirror the processor/base.go expansion rules (literal with absolute-path support, or filepath.Glob for patterns containing *?[]) so the estimate matches what the builder actually processes. Also raise the size-estimation error log level from Debug to Warn so silent under-estimation becomes visible. Addresses review feedback on PR #471 (codex, gemini). Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
Add //go:build linux || darwin to check.go to make the Unix-only requirement explicit at compile time, and introduce a no-op stub in check_other.go so the package (and its callers in pkg/backend) still compile on Windows — the pre-check is simply skipped there. Native Windows support is deferred. Also rewrite check_test.go using stretchr/testify/assert for consistency with the rest of the project (e.g. pkg/backend/build_test.go). Addresses review feedback on PR #471 (gemini). Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
|
|
||
| // Check disk space before building (only for local output). | ||
| if !cfg.OutputRemote { | ||
| totalSize := estimateBuildSize(workDir, modelfile) |
There was a problem hiding this comment.
what if there are duplicates on the local disk? E.g., can I pull the same model multiple times?
There was a problem hiding this comment.
Good question. The two paths behave differently, so I handled them separately.
Pull (fixed in 7877f13). You can pull the same model many times. pullIfNotExist skips every blob that already exists in the local storage, so a repeated pull writes nothing. The old estimate still summed all layers and warned about space the pull did not need. estimatePullSize now stats each blob with store.StatBlob and only counts the missing ones. In extract-from-remote mode nothing is stored locally, so all layers count. A stat error counts the blob, so the estimate stays an upper bound. TestEstimatePullSize covers: nothing cached, partial overlap, full overlap (repeated pull estimates 0 bytes), stat error, and extract mode.
End-to-end check on a 6 MB tmpfs with a 4.2 MB model and a local registry:3.1.2:
| step | binary | free space before | warning |
|---|---|---|---|
| 1st pull | this branch | 6.0 MB | none |
| 2nd pull, same model | this branch | 2.0 MB | none (all 4 blobs skipped) |
| 2nd pull, same model | old head e37eb9f | 2.0 MB | insufficient disk space in /mnt/small/store: available 1.94 MB, required 4.41 MB |
Build (this line). The estimate sums the source files before hashing, so digests are not known yet and blobs from an earlier build of the same workspace cannot be subtracted. It stays an upper bound, and the check is warning-only, so a rebuild still succeeds. The warning text now says (upper bound: blobs already in the local storage are counted again). If you prefer, a follow-up could consult the build cache (path + mtime + size -> digest) and stat those digests, but that needs the builder to exist before the check, so I kept it out of this PR.
Resolve import conflict in pkg/backend/pull.go: keep both diskspace (from feature branch) and iometrics (from main) imports. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…mate A repeated pull of the same model (or of a model that shares layers with one already pulled) writes nothing for the blobs that already exist in the local storage, because pullIfNotExist skips them. The pre-check still summed every layer, so it warned about space the pull did not need. Add estimatePullSize, which stats each blob in the local storage and only counts the missing ones. In extract-from-remote mode nothing is stored locally, so all layers count. A stat error counts the blob, which keeps the estimate an upper bound. The build estimate stays an upper bound: digests are not known before hashing, so blobs from an earlier build of the same workspace cannot be subtracted. The build warning now says so. Signed-off-by: Zhao Chen <zhaochen.zju@gmail.com>
Same fix as #679. The recordingStrategy helper merged from main uses io.Reader and io.Copy without importing io, which breaks go vet and golangci-lint on this branch. Signed-off-by: Zhao Chen <zhaochen.zju@gmail.com>
|
@bergwolf Thanks for the review. Summary of the update:
PTAL when you have a moment. |
This branch has not been deployed
2 participants
Summary
pkg/diskspacepackage withCheck()function that usesunix.Statfsto verify available disk space before write operationsPull()andBuild()logrus.Warnfwarning but continues execution without returning an errorHow the estimate is computed
estimatePullSizestats each blob of the manifest in the local storage withStatBloband only counts the missing ones.pullIfNotExistskips existing blobs, so a repeated pull of the same model (or of a model that shares layers with one already pulled) estimates 0 bytes and does not warn. In extract-from-remote mode nothing is stored locally, so all layers count. A stat error counts the blob, so the estimate stays an upper bound.estimateBuildSizeexpands the Modelfile entries (literal paths, absolute paths, and glob patterns) and sums the source file sizes. Digests are not known before hashing, so blobs from an earlier build of the same workspace are counted again. The warning says so:(upper bound: blobs already in the local storage are counted again).pkg/diskspacebuilds onlinux || darwin. Other platforms compile a no-opCheck(native Windows support is tracked in Add Windows support for disk-space pre-check and other Unix-only packages #520).Test plan
pkg/diskspace(zero bytes, negative bytes, small size, extremely large size, non-existent dir walk-up, formatBytes)TestEstimateBuildSize(single files, directory entries, missing files, empty Modelfile)TestEstimatePullSize(nothing cached, partial overlap, full overlap, stat error, extract mode)go test ./... -count=1,go test -race ./pkg/backend/ ./pkg/diskspace/,golangci-lint run— all passregistry:3.1.2insufficient disk space in /mnt/small/store: available 1.94 MB, required 4.41 MB