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

Use torch core instead of torchdata modules. #7609

Merged
merged 4 commits into from
Aug 1, 2024

Conversation

frozenbugs
Copy link
Collaborator

Description

Checklist

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [$CATEGORY] (such as [NN], [Model], [Doc], [Feature]])
  • I've leverage the tools to beautify the python and c++ code.
  • The PR is complete and small, read the Google eng practice (CL equals to PR) to understand more about small PR. In DGL, we consider PRs with less than 200 lines of core code change are small (example, test and documentation could be exempted).
  • All changes have test coverage
  • Code is well-documented
  • To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change
  • Related issue is referred in this PR
  • If the PR is for a new model/paper, I've updated the example index here.

Changes

@dgl-bot
Copy link
Collaborator

dgl-bot commented Jul 29, 2024

To trigger regression tests:

  • @dgl-bot run [instance-type] [which tests] [compare-with-branch];
    For example: @dgl-bot run g4dn.4xlarge all dmlc/master or @dgl-bot run c5.9xlarge kernel,api dmlc/master

@dgl-bot
Copy link
Collaborator

dgl-bot commented Jul 29, 2024

Commit ID: 4dc7e6effe90c07cfaf6e300c38ea0d6ab86617b

Build ID: 1

Status: ❌ CI test failed in Stage [Lint Check].

Report path: link

Full logs path: link

@frozenbugs frozenbugs changed the title blabal Use torch core instead of torchdata modules. Jul 29, 2024
@dgl-bot
Copy link
Collaborator

dgl-bot commented Jul 29, 2024

Commit ID: 9b7c85660f210205e02aded19df3be1b7b15ca06

Build ID: 2

Status: ⚪️ CI test cancelled due to overrun.

Report path: link

Full logs path: link

@frozenbugs frozenbugs requested review from mfbalin and Rhett-Ying and removed request for mfbalin July 29, 2024 09:02
@dgl-bot
Copy link
Collaborator

dgl-bot commented Jul 29, 2024

Commit ID: dd3496261f58dc5db9160f9f678eb180cee988f4

Build ID: 3

Status: ✅ CI test succeeded.

Report path: link

Full logs path: link

@mfbalin
Copy link
Collaborator

mfbalin commented Jul 29, 2024

@frozenbugs wouldn't they remove these too? Since we want to be able to use later torch versions, if they remove these from core repo as well, we will have to go back to torchdata <0.8.0. I feel depending on torchdata might be a better choice. We can verify whether they plan to remove the datapipes from the core as that is my current understanding.

@mfbalin
Copy link
Collaborator

mfbalin commented Jul 30, 2024

@frozenbugs Andrew Kho, the lead torchdata developer said that the datapipes in torch.utils.data are most likely to be deprecated and removed as well. So #7604 seems to be the way to go for now.

@frozenbugs
Copy link
Collaborator Author

Currently we are rely on functional_datapipe from torch.data package as well, it could break us if they remove datapipe from core entirely. Also the torchdata module seems rely on torch.data for many of the modules, if they remove the code from torch core, how would the dependency look like?

@mfbalin
Copy link
Collaborator

mfbalin commented Jul 30, 2024

Currently we are rely on functional_datapipe from torch.data package as well, it could break us if they remove datapipe from core entirely. Also the torchdata module seems rely on torch.data for many of the modules, if they remove the code from torch core, how would the dependency look like?

I asked that now. I asked if we pin torchdata<0.8.0, whether we can update to later torch versions. If we are lucky, maybe they will make a new 0.7.2 release that is compatible with future torch releases, maybe not.

Worst case, we implement our own datapipes or consider if the new thing they have can replace datapipes for our use case.

@frozenbugs
Copy link
Collaborator Author

For reference: pytorch/data#1196

@mfbalin
Copy link
Collaborator

mfbalin commented Jul 30, 2024

They said this:

0.8.0 will still have datapipes, but generally we won’t be able to continuously support all cross versioning so at some point it will probably break, and you’ll probably need to set up your package manager appropriately to try it out

@mfbalin
Copy link
Collaborator

mfbalin commented Jul 31, 2024

@frozenbugs

image

@frozenbugs
Copy link
Collaborator Author

From the announcement, it seems that they haven't made the decision on remove datapipe from torch core. That says we can merge this PR first and get rid of import torchdata.dataloader2.graph as dp_utils. @mfbalin

@dgl-bot
Copy link
Collaborator

dgl-bot commented Aug 1, 2024

Commit ID: 6d2501f

Build ID: 4

Status: ✅ CI test succeeded.

Report path: link

Full logs path: link

@mfbalin mfbalin merged commit ea3716d into dmlc:master Aug 1, 2024
2 checks passed
@frozenbugs frozenbugs deleted the datapipe branch August 2, 2024 01:40
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

Successfully merging this pull request may close these issues.

3 participants