[DCP] Cache save plans: planner helpers and interface updates #147116
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/147116
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 1 Unrelated FailureAs of commit 27fe73e with merge base 06f4a5c ( NEW FAILURE - The following job has failed:
BROKEN TRUNK - The following job failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
This pull request was exported from Phabricator. Differential Revision: D69224488 |
c9e1453 to
8000
3cee524
Compare
|
This pull request was exported from Phabricator. Differential Revision: D69224488 |
3cee524 to
9ba1785
Compare
|
This pull request was exported from Phabricator. Differential Revision: D69224488 |
9ba1785 to
20a950b
Compare
|
This pull request was exported from Phabricator. Differential Revision: D69224488 |
20a950b to
d556b9f
Compare
|
This pull request was exported from Phabricator. Differential Revision: D69224488 |
1 similar comment
|
This pull request was exported from Phabricator. Differential Revision: D69224488 |
8eff3e1 to
bc3927e
Compare
|
This pull request was exported from Phabricator. Differential Revision: D69224488 |
bc3927e to
9b74f41
Compare
|
This pull request was exported from Phabricator. Differential Revision: D69224488 |
|
This pull request was exported from Phabricator. Differential Revision: D69224488 |
| _cached_save_plan: Dict[str, SavePlan] = {} | ||
| _cached_final_save_plan: Dict[str, SavePlan] = {} | ||
| _cached_all_plans: Dict[str, list[SavePlan]] = {} | ||
| _cached_global_plan: Dict[str, list[SavePlan]] = {} | ||
|
|
There was a problem hiding this comment.
Should we add some doc comments on what each cached variable could be used for? Intention is to provide clarity to users who could use in their custom planner implementation.
There was a problem hiding this comment.
Took a look at the next PR in the stack, I am not sure what's the difference between _cached_final_save_plan and _cached_save_plan.
There was a problem hiding this comment.
Yes. Let me add that now. _cached_save_plan is the local plan produced by the _create_local_plan. However this plan can change after de-dupe or during global plan creation due to additional metadata. Hence we can not use that to send to the write_data method. So basically, _cached_save_plan is the local plan and _cached_final_save_plan is the plan after de-dupe and any additional metadata during global_plan creation.
There was a problem hiding this comment.
Updated the code documentation for the different cached plans. Thanks @MeetVadakkanchery
acec49e to
2bc4c35
Compare
…h#147116) Summary: This PR updates the planner interface and introduces the class variables to cache the local and global plans. Two new helpers are also introduced which will be used to compare if the plans have changed across save attempts and merge the delta plans. Test Plan: UTs Differential Revision: D69224488
|
This pull request was exported from Phabricator. Differential Revision: D69224488 |
…h#147116) Summary: This PR updates the planner interface and introduces the class variables to cache the local and global plans. Two new helpers are also introduced which will be used to compare if the plans have changed across save attempts and merge the delta plans. Test Plan: UTs Reviewed By: MeetVadakkanchery Differential Revision: D69224488
2bc4c35 to
27fe73e
Compare
|
This pull request was exported from Phabricator. Differential Revision: D69224488 |
|
@pytorchbot merge (Initiating merge automatically since Phabricator Diff has merged) |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Merge failedReason: 1 jobs have failed, first few of them are: trunk / macos-py3-arm64 / test (default, 1, 3, macos-m1-stable) Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot merge -f 'The macos failure might be related but I am not sure' As this PR has been landed internally, I will merge it. But if the failure shows up in trunk, it will be reverted |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
This PR needs a
|
Summary:
This PR updates the planner interface and introduces the class variables to cache the local and global plans.
Two new helpers are also introduced which will be used to compare if the plans have changed across save attempts and merge the delta plans.
Test Plan: UTs
Differential Revision: D69224488
cc @H-Huang @awgu @kwen2501 @wanchaol @fegin @fduwjj @wz337 @wconstab @d4l3k @c-p-i-o @LucasLLC @MeetVadakkanchery @mhorowitz @pradeepfn @ekr0