Repository navigation
feat(modelfile): support TF SavedModel, ONNX external_data, and online-learning trees - #530
Conversation
![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: 4f303daf59
ℹ️ 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".
![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 logic to correctly classify model-related files for TensorFlow SavedModels and ONNX models. It adds literal name matching for specific TensorFlow files and implements a custom ONNX parser to identify external data files referenced within .onnx files, ensuring they are categorized as model weights regardless of size or extension. A security concern was identified regarding potential path traversal vulnerabilities when processing external data paths extracted from ONNX files, and a validation check was suggested to ensure paths do not escape the workspace.
Three follow-up fixes from independent review of PR #530: P1 (correctness/security): reclassifyONNXExternalData now only moves paths already collected by the workspace walker, naturally inheriting all of the walker's filtering: ExcludePatterns, isSkippable directories, file count / size limits, and the workspace boundary. Adversarial or malformed ONNX referencing `../escape` is silently dropped instead of being added to mf.model. P2 (coverage): the ONNX parser now recurses into NodeProto.attribute so external_data references attached via Constant ops (attribute.t / tensors / sparse_tensor / sparse_tensors) and inside If / Loop / Scan subgraphs (attribute.g / graphs) are discovered. Subgraph recursion is bounded at 32 levels to defend against pathological inputs. P3 (visibility): ONNX parse failures now print a clearly prefixed WARNING line that names the offending file and explains the fallback ("external tensor files will keep walker-assigned classification"). Generate does NOT abort -- a single bad .onnx degrades to the pre-fix walker classification without killing the whole pass. New tests: NodeAttributeTensor, Subgraph (P2 unit); PathTraversalIgnored, RespectsExclude, ParseFailureFallsBack (P1/P3 integration). go vet, go test -race ./pkg/modelfile/..., and go test ./... all clean. Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 660ae9b82f
ℹ️ 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".
Codex review on PR #530 noted a remaining coverage gap: ExtractONNXExternal- DataPaths only walked ModelProto.graph (field 7), missing external_data references in training-enabled ONNX (IR v7+) where weights/states can also appear in ModelProto.training_info[*].initialization (field 1) and .algorithm (field 2) GraphProtos. Refactor ExtractONNXExternalDataPaths to iterate ModelProto fields directly via forEachField rather than the now-removed readSubMessage helper. Both graph and training_info subtrees route into the same walkGraph entry, so all the existing recursion (initializer / sparse_initializer / NodeProto attribute / subgraph) applies uniformly to inference and training graphs. Tests: TrainingInfoGraphs covers a training-only model; InferenceAndTrainingCombined verifies both surfaces are merged. Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
…ING text, fix doc Three follow-up findings from a second codex review pass on PR #530: 1. Reject absolute external_data.location values: filepath.Join(onnxDir, /decoy) silently strips the leading separator and produces a relative decoy path, which would reclassify an unrelated workspace file. Add filepath.IsAbs(ext) guard before Join. 2. Lock the WARNING contract: ParseFailureFallsBack now captures os.Stderr and asserts the warning prefix, file path, and fallback note are printed. 3. Update ExtractONNXExternalDataPaths godoc to enumerate the real error sources: I/O, size cap, and malformed protobuf wire data. go vet, go test -race ./pkg/modelfile/..., and go test ./... all clean. Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
|
@codex review Adds best-effort
Generic Contracts:
New commit: 876d157. Tests: 4 dedicated cases (CLI flag wins, no-signal stays empty, priority order saved_model.pb > .safetensors, gguf), plus updated 4 existing table cases. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 876d1571ab
ℹ️ 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".
|
@codex review Addresses the P2 saved_model.pbtxt false-negative finding from the previous review (commit 4c24aa3). |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
E2E verificationCross-compiled the binary from commit Fixture 1 — TF SavedModel
Fixture 2 — ONNX + external_data
Fixture 3 — online-learning base + delta tree
The three behavior changes shipped in this PR are confirmed end-to-end (not only in unit tests):
|
…e-learning trees Generator now correctly classifies extension-less TF SavedModel literals (feature_map, checkpoint) as MODEL via new ModelFilePatterns entries, and deterministically reclassifies all ONNX external_data tensor files as MODEL by parsing the .onnx protobuf with google.golang.org/protobuf/encoding/protowire. This removes the size-heuristic dependency that previously misclassified small external tensor files as CODE. Online-learning directories (a parent dir containing base/<ts> and delta/<ts> subtrees) are handled by existing recursive walk; new test cases mirror the real workflow_10062365 layout. No changes to build/push/pull/attach/upload/codec/storage paths --- those remain byte-level and model-type-agnostic. Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
Three follow-up fixes from independent review of PR #530: P1 (correctness/security): reclassifyONNXExternalData now only moves paths already collected by the workspace walker, naturally inheriting all of the walker's filtering: ExcludePatterns, isSkippable directories, file count / size limits, and the workspace boundary. Adversarial or malformed ONNX referencing `../escape` is silently dropped instead of being added to mf.model. P2 (coverage): the ONNX parser now recurses into NodeProto.attribute so external_data references attached via Constant ops (attribute.t / tensors / sparse_tensor / sparse_tensors) and inside If / Loop / Scan subgraphs (attribute.g / graphs) are discovered. Subgraph recursion is bounded at 32 levels to defend against pathological inputs. P3 (visibility): ONNX parse failures now print a clearly prefixed WARNING line that names the offending file and explains the fallback ("external tensor files will keep walker-assigned classification"). Generate does NOT abort -- a single bad .onnx degrades to the pre-fix walker classification without killing the whole pass. New tests: NodeAttributeTensor, Subgraph (P2 unit); PathTraversalIgnored, RespectsExclude, ParseFailureFallsBack (P1/P3 integration). go vet, go test -race ./pkg/modelfile/..., and go test ./... all clean. Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
Codex review on PR #530 noted a remaining coverage gap: ExtractONNXExternal- DataPaths only walked ModelProto.graph (field 7), missing external_data references in training-enabled ONNX (IR v7+) where weights/states can also appear in ModelProto.training_info[*].initialization (field 1) and .algorithm (field 2) GraphProtos. Refactor ExtractONNXExternalDataPaths to iterate ModelProto fields directly via forEachField rather than the now-removed readSubMessage helper. Both graph and training_info subtrees route into the same walkGraph entry, so all the existing recursion (initializer / sparse_initializer / NodeProto attribute / subgraph) applies uniformly to inference and training graphs. Tests: TrainingInfoGraphs covers a training-only model; InferenceAndTrainingCombined verifies both surfaces are merged. Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
…ING text, fix doc Three follow-up findings from a second codex review pass on PR #530: 1. Reject absolute external_data.location values: filepath.Join(onnxDir, /decoy) silently strips the leading separator and produces a relative decoy path, which would reclassify an unrelated workspace file. Add filepath.IsAbs(ext) guard before Join. 2. Lock the WARNING contract: ParseFailureFallsBack now captures os.Stderr and asserts the warning prefix, file path, and fallback note are printed. 3. Update ExtractONNXExternalDataPaths godoc to enumerate the real error sources: I/O, size cap, and malformed protobuf wire data. go vet, go test -race ./pkg/modelfile/..., and go test ./... all clean. Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
Best-effort fill of mf.format when --format is not passed. Inference keys off uniquely diagnostic signals only: saved_model.pb / saved_model.pbtxt -> tensorflow *.onnx -> onnx *.gguf -> gguf *.safetensors -> safetensors Generic .bin/.pt/.pth are intentionally NOT signals: they appear in many formats and would produce false positives on existing repos. Failure (no signal, panic on a malformed hashset value, etc.) leaves mf.format empty and degrades silently with a stderr WARNING. CLI --format always wins over inference. Format remains best-effort metadata; downstream build/push/pull paths already handle it being blank, so this never blocks generation. Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
Codex review (P2) caught that saved_model.pbtxt is not in ModelFilePatterns -- the walker lands it in CODE/DOC, so the prior inferFormat (which only scanned mf.model) silently missed the textual SavedModel variant. A workspace whose only TF signal was a .pbtxt would yield format="" instead of "tensorflow". Fix: extract the per-file scan into a closure and apply it to all four walker buckets (model + config + code + doc). Set-based: duplicates across buckets are harmless. Walker classification is unchanged. Adds TestNewModelfileByWorkspace_InferFormatSavedModelPbtxt as regression coverage and updates the doc comment to spell out the multi-bucket scan and the rationale. Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
4c24aa3 to
5c87b50
Compare
|
@copilot resolve the merge conflicts in this pull request |
# Conflicts: # go.mod Co-authored-by: bergwolf <88972+bergwolf@users.noreply.github.com>
Head branch was pushed to by a user without write access
Co-authored-by: bergwolf <88972+bergwolf@users.noreply.github.com>
Resolved by merging |
Summary
feature_mapandcheckpointtoModelFilePatternsso TF SavedModel artifacts no longer have these extension-less files misclassified as CODE.pkg/modelfile/onnx.go: parses ONNX protobuf withgoogle.golang.org/protobuf/encoding/protowireto extract everyexternal_data.locationtensor path. Generator post-processes these and reclassifies them as MODEL deterministically — independent of size.base/<ts>/+delta/<ts>/subtrees, each holding a TF SavedModel layout) work via existing recursive walk; new tests use the real timestamp directory names from a production tree.No changes to build / push / pull / attach / upload / codec / storage code paths. Those layers are byte-level and remain model-type-agnostic.
Why
Today
InferFileTypefalls back to a 128MB size heuristic for extension-less files, which misclassifies:feature_map,variables/checkpoint, etc. (small TF literals) → CODEtower_deep_layer_0_kernel_read__448_1→ CODE when small / MODEL when large (inconsistent)The fix is local to the generator — once the Modelfile globs are correct, the rest of the pipeline already works because layers are encoded by media type chosen at build time and reconstructed via
modelspec.AnnotationFilepathon pull.Test plan
go test ./pkg/modelfile/...— all new and existing tests passgo test ./...— no regression in any downstream packagego vet ./...— cleanTestNewModelfileByWorkspace/tensorflow_saved_model_directoryTestNewModelfileByWorkspace/online-learning_base_plus_delta_treeTestNewModelfileByWorkspace_ONNXExternalData(12 real ONNX external_data filenames)TestExtractONNXExternalDataPaths_*covers empty, single, multiple, dedup, missing-file, and corrupted-bytes casesOut of scope
config.jsonhandling — TF/ONNX directories don't have one and the existing skip logic is correct; users wanting metadata in the manifest can pass--family/--arch/--formattomodctl modelfile generate.