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

[GraphBolt] Add read_async to gb.Feature. [1] #7546

Merged
merged 3 commits into from
Jul 20, 2024

Conversation

mfbalin
Copy link
Collaborator

@mfbalin mfbalin commented Jul 20, 2024

Description

#7540 first part.

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

@mfbalin mfbalin added the expedited if it doesn't affect the main path approve first to unblock related projects, and review later label Jul 20, 2024
@mfbalin mfbalin requested a review from frozenbugs July 20, 2024 15:57
@mfbalin mfbalin changed the title [GraphBolt] Add read_async to gb.Feature. [GraphBolt] Add read_async to gb.Feature. [1] Jul 20, 2024
@dgl-bot
Copy link
Collaborator

dgl-bot commented Jul 20, 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 20, 2024

Commit ID: b1979ad6d846a3ec0fc7e4753de1183901159c09

Build ID: 1

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

Report path: link

Full logs path: link

@dgl-bot
Copy link
Collaborator

dgl-bot commented Jul 20, 2024

Commit ID: d84784c891e4f03227621defaf5279d6d636ce87

Build ID: 2

Status: ❌ CI test failed in Stage [Torch CPU (Win64) Unit test].

Report path: link

Full logs path: link

@mfbalin
Copy link
Collaborator Author

mfbalin commented Jul 20, 2024

https://dgl-ci-result.s3.us-west-2.amazonaws.com/dgl/PR-7546/2/2/logs/report.html

=========================== short test summary info ===========================
FAILED tests/python/pytorch/graphbolt/impl/test_fused_csc_sampling_graph.py::test_hetero_graph_on_shared_memory[False-False-1000-1000-10000-100000] - RuntimeError: Memory mapping failed, Win32 error: 5
=== 1 failed, 5829 passed, 861 skipped, 5173 warnings in 724.99s (0:12:04) ====

@dgl-bot
Copy link
Collaborator

dgl-bot commented Jul 20, 2024

Commit ID: 507bbd4

Build ID: 3

Status: ✅ CI test succeeded.

Report path: link

Full logs path: link

@mfbalin mfbalin merged commit 8d770b6 into dmlc:master Jul 20, 2024
2 checks passed
@mfbalin mfbalin deleted the gb_read_async_API branch July 20, 2024 17:06
"""
raise NotImplementedError

def read_async_num_stages(self, ids_device: torch.device):
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you already returned an async_handle, why do you need to define read_async_num_stages?
Consider merging?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because read_async returns a generator object. There is no way to also return how many stages are in it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can always override the generator object to provide an extra field?

Copy link
Collaborator

Choose a reason for hiding this comment

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

e.g. implement the len function of the object?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How can I do that?

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
expedited if it doesn't affect the main path approve first to unblock related projects, and review later
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants