Skip to content

[onnx_importer] Fix bug with path-based shape inference when --temp-dir is set and --data-dir is not #4375

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.

Sign up for GitHub

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

Jump to bottom
Merged
zjgarvey merged 1 commit into from
Dec 21, 2025
Merged

[onnx_importer] Fix bug with path-based shape inference when --temp-dir is set and --data-dir is not #4375

zjgarvey merged 1 commit into from
Dec 21, 2025
+72 −19

Conversation

@tkocmathla
Copy link
Contributor

@tkocmathla tkocmathla commented Nov 12, 2025 •
edited
Loading

Fixes #4210.

Fix data_dir assignment and add tests for 2 new cases:

  1. When --temp-dir is set and --data-dir is not and path-based shape inference is used
  2. When raw_model_modified and model uses external data

@sjarus sjarus requested a review from zjgarvey December 9, 2025 17:47
zjgarvey
zjgarvey requested changes Dec 10, 2025
View reviewed changes
Copy link
Collaborator

@zjgarvey zjgarvey left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for finding this. It does seem strange that we would be checking for an explicit args.temp_dir for the external data.

I have a few comments. Most importantly, I think we should find a way to test without needing to generate a 2GB random tensor.

@tkocmathla tkocmathla requested a review from zjgarvey December 15, 2025 15:14
@tkocmathla
Fix llvm#4210.
b54d281 
Fix data_dir assignment and add tests for 2 new cases:
  1. When `--temp-dir` is set and `--data-dir` is not
  2. When `raw_model_modified` and model uses external data
@tkocmathla
Copy link
Contributor Author

tkocmathla commented Dec 17, 2025

@zjgarvey would you mind kicking the CI and doing another review? 🙏🏻

zjgarvey
zjgarvey approved these changes Dec 21, 2025
View reviewed changes
Copy link
Collaborator

@zjgarvey zjgarvey left a comment

Choose a reason for hiding this comment

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

LGTM

@zjgarvey zjgarvey merged commit 7755008 into llvm:main Dec 21, 2025
3 checks passed
@tkocmathla tkocmathla deleted the matt/issue-4210 branch December 22, 2025 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zjgarvey zjgarvey zjgarvey approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ONNX importer fails when --temp-dir is set and --data-dir is not

2 participants

@tkocmathla @zjgarvey