FileDataLoader fails to read the file when size > INT32_MAX

[cymbalrush]

On macOS, the read function will fail with an EINVAL error if the size parameter exceeds INT32_MAX. This update addresses the issue by adding a check to ensure that the read size does not surpass INT32_MAX. On Linux, the maximum permissible read size is 2,147,479,552 bytes ( < INT32_MAX), so attempting to read beyond this limit is inconsequential.

Test Plan:

Exporting llama3 with python -m examples.models.llama2.export_llama --checkpoint examples/models/llama-2-7B/consolidated.00.pth --params examples/models/llama-2-7B/params.json --coreml --disable_dynamic_shape -kv

Without fix
Fails with invalid argument error.

With fix
Succeeds.

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/4435

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 Cancelled Job

As of commit 0a3116c with merge base dbf7d6e ():

CANCELLED JOB - The following job was cancelled. Please retry:

• pull / unittest / macos / macos-job (gh)
  ##[error]The operation was canceled.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[dbort]

Thank you so much for this fix! One super-minor request but the code changes look great.

I would suggest adding a unit test for this, but I really don't want our test jobs creating and reading a >2GB file. But because there's no automated testing, please add a Test Plan: section to the PR description explaining how you tested that this works.

[dbort]

Please begin comments with uppercase letters and end with a period, to be consistent with the rest of the file. Thanks for adding this comment, though, to explain why this extra logic is necessary, and where to focus testing if it changes.

Suggested change

	// read on macos would fail with EINVAL if size > INT32_MAX
	// Reads on macOS will fail with EINVAL if size > INT32_MAX.

[dbort]

When you get a chance please update this comment, then we can land the PR

[cymbalrush]

Done.

[cymbalrush]

Thank you so much for this fix! One super-minor request but the code changes look great.

I would suggest adding a unit test for this, but I really don't want our test jobs creating and reading a >2GB file. But because there's no automated testing, please add a Test Plan: section to the PR description explaining how you tested that this works.

That's the reason for not adding test, I didn't want to create a 2GB file for unit testing this.

[facebook-github-bot]

@shoumikhin has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

@shoumikhin has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

@shoumikhin merged this pull request in 1e4603d.