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

DAOS-16362: pydaos.torch checkpointing #15691

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

DAOS-16362: pydaos.torch checkpointing #15691

wants to merge 20 commits into from
+1,156 −120

Conversation

enakta
Copy link

@enakta enakta commented Jan 7, 2025 •
edited
Loading

Introducing PyTorch checkpoint interface and user documentation for pydaos.torch module.

Before requesting gatekeeper:

  • Two review approvals and any prior change requests have been resolved.
  • Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
  • Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
  • Commit messages follows the guidelines outlined here.
  • Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

  • You are the appropriate gatekeeper to be landing the patch.
  • The PR has 2 reviews by people familiar with the code, including appropriate owners.
  • Githooks were used. If not, request that user install them and check copyright dates.
  • Checkpatch issues are resolved. Pay particular attention to ones that will show up on future PRs.
  • All builds have passed. Check non-required builds for any new compiler warnings.
  • Sufficient testing is done. Check feature pragmas and test tags and that tests skipped for the ticket are run and now pass with the changes.
  • If applicable, the PR has addressed any potential version compatibility issues.
  • Check the target branch. If it is master branch, should the PR go to a feature branch? If it is a release branch, does it have merge approval in the JIRA ticket.
  • Extra checks if forced landing is requested
    • Review comments are sufficiently resolved, particularly by prior reviewers that requested changes.
    • No new NLT or valgrind warnings. Check the classic view.
    • Quick-build or Quick-functional is not used.
  • Fix the commit message upon landing. Check the standard here. Edit it to create a single commit. If necessary, ask submitter for a new summary.

@enakta enakta requested review from a team as code owners January 7, 2025 05:35
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 7, 2025

Errors are component not formatted correctly,Ticket number suffix is not a number. See https://daosio.atlassian.net/wiki/spaces/DC/pages/11133911069/Commit+Comments,Unable to load ticket data
https://daosio.atlassian.net/browse/DAOS-16362:

@enakta enakta marked this pull request as draft January 7, 2025 05:36
@daosbuild1
Copy link
Collaborator

Test stage Python Bandit check completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15691/1/execution/node/133/log

@daosbuild1
Copy link
Collaborator

Test stage Python Bandit check completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15691/2/execution/node/134/log

@0xE0F 0xE0F force-pushed the 0xe0f/pydaos.torch.checkpointing branch from 91c5ab5 to 9edb95b Compare January 7, 2025 06:15
@daosbuild1
Copy link
Collaborator

Test stage Python Bandit check completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15691/3/execution/node/134/log

@0xE0F 0xE0F force-pushed the 0xe0f/pydaos.torch.checkpointing branch from 9edb95b to 60a4612 Compare January 7, 2025 06:20
0xE0F added 8 commits January 7, 2025 17:21
@0xE0F
client: initialize DAOS on Dataset creation rather than on module import
28b2d49 
Signed-off-by: Denis Barakhtanov <[email protected]>
@0xE0F
tests: PoC of torch tests
ffdcd72 
Signed-off-by: Denis Barakhtanov <[email protected]>
@0xE0F
client: torch module needs lazy DAOS init
80290e0 
Calling daos_init on Python module import creates challenges for
functional tests (and for some user cases) when DAOS agent is not ready
yet.

Better solution is to try initialize DAOS lib when the Dataset is
created.

Signed-off-by: Denis Barakhtanov <[email protected]>
@0xE0F
ftests: add tests for pydaos.torch.Dataset
cf78260 
Signed-off-by: Denis Barakhtanov <[email protected]>
@0xE0F
client: add checkpoint interface to pydaos.torch
ae0375c 
Signed-off-by: Denis Barakhtanov <[email protected]>
@0xE0F
ftest: torch checkpoint test
44367ba 
Signed-off-by: Denis Barakhtanov <[email protected]>
@0xE0F
docs: for pydaos.torch module
cfb76a4 
Signed-off-by: Denis Barakhtanov <[email protected]>
@0xE0F
Linter fixes
e72b63d 
Ave linters !

Signed-off-by: Denis Barakhtanov <[email protected]>
@0xE0F 0xE0F force-pushed the 0xe0f/pydaos.torch.checkpointing branch from 60a4612 to e72b63d Compare January 7, 2025 06:21
@daosbuild1
Copy link
Collaborator

Test stage Python Bandit check completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15691/4/execution/node/135/log

@daosbuild1
Copy link
Collaborator

Test stage Unit Test with memcheck on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-15691/3/display/redirect

@daosbuild1
Copy link
Collaborator

Test stage Unit Test bdev on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-15691/3/display/redirect

@daosbuild1
Copy link
Collaborator

Test stage Unit Test on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-15691/3/display/redirect

@daosbuild1
Copy link
Collaborator

Test stage Unit Test bdev with memcheck on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-15691/3/display/redirect

@daosbuild1
Copy link
Collaborator

Test stage Python Bandit check completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15691/5/execution/node/135/log

daltonbohning
daltonbohning reviewed Jan 7, 2025
View reviewed changes
Copy link
Contributor

@daltonbohning daltonbohning left a comment

Choose a reason for hiding this comment

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

Mostly nits. Overall looks good!

Since directory_tree.py was modified we should run the test that uses it. You can do that by including this string in future commit messages

Features: DfuseFind

docs/user/pytorch.md Outdated Show resolved Hide resolved
docs/user/pytorch.md Outdated Show resolved Hide resolved
docs/user/pytorch.md Outdated Show resolved Hide resolved
docs/user/pytorch.md Outdated Show resolved Hide resolved
docs/user/pytorch.md Outdated Show resolved Hide resolved
src/client/pydaos/torch/Readme.md Outdated Show resolved Hide resolved
src/client/pydaos/torch/torch_api.py Outdated Show resolved Hide resolved
src/tests/ftest/directory_tree.py Show resolved Hide resolved
src/tests/ftest/pytorch/map_dataset.py Outdated Show resolved Hide resolved
src/tests/ftest/pytorch/map_dataset.py Outdated Show resolved Hide resolved
mchaarawi
mchaarawi previously approved these changes Jan 7, 2025
View reviewed changes
Copy link
Contributor

@mchaarawi mchaarawi left a comment

Choose a reason for hiding this comment

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

the C shim part looks good to me. no changes requested; just some clarifications / comments added.

src/client/pydaos/torch/torch_shim.c Show resolved Hide resolved
src/client/pydaos/torch/torch_shim.c Outdated Show resolved Hide resolved
src/client/pydaos/torch/torch_shim.c Outdated Show resolved Hide resolved
src/client/pydaos/torch/torch_shim.c Outdated Show resolved Hide resolved
src/client/pydaos/torch/torch_shim.c

assert(hdl->dfs != NULL);

int rc = dfs_lookup(hdl->dfs, path, O_RDONLY, &obj, NULL, &st);
Copy link
Contributor

Choose a reason for hiding this comment

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

we can optimize this a bit if the dir path is cached to just call dfs_stat. but for now this is fine.

@enakta @daltonbohning
Apply suggestions from code review
f3eafd1 
Co-authored-by: Dalton Bohning <[email protected]>
Signed-off-by: enakta <[email protected]>
@daosbuild1
Copy link
Collaborator

Test stage Python Bandit check completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15691/6/execution/node/133/log

@0xE0F
Fixes for copyright linter
ee14978 
Features: DfuseFind

Signed-off-by: Denis Barakhtanov <[email protected]>
@daosbuild1
Copy link
Collaborator

Test stage Python Bandit check completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15691/7/execution/node/134/log

0xE0F added 4 commits January 8, 2025 13:43
@0xE0F
client: pass mode, oflags, class and chunk size to torch shim layer
15a142e 
This allows better flexibility.

Features: DfuseFind

Signed-off-by: Denis Barakhtanov <[email protected]>
@0xE0F
nlt: remove torch tests in favour of functional tests
0f44ee8 
Functional tests for pydaos.torch module is now available.

Features: DfuseFind

Signed-off-by: Denis Barakhtanov <[email protected]>
@0xE0F
torch: uniform way to return error
9c55f68 
No more negative error values.

Features: DfuseFind

Signed-off-by: Denis Barakhtanov <[email protected]>
@0xE0F
ftest: add iterable dataset to tests
6bebeb4 
Features: DfuseFind

Signed-off-by: Denis Barakhtanov <[email protected]>
@daosbuild1
Copy link
Collaborator

Test stage Python Bandit check completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15691/8/execution/node/134/log

@0xE0F
linter: oflags -> open_flags
7ececac 
Features: DfuseFind

Signed-off-by: Denis Barakhtanov <[email protected]>
@daosbuild1
Copy link
Collaborator

Test stage Python Bandit check completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15691/9/execution/node/134/log

@daosbuild1
Copy link
Collaborator

Test stage Functional on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15691/9/execution/node/1229/log

@0xE0F
torch: add parallel, chunked checkpoint writes
f161aa4 
Checkpoint writes can be now done in chunks in parallel.

Features: DfuseFind

Signed-off-by: Denis Barakhtanov <[email protected]>
@enakta enakta marked this pull request as ready for review January 9, 2025 06:06
@daosbuild1
Copy link
Collaborator

Test stage Python Bandit check completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15691/10/execution/node/133/log

@0xE0F
Linter fixes
8e2aa58 
Features: DfuseFind

Signed-off-by: Denis Barakhtanov <[email protected]>
@daosbuild1
Copy link
Collaborator

Test stage Python Bandit check completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15691/11/execution/node/134/log

@0xE0F
ftests: bump timeout for the checkpoint tests
f16b3a2 
Did not take into account that timeout is shared across all
tests in suit.

Features: DfuseFind
Signed-off-by: Denis Barakhtanov <[email protected]>
@daosbuild1
Copy link
Collaborator

Test stage Python Bandit check completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15691/12/execution/node/134/log

@enakta
Copy link
Author

enakta commented Jan 9, 2025

@mchaarawi I've added parameters passthrough for dfs_open.
@johannlombardi I've added parallel, chunked checkpoint writes.

Could you please have another look ?

@daosbuild1
Copy link
Collaborator

Test stage Functional on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15691/12/execution/node/1184/log

daltonbohning
daltonbohning reviewed Jan 9, 2025
View reviewed changes
Copy link
Contributor

@daltonbohning daltonbohning left a comment

Choose a reason for hiding this comment

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

Python bandit (security check) has some complaints. Since this is test code it's not a real concern but we should get the check clean

src/tests/ftest/pytorch/dataset.py Outdated Show resolved Hide resolved
src/tests/ftest/pytorch/dataset.py Outdated Show resolved Hide resolved
src/tests/ftest/pytorch/dataset.py Outdated Show resolved Hide resolved
src/tests/ftest/directory_tree.py Outdated Show resolved Hide resolved
src/tests/ftest/pytorch/checkpoint.py Outdated Show resolved Hide resolved
src/tests/ftest/pytorch/checkpoint.py Outdated Show resolved Hide resolved
enakta and others added 2 commits January 10, 2025 09:27
@enakta @daltonbohning
Bandit's checker suggestions
89bd050 
Co-authored-by: Dalton Bohning <[email protected]>
Signed-off-by: enakta <[email protected]>
@0xE0F
ftest: add torch to the requirements
5ea3a6a 
Features: DfuseFind,PytorchCheckpointTest,PytorchDatasetsTest
Signed-off-by: Denis Barakhtanov <[email protected]>
@daosbuild1
Copy link
Collaborator

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15691/14/testReport/

@daosbuild1
Copy link
Collaborator

Test stage Test RPMs on EL 8.6 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15691/14/execution/node/1140/log

jolivier23
jolivier23 reviewed Jan 11, 2025
View reviewed changes
src/client/pydaos/torch/torch_shim.c

out:
D_FREE(cp1);
D_FREE(cp2);
Copy link
Contributor

Choose a reason for hiding this comment

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

D_FREE(*dir) ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daltonbohning daltonbohning daltonbohning left review comments

@jolivier23 jolivier23 jolivier23 left review comments

@mchaarawi mchaarawi mchaarawi left review comments

@mjmac mjmac Awaiting requested review from mjmac

@johannlombardi johannlombardi Awaiting requested review from johannlombardi

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@enakta @daosbuild1 @mchaarawi @jolivier23 @daltonbohning @0xE0F