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 28 commits into
base: master
Choose a base branch
from
Open

DAOS-16362: pydaos.torch checkpointing #15691

wants to merge 28 commits into from

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 2 times, most recently 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.

@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

@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

@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

@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 and others added 10 commits January 14, 2025 10:17
@0xE0F
nlt: remove torch tests in favour of functional tests
65e7c9c 
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
eb0059d 
No more negative error values.

Features: DfuseFind

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

Signed-off-by: Denis Barakhtanov <[email protected]>
@0xE0F
linter: oflags -> open_flags
b6512c9 
Features: DfuseFind

Signed-off-by: Denis Barakhtanov <[email protected]>
@0xE0F
torch: add parallel, chunked checkpoint writes
770f45d 
Checkpoint writes can be now done in chunks in parallel.

Features: DfuseFind

Signed-off-by: Denis Barakhtanov <[email protected]>
@0xE0F
Linter fixes
2f3488f 
Features: DfuseFind

Signed-off-by: Denis Barakhtanov <[email protected]>
@0xE0F
ftests: bump timeout for the checkpoint tests
20d417a 
Did not take into account that timeout is shared across all
tests in suit.

Features: DfuseFind
Signed-off-by: Denis Barakhtanov <[email protected]>
@enakta @daltonbohning @0xE0F
Bandit's checker suggestions
9354f20 
Co-authored-by: Dalton Bohning <[email protected]>
Signed-off-by: enakta <[email protected]>
@0xE0F
ftest: add torch to the requirements
21d8222 
Features: DfuseFind,PytorchCheckpointTest,PytorchDatasetsTest
Signed-off-by: Denis Barakhtanov <[email protected]>
@0xE0F
ftests: drop version requirement for pytorch
82c629f 
EL8 has a bit old pytorch version

Features: DfuseFind

Signed-off-by: Denis Barakhtanov <[email protected]>
@0xE0F 0xE0F force-pushed the 0xe0f/pydaos.torch.checkpointing branch from 5ea3a6a to 82c629f Compare January 13, 2025 23:19
@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/16/execution/node/1220/log

johannlombardi
johannlombardi reviewed Jan 15, 2025
View reviewed changes
src/client/pydaos/torch/__init__.py
# (C) Copyright 2024 Google LLC
# (C) Copyright 2024 Enakta Labs Ltd
# (C) Copyright 2024-2025 Intel Corporation.
# (C) Copyright 2025 Hewlett Packard Enterprise Development LP
Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably not touch those.

Copy link
Author

Choose a reason for hiding this comment

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

Linting was failing without these 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

utils/cq/check_update_copyright.sh

Just add handling for Enakta there and you should be fine

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 still land it with copyright issues though if you just want to change them

@0xE0F
ftest: bump pool size for tests
57301cd 
Features: DfuseFind

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

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

@enakta enakta marked this pull request as draft January 16, 2025 05:35
@enakta
Copy link
Author

enakta commented Jan 16, 2025

Looks like lazy python shim module initialisation does not always work correctly, most likely the same issue as #15277.

0xE0F added 2 commits January 20, 2025 10:49
@0xE0F
ftest: be modest with vm test machines
4e189fc 
Features: DfuseFind

Signed-off-by: Denis Barakhtanov <[email protected]>
@0xE0F
client: torch: use old multiprocess API to support python 3.6
d1e2f51 
EL8.8 has python 3.6 that does not have `initializer` argument in
ProcessPoolExecutor which makes it impossible to use due to forking and
needs of `daos_reinit` call.

This commit replace ProcessPoolExecutor API to its underlying multiprocess API.

Features: DfuseFind

Signed-off-by: Denis Barakhtanov <[email protected]>
@enakta enakta marked this pull request as ready for review January 20, 2025 00:17
@0xE0F 0xE0F force-pushed the 0xe0f/pydaos.torch.checkpointing branch from 3c0067d to 5843c45 Compare January 20, 2025 00:52
@0xE0F
torch: linter fix
f19fe56 
Features: DfuseFind

Signed-off-by: Denis Barakhtanov <[email protected]>
@0xE0F 0xE0F force-pushed the 0xe0f/pydaos.torch.checkpointing branch from 5843c45 to f19fe56 Compare January 20, 2025 01:21
@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/21/execution/node/1176/log

@0xE0F
ftest: bump timout for checkpoint tests
b17a6f7 
Features: DfuseFind

Signed-off-by: Denis Barakhtanov <[email protected]>
@enakta enakta requested a review from johannlombardi January 21, 2025 10:29
@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15691/22/execution/node/1565/log

@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/23/testReport/

@0xE0F
ftest: restarting tests with latest master
d1d5580 
Features: DfuseFind
Allow-unstable-test: true

Signed-off-by: Denis Barakhtanov <[email protected]>
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

@mjmac mjmac Awaiting requested review from mjmac

@mchaarawi mchaarawi Awaiting requested review from mchaarawi

@jolivier23 jolivier23 Awaiting requested review from jolivier23

@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.
7 participants
@enakta @daosbuild1 @johannlombardi @mchaarawi @jolivier23 @daltonbohning @0xE0F