-
Notifications
You must be signed in to change notification settings - Fork 607
[AOSP] add file descriptor support to file_data_loader #6611
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.
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
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/6611
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New FailureAs of commit 83f3ff9 with merge base 97a4600 ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
@@ -26,6 +26,24 @@ namespace extension { | |||
*/ | |||
class FileDataLoader final : public executorch::runtime::DataLoader { | |||
public: | |||
/** | |||
* Creates a new FileDataLoader that wraps the named file descriptor. | |||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be mention that the fd ownership will be passed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does passing the FD ownership mean that ET is responsible for closing the file afterwards? or is ownership of the FD independent of ownership of the file opened?
edit: read the destructor for FileDataLoader, makes sense now!
@@ -0,0 +1,6 @@ | |||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not necessary to put this file to repo. Maybe just mention it in commit message?
Overall LGTM! Let's try to merge to main, not I will rebase |
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
int fd = parsed_fd.get(); | ||
|
||
// Cache the file size. | ||
struct stat st; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it's possible to factor the common code between here and FileDataLoader::from
?
@kirklandsign moved centralized logic to a private method |
Summary: pytorch#6611 causes internal builds to break due to missing static qualifier. adding it here Differential Revision: D65490319
This reverts commit 3a1f8d2.
This shouldn't have been part of FileDataLoader; we should create a new DataLoader with this functionality. This also caused the test-binary-size-linux-gcc job to start failing: https://github.com/pytorch/executorch/actions/runs/11674336719/job/32509252770 |
Summary:
For Google's ODP, the model will execute in a process that does not have access to the file system. Instead, the caller must open the file then pass ownership of the FD to the model executor.
file_data_loader
.Test Plan:
result: