Skip to content
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

Adding ability to redefine cache timeout #571

Closed

Conversation

VitalyFedyunin
Copy link
Contributor

@VitalyFedyunin VitalyFedyunin commented Jul 5, 2022

Stack from ghstack (oldest at bottom):

Differential Revision: D37723437

VitalyFedyunin added a commit that referenced this pull request Jul 5, 2022
ghstack-source-id: 48b0d5cdc8e0a1c46e4017a498f66ad2422fb228
Pull Request resolved: #571
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 5, 2022
VitalyFedyunin added a commit that referenced this pull request Jul 5, 2022
ghstack-source-id: 9b9907fc7708b0dd3c0db24604b933ef102b5ab5
Pull Request resolved: #571
VitalyFedyunin added a commit that referenced this pull request Jul 5, 2022
ghstack-source-id: fbda8a5f9794af5b9ade98aa1f3981c45812adae
Pull Request resolved: #571
VitalyFedyunin added a commit that referenced this pull request Jul 5, 2022
ghstack-source-id: 004abd582a60b42dbc7d678e3dcd6677b9537640
Pull Request resolved: #571
@VitalyFedyunin
Copy link
Contributor Author

@parmeet can you please review this code (@ejguan and @NivekT are on PTO)

Copy link
Contributor

@ejguan ejguan left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

I just realize one general question for Adapter. Since adapters would modify the DataPipe graph in-place, do we want to revert the modification for the DataPipe graph after one?epoch?

@VitalyFedyunin
Copy link
Contributor Author

IMO DLv2 should apply adapter modifications once on the local copy of the graph it gets from the args.

VitalyFedyunin added a commit that referenced this pull request Jul 8, 2022
ghstack-source-id: b131266792bf3f3a7c6d9dec8e7451d2fb36015c
Pull Request resolved: #571
VitalyFedyunin added a commit that referenced this pull request Jul 8, 2022
ghstack-source-id: 1795da50e7121358c20285d288d8f9a29680116c
Pull Request resolved: #571
@VitalyFedyunin
Copy link
Contributor Author

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

VitalyFedyunin added a commit that referenced this pull request Jul 19, 2022
ghstack-source-id: 09b4f529a24692aed75c382825d530da1c81c061
Pull Request resolved: #571
@VitalyFedyunin
Copy link
Contributor Author

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

Copy link
Contributor

@parmeet parmeet left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @VitalyFedyunin for adding the ability to manually set time for caching files. Just to be sure, the users can add this adapter on top of datapipe we return for datasets, in other words, we do not have to add this adapter internally in the dataset implementation right?

@VitalyFedyunin
Copy link
Contributor Author

LGTM! Thanks @VitalyFedyunin for adding the ability to manually set time for caching files. Just to be sure, the users can add this adapter on top of datapipe we return for datasets, in other words, we do not have to add this adapter internally in the dataset implementation right?

Correct

@facebook-github-bot facebook-github-bot deleted the gh/VitalyFedyunin/12/head branch July 23, 2022 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants