Skip to content

Adding ability to redefine cache timeout#571

Closed
VitalyFedyunin wants to merge 7 commits intogh/VitalyFedyunin/12/basefrom
gh/VitalyFedyunin/12/head
Closed

Adding ability to redefine cache timeout#571
VitalyFedyunin wants to merge 7 commits intogh/VitalyFedyunin/12/basefrom
gh/VitalyFedyunin/12/head

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: 48b0d5c
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: 9b9907f
Pull Request resolved: #571
VitalyFedyunin added a commit that referenced this pull request Jul 5, 2022
ghstack-source-id: fbda8a5
Pull Request resolved: #571
VitalyFedyunin added a commit that referenced this pull request Jul 5, 2022
ghstack-source-id: 004abd5
Pull Request resolved: #571
@VitalyFedyunin VitalyFedyunin requested a review from parmeet July 5, 2022 18:20
@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: b131266
Pull Request resolved: #571
VitalyFedyunin added a commit that referenced this pull request Jul 8, 2022
ghstack-source-id: 1795da5
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: 09b4f52
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