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

Add comprehensive serialization tests #172

Closed
4 tasks done
ejguan opened this issue Jan 19, 2022 · 6 comments
Closed
4 tasks done

Add comprehensive serialization tests #172

ejguan opened this issue Jan 19, 2022 · 6 comments

Comments

@ejguan
Copy link
Contributor

ejguan commented Jan 19, 2022

🚀 The feature

We have serialization tests in PyTorch Core https://github.com/pytorch/pytorch/blob/b56ba296b1cb5d65a3fe2e33cc1d910481baa644/test/test_datapipe.py#L431

There are a few more things needed:

  • Cover all DataPipe
    • Dill test for DataPipes that accept function as input
  • Cover DataPipe serialization after iteration starts
  • Cover serialization for factory DataPipe (1-to-N) like demux and fork, especially when one of children is not depleted

Motivation, pitch

These tests are going to make sure DataLoader graph can be created. And, for forward compatibility, snapshotting also requires DataPipe serializable.

Alternatives

No response

Additional context

No response

@NivekT
Copy link
Contributor

NivekT commented Jan 19, 2022

I just added these PRs related to this issue:

pytorch/pytorch#71456
pytorch/pytorch#71497

Those additions should cover the basic serialization (first checkbox), but we need more work the remaining ones.

Another thing to add may be - if a DataPipe takes a function as argument, it should be serializable with dill?

@ejguan
Copy link
Contributor Author

ejguan commented Jan 19, 2022

Another thing to add may be - if a DataPipe takes a function as argument, it should be serializable with dill?

Please correct me if I misunderstand it. I think the test has been added https://github.com/pytorch/pytorch/blob/f45e217c0158c247772f6491a18662cc972c29cd/test/test_datapipe.py#L445

@NivekT
Copy link
Contributor

NivekT commented Jan 19, 2022

Another thing to add may be - if a DataPipe takes a function as argument, it should be serializable with dill?

Please correct me if I misunderstand it. I think the test has been added https://github.com/pytorch/pytorch/blob/f45e217c0158c247772f6491a18662cc972c29cd/test/test_datapipe.py#L445

Yep, the test has been added but we need to make sure all relevant DataPipes are added to the test.

@ejguan
Copy link
Contributor Author

ejguan commented Jan 19, 2022

Another thing to add may be - if a DataPipe takes a function as argument, it should be serializable with dill?

Please correct me if I misunderstand it. I think the test has been added https://github.com/pytorch/pytorch/blob/f45e217c0158c247772f6491a18662cc972c29cd/test/test_datapipe.py#L445

Yep, the test has been added but we need to make sure all relevant DataPipes are added to the test.

Sounds great.

facebook-github-bot pushed a commit to pytorch/pytorch that referenced this issue Jan 21, 2022
Summary:
Pull Request resolved: #71456

Related to pytorch/data#172

cc VitalyFedyunin ejguan NivekT

Test Plan: Imported from OSS

Reviewed By: zou3519

Differential Revision: D33668748

Pulled By: NivekT

fbshipit-source-id: ea2085d5ed47533ca49258cc52471373c6ae1847
facebook-github-bot pushed a commit to pytorch/pytorch that referenced this issue Jan 21, 2022
Summary:
Pull Request resolved: #71497

Related to pytorch/data#172

cc VitalyFedyunin ejguan NivekT

Test Plan: Imported from OSS

Reviewed By: ejguan

Differential Revision: D33668749

Pulled By: NivekT

fbshipit-source-id: 6506614e9d4389dc645d8985c00fdb3402122d9b
pytorchmergebot pushed a commit to pytorch/pytorch that referenced this issue Jan 21, 2022
Summary:
Pull Request resolved: #71456

Related to pytorch/data#172

cc VitalyFedyunin ejguan NivekT

Test Plan: Imported from OSS

Reviewed By: zou3519

Differential Revision: D33668748

Pulled By: NivekT

fbshipit-source-id: ea2085d5ed47533ca49258cc52471373c6ae1847
(cherry picked from commit d5f6fde)
pytorchmergebot pushed a commit to pytorch/pytorch that referenced this issue Jan 21, 2022
Summary:
Pull Request resolved: #71497

Related to pytorch/data#172

cc VitalyFedyunin ejguan NivekT

Test Plan: Imported from OSS

Reviewed By: ejguan

Differential Revision: D33668749

Pulled By: NivekT

fbshipit-source-id: 6506614e9d4389dc645d8985c00fdb3402122d9b
(cherry picked from commit 458e76f)
cyyever pushed a commit to cyyever/pytorch_private that referenced this issue Feb 3, 2022
Summary:
Pull Request resolved: pytorch/pytorch#71456

Related to pytorch/data#172

cc VitalyFedyunin ejguan NivekT

Test Plan: Imported from OSS

Reviewed By: zou3519

Differential Revision: D33668748

Pulled By: NivekT

fbshipit-source-id: ea2085d5ed47533ca49258cc52471373c6ae1847
(cherry picked from commit d5f6fde)
cyyever pushed a commit to cyyever/pytorch_private that referenced this issue Feb 3, 2022
Summary:
Pull Request resolved: pytorch/pytorch#71497

Related to pytorch/data#172

cc VitalyFedyunin ejguan NivekT

Test Plan: Imported from OSS

Reviewed By: ejguan

Differential Revision: D33668749

Pulled By: NivekT

fbshipit-source-id: 6506614e9d4389dc645d8985c00fdb3402122d9b
(cherry picked from commit 458e76f)
cyyever pushed a commit to cyyever/pytorch_private that referenced this issue Feb 3, 2022
Summary:
Pull Request resolved: pytorch/pytorch#71456

Related to pytorch/data#172

cc VitalyFedyunin ejguan NivekT

Test Plan: Imported from OSS

Reviewed By: zou3519

Differential Revision: D33668748

Pulled By: NivekT

fbshipit-source-id: ea2085d5ed47533ca49258cc52471373c6ae1847
(cherry picked from commit d5f6fde)
cyyever pushed a commit to cyyever/pytorch_private that referenced this issue Feb 3, 2022
Summary:
Pull Request resolved: pytorch/pytorch#71497

Related to pytorch/data#172

cc VitalyFedyunin ejguan NivekT

Test Plan: Imported from OSS

Reviewed By: ejguan

Differential Revision: D33668749

Pulled By: NivekT

fbshipit-source-id: 6506614e9d4389dc645d8985c00fdb3402122d9b
(cherry picked from commit 458e76f)
cyyever pushed a commit to cyyever/pytorch_private that referenced this issue Feb 9, 2022
Summary:
Pull Request resolved: pytorch/pytorch#71456

Related to pytorch/data#172

cc VitalyFedyunin ejguan NivekT

Test Plan: Imported from OSS

Reviewed By: zou3519

Differential Revision: D33668748

Pulled By: NivekT

fbshipit-source-id: ea2085d5ed47533ca49258cc52471373c6ae1847
(cherry picked from commit d5f6fde)
cyyever pushed a commit to cyyever/pytorch_private that referenced this issue Feb 9, 2022
Summary:
Pull Request resolved: pytorch/pytorch#71497

Related to pytorch/data#172

cc VitalyFedyunin ejguan NivekT

Test Plan: Imported from OSS

Reviewed By: ejguan

Differential Revision: D33668749

Pulled By: NivekT

fbshipit-source-id: 6506614e9d4389dc645d8985c00fdb3402122d9b
(cherry picked from commit 458e76f)
cyyever pushed a commit to cyyever/pytorch_private that referenced this issue Feb 9, 2022
Summary:
Pull Request resolved: pytorch/pytorch#71456

Related to pytorch/data#172

cc VitalyFedyunin ejguan NivekT

Test Plan: Imported from OSS

Reviewed By: zou3519

Differential Revision: D33668748

Pulled By: NivekT

fbshipit-source-id: ea2085d5ed47533ca49258cc52471373c6ae1847
(cherry picked from commit d5f6fde)
cyyever pushed a commit to cyyever/pytorch_private that referenced this issue Feb 9, 2022
Summary:
Pull Request resolved: pytorch/pytorch#71497

Related to pytorch/data#172

cc VitalyFedyunin ejguan NivekT

Test Plan: Imported from OSS

Reviewed By: ejguan

Differential Revision: D33668749

Pulled By: NivekT

fbshipit-source-id: 6506614e9d4389dc645d8985c00fdb3402122d9b
(cherry picked from commit 458e76f)
@NivekT
Copy link
Contributor

NivekT commented Feb 24, 2022

Note that pytorch/pytorch#73119 addresses these requirements, we still need to do the same for the DataPipes in TorchData.

Also, see #106 for individual DataPipe's test coverage.

@ejguan
Copy link
Contributor Author

ejguan commented Apr 28, 2022

Closing this issue as we rely on #106 to track test coverage.

@ejguan ejguan closed this as completed Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants