Skip to content

Allow ONNX imports without external data files - #2

Merged
justinchuby merged 6 commits into
mainfrom
copilot/fix-model-parsing-requirements
Aug 12, 2026
Merged

Allow ONNX imports without external data files#2
justinchuby merged 6 commits into
mainfrom
copilot/fix-model-parsing-requirements

Conversation

Copilot AI commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

ONNX model uploads failed when referenced external-data sidecars were absent, although simulation only requires initializer metadata embedded in the model protobuf.

  • Model inspection

    • Allow external-data resolution to return no source.
    • Preserve initializer locations, extents, and memory totals.
    • Continue hashing and range-validating supplied sidecars.
  • Browser import

    • Import standalone .onnx files without requiring weight data.
    • Continue rejecting unsafe external-data paths.
  • Coverage and documentation

    • Add regressions for missing sidecars and path traversal.
    • Document optional browser sidecars and verification behavior.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copilot AI linked an issue Aug 12, 2026 that may be closed by this pull request
@justinchuby

Copy link
Copy Markdown
Member

Copilot AI and others added 2 commits August 12, 2026 17:56
Co-authored-by: justinchuby <11205048+justinchuby@users.noreply.github.com>
Co-authored-by: justinchuby <11205048+justinchuby@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix model parsing to not require the data file Allow ONNX imports without external data files Aug 12, 2026
Copilot AI requested a review from justinchuby August 12, 2026 18:05
Co-authored-by: justinchuby <11205048+justinchuby@users.noreply.github.com>

Copilot AI commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

@copilot you can test with https://huggingface.co/justinchuby/Muse-Glimmer-30B-ONNX-INT4-CUDA

Hugging Face DNS is blocked in this runner, so I couldn’t download that graph directly. I validated the same external-data encoding with the regression fixture and fixed two related bare-model edge cases in 294d677 (./ sidecar paths and metadata-free architecture classification). Targeted tests and builds pass.

@justinchuby

Copy link
Copy Markdown
Member

@copilot it should just never look at any external data file, because they can be very large

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the ONNX inspection/import pipeline so browser uploads can successfully inspect standalone .onnx protobufs even when referenced external-data sidecars are absent, while still preserving initializer external-data metadata and validating/hash-checking any sidecars that are provided.

Changes:

  • Allow resolveExternalData() to return undefined and skip external sidecar hashing/range validation when the sidecar is not supplied.
  • Update browser model-package import to treat missing sidecars as optional, while still rejecting unsafe external-data locations.
  • Add/adjust tests and documentation to cover missing sidecars, path normalization, and traversal rejection.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
packages/web/src/model-package-import.ts Makes browser import tolerant of missing external-data sidecars and avoids passing dummy metadata into ONNX architecture normalization.
packages/web/src/model-package-import.test.ts Adds coverage for missing sidecars, relative-path normalization, and unsafe-path rejection.
packages/onnx-inspector/src/index.ts Extends the inspector resolver contract to allow “no sidecar supplied” and normalizes external-data locations.
docs/DESIGN.md Documents that browser imports may omit external-data files while still validating/hash-checking provided sidecars.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Co-authored-by: justinchuby <11205048+justinchuby@users.noreply.github.com>

Copilot AI commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

@copilot it should just never look at any external data file, because they can be very large

Addressed in 9804113: ONNX inspection now never opens, stats, hashes, or range-validates external data sidecar files. It only preserves the external initializer metadata from the protobuf and still rejects unsafe external-data locations.

@justinchuby

Copy link
Copy Markdown
Member

@copilot it should just never look at any external data file, because they can be very large

Addressed in 9804113: ONNX inspection now never opens, stats, hashes, or range-validates external data sidecar files. It only preserves the external initializer metadata from the protobuf and still rejects unsafe external-data locations.

@copilot no need to reject anything if you are not reading it in the first place. External data validation is not this apps concern.

Co-authored-by: justinchuby <11205048+justinchuby@users.noreply.github.com>

Copilot AI commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

@copilot it should just never look at any external data file, because they can be very large
...

Addressed in ca42a54: external-data locations are now treated only as ONNX metadata. The inspector preserves them verbatim (or omits them if missing), and browser/CLI imports no longer reject unsafe-looking external-data paths while still never opening sidecars.

@justinchuby
justinchuby marked this pull request as ready for review August 12, 2026 22:52
@justinchuby
justinchuby merged commit c9a96d0 into main Aug 12, 2026
2 of 3 checks passed
@justinchuby
justinchuby deleted the copilot/fix-model-parsing-requirements branch August 12, 2026 22:53
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.

Model parsing should not require the data file

4 participants