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

Officially graduate ProtypeMPRS to MPRS #1009

Closed
wants to merge 1 commit into from
Closed

Officially graduate ProtypeMPRS to MPRS #1009

wants to merge 1 commit into from

Conversation

ejguan
Copy link
Contributor

@ejguan ejguan commented Feb 13, 2023 •
edited
Loading

Summary:
Fixes: #932

  • Convert all references from ProtypeMPRS to MPRS
  • Remove usage of MPRS
  • Update colab
  • Add BC-breaking notes

Differential Revision: D43245136

BC-breaking Note:

PrototypeMultiProcessingReadingService replaces MultiProcessingReadingService.

0.5.0

MultiProcessingReadingService previously took the following arguments:

MultiProcessingReadingService(
    num_workers: int = 0,
    pin_memory: bool = False,
    timeout: float = 0,
    worker_init_fn: Optional[Callable[[int], None]] = None,
    multiprocessing_context=None,
    prefetch_factor: Optional[int] = None,
    persistent_workers: bool = False,
)

This PR

MultiProcessingReadingService takes the followng arguments:

MultiProcessingReadingService(
    num_workers: int = 0,
    multiprocessing_context: Optional[str] = None,
    worker_prefetch_cnt: int = 10,
    main_prefetch_cnt: int = 10,
    worker_init_fn: Optional[Callable[[DataPipe, WorkerInfo], DataPipe]] = None,
    worker_reset_fn: Optional[Callable[[DataPipe, WorkerInfo, SeedGenerator], DataPipe]] = None,
)

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported labels Feb 13, 2023
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D43245136

ejguan added a commit to ejguan/data that referenced this pull request Feb 13, 2023
@ejguan @facebook-github-bot
Officially graduate ProtypeMPRS to MPRS (pytorch#1009)
f70b3ce 
Summary:
Pull Request resolved: pytorch#1009

Fixes: pytorch#932

- Convert all references from `ProtypeMPRS` to `MPRS`
- Remove usage of `MPRS`

Differential Revision: D43245136

fbshipit-source-id: eccfb9f31696a4f810a9f48dd5392e1802e0028a
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D43245136

@ejguan
Copy link
Contributor Author

ejguan commented Feb 13, 2023

Needs to update colab after this PR has been landed.

NivekT
NivekT approved these changes Feb 13, 2023
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.

Wooo! Gonna need a BC-breaking/feature announcement note for this one to inform users that the arguments have changed and the internals are different.

Also need to look closely at internal/external CIs.

wenleix
wenleix approved these changes Feb 13, 2023
View reviewed changes
Copy link
Contributor

@wenleix wenleix left a comment

Choose a reason for hiding this comment

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

Awesome!!!

test/dataloader2/test_dataloader2.py
@@ -120,16 +119,6 @@ def test_dataloader2_reading_service(self) -> None:
self.assertEqual(batch, expected_batch)
expected_batch += 1

def test_dataloader2_multi_process_reading_service(self) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have dataaloder 2 tests with new graduated MPRS? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do have all prototypeMPRS in test_dataloader2. I just rename them from prototypeMPRS to MPRS in the test file.

@ejguan
Copy link
Contributor Author

ejguan commented Feb 14, 2023

Wooo! Gonna need a BC-breaking/feature announcement note for this one to inform users that the arguments have changed and the internals are different.

Sounds good. Will add notes.

@ejguan ejguan added the topic: bc_breaking topic category label Feb 14, 2023
@ejguan @facebook-github-bot
Officially graduate ProtypeMPRS to MPRS (pytorch#1009)
9707a13 
Summary:
Pull Request resolved: pytorch#1009

Fixes: pytorch#932

- Convert all references from `ProtypeMPRS` to `MPRS`
- Remove usage of `MPRS`

Reviewed By: wenleix, NivekT

Differential Revision: D43245136

fbshipit-source-id: 2195a7aae64b8ded5483f5b37383a4fa2d94f13e
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D43245136

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 73d3aa9.

facebook-github-bot pushed a commit to pytorch/tnt that referenced this pull request Feb 14, 2023
@ejguan @facebook-github-bot
Officially graduate ProtypeMPRS to MPRS (#1009)
ad45d1a 
Summary:
X-link: pytorch/data#1009

Fixes: pytorch/data#932

- Convert all references from `ProtypeMPRS` to `MPRS`
- Remove usage of `MPRS`

Reviewed By: wenleix, NivekT

Differential Revision: D43245136

fbshipit-source-id: 1fd67f0e9d55a984209b8e58edecdb6070d919cd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NivekT NivekT NivekT approved these changes

@wenleix wenleix wenleix 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. fb-exported Merged topic: bc_breaking topic category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Start to graduate PrototypeMultiProcessingReadingService from "prototype mode"
4 participants
@ejguan @facebook-github-bot @wenleix @NivekT