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.

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

Fix ArchiveReader to keep archive path #73

Closed
wants to merge 8 commits into from
Closed

Fix ArchiveReader to keep archive path #73

wants to merge 8 commits into from

Conversation

ejguan
Copy link
Contributor

@ejguan ejguan commented Oct 20, 2021 •
edited
Loading

Stack from ghstack:

Previous implementation of ArchiveReader has a bug. Please take a reference from pytorch/pytorch#65424 (comment)

cc: @Nayef211

Differential Revision: D31797765

This was referenced Oct 20, 2021
Closed
Closed
@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 Oct 20, 2021
@ejguan ejguan requested a review from Nayef211 October 20, 2021 16:05
@ejguan
Copy link
Contributor Author

ejguan commented Oct 20, 2021

The tests are passed in the next PR that triggers slowTest

@ejguan ejguan requested a review from NivekT October 20, 2021 16:08
@ejguan
Copy link
Contributor Author

ejguan commented Oct 20, 2021

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

@ejguan
Update on "Fix ArchiveReader to keep archive path"
47914eb 

Previous implementation of `ArchiveReader` has a bug. Please take a reference from pytorch/pytorch#65424 (comment)

cc: @Nayef211

Differential Revision: [D31797765](https://our.internmc.facebook.com/intern/diff/D31797765)

[ghstack-poisoned]
@ejguan
Copy link
Contributor Author

ejguan commented Oct 20, 2021

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

@ejguan
Update on "Fix ArchiveReader to keep archive path"
a5d41fb 

Previous implementation of `ArchiveReader` has a bug. Please take a reference from pytorch/pytorch#65424 (comment)

cc: @Nayef211

Differential Revision: [D31797765](https://our.internmc.facebook.com/intern/diff/D31797765)

[ghstack-poisoned]
@ejguan
Copy link
Contributor Author

ejguan commented Oct 20, 2021

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

@ejguan
Update on "Fix ArchiveReader to keep archive path"
30210df 

Previous implementation of `ArchiveReader` has a bug. Please take a reference from pytorch/pytorch#65424 (comment)

cc: @Nayef211

Differential Revision: [D31797765](https://our.internmc.facebook.com/intern/diff/D31797765)

[ghstack-poisoned]
@ejguan
Update on "Fix ArchiveReader to keep archive path"
a35a384 

Previous implementation of `ArchiveReader` has a bug. Please take a reference from pytorch/pytorch#65424 (comment)

cc: @Nayef211

Differential Revision: [D31797765](https://our.internmc.facebook.com/intern/diff/D31797765)

[ghstack-poisoned]
@ejguan
Update on "Fix ArchiveReader to keep archive path"
85c5288 

Previous implementation of `ArchiveReader` has a bug. Please take a reference from pytorch/pytorch#65424 (comment)

cc: @Nayef211

Differential Revision: [D31797765](https://our.internmc.facebook.com/intern/diff/D31797765)

[ghstack-poisoned]
NivekT
NivekT approved these changes Oct 20, 2021
View reviewed changes
Copy link
Contributor

@NivekT NivekT left a comment

Choose a reason for hiding this comment

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

LGTM

Nayef211
Nayef211 reviewed Oct 20, 2021
View reviewed changes
Copy link

@Nayef211 Nayef211 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this @ejguan. I recently created a PR upstreaming the SST2 dataset from torchdata to torchtext (pytorch/text#1410). Moving forward, do we want to remove the SST2 dataset in torchdata and have a dependency on the torchtext dataset instead?

As a followup, should we also be updating the SST2 dataset within torchtext with the changed you've made here to the file paths?

@ejguan
Copy link
Contributor Author

ejguan commented Oct 20, 2021

@Nayef211 Thanks for the reminder!

I recently created a PR upstreaming the SST2 dataset from torchdata to torchtext (pytorch/text#1410). Moving forward, do we want to remove the SST2 dataset in torchdata and have a dependency on the torchtext dataset instead?

I will remove SST from TorchData then. Do you mind creating a list of dataset that you have implemented (and updating them when new dataset is implemented) with code pointer in this issue? #69
Then, we can reference tests in TorchData.

As a followup, should we also be updating the SST2 dataset within torchtext with the changed you've made here to the file paths?

Yes, please.

@ejguan
Update on "Fix ArchiveReader to keep archive path"
cc71cdc 

Previous implementation of `ArchiveReader` has a bug. Please take a reference from pytorch/pytorch#65424 (comment)

cc: @Nayef211

Differential Revision: [D31797765](https://our.internmc.facebook.com/intern/diff/D31797765)

[ghstack-poisoned]
@ejguan
Copy link
Contributor Author

ejguan commented Oct 20, 2021

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

@ejguan ejguan mentioned this pull request Oct 20, 2021
Merged
@ejguan
Update on "Fix ArchiveReader to keep archive path"
7d0925b 

Previous implementation of `ArchiveReader` has a bug. Please take a reference from pytorch/pytorch#65424 (comment)

cc: @Nayef211

Differential Revision: [D31797765](https://our.internmc.facebook.com/intern/diff/D31797765)

[ghstack-poisoned]
@ejguan
Copy link
Contributor Author

ejguan commented Oct 20, 2021

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

@Nayef211
Copy link

As a followup, you may also need to update the notebook with usage examples since you're removing the SST2 dataset from torchdata

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in d28a139.

@ejguan ejguan mentioned this pull request Oct 21, 2021
Closed
facebook-github-bot pushed a commit to pytorch/pytorch that referenced this pull request Oct 22, 2021
@ejguan @facebook-github-bot
Fix ArchiveReader to keep archive path (#67035)
3d7a344 
Summary:
Pull Request resolved: #67035

Incorporate the same change from pytorch/data#73

Test Plan: Imported from OSS

Reviewed By: NivekT

Differential Revision: D31837963

Pulled By: ejguan

fbshipit-source-id: 3b0171ba30f392c8773c497702bc60aa4fbe28c6
@facebook-github-bot facebook-github-bot deleted the gh/ejguan/7/head branch October 24, 2021 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nayef211 Nayef211 Nayef211 left review comments

@NivekT NivekT NivekT approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ejguan @Nayef211 @facebook-github-bot @NivekT