[DistDGL][Robustness]Use appropriate delimiter when reading edge files. #5447
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
|
To trigger regression tests:
|
Collaborator
frozenbugs
reviewed
Mar 14, 2023
Collaborator
Collaborator
thvasilo
reviewed
Mar 16, 2023
1. reverting back to the original pytest_utils.py. This will remove the random delimiter used when creating edge files.
Collaborator
Collaborator
Collaborator
Collaborator
Collaborator
thvasilo
reviewed
Apr 27, 2023
Contributor
thvasilo
left a comment
There was a problem hiding this comment.
I've mentioned this before: The amount of setup required for the tests is very large, and they seem to be duplicating the implementation. As a result the tests are brittle and refactoring the code will definitely lead to us having to change the test as well.
I'm also concerned that the values as noted in the comments do not seem to follow an expected pattern yet the tests pass. Are we sure we are testing the right things here?
Please take another look at the comments, fix what is needed and we can take a second look.
Let's ask the question: what's the minimum amount of setup possible to test the functionality that this code change adds.
| assert np.all(exp_etype_ids == edge_dict[constants.ETYPE_ID]) | ||
|
|
||
| # validate edge_tids here. | ||
| assert edge_tids["n1:e1:n1"][0] == ( |
Contributor
There was a problem hiding this comment.
For constants like "n1:e1:n1" better to define them as a file constant EDGE_TYPE = "n1:e1:n1" and use the variable throughout the code.
|
|
||
|
|
||
| def _validate_edges( | ||
| rank, world_size, num_chunks, edge_dict, edge_tids, schema_map |
Contributor
There was a problem hiding this comment.
edge_dict is not in the docstring
| edge_feats.append(data) | ||
|
|
||
| if len(edge_feats) == 0: | ||
| actual_results = edge_features["n1:e1:n1/edge_feat_1/0"] |
Contributor
There was a problem hiding this comment.
Same here define constants and use something like f"{EDGE_TYPE}/{FEATURE_NAME}/0"
| else: | ||
| edge_feats = np.concatenate(edge_feats) | ||
|
|
||
| # assert |
| edge_feats = np.concatenate(edge_feats) | ||
|
|
||
| # assert | ||
| assert np.all( |
Contributor
There was a problem hiding this comment.
Use numpy.testing.assert_array_equal
Comment on lines
+175
to
+182
| schema["edge_type"] = ["n1:e1:n1"] | ||
| schema["node_type"] = ["n1"] | ||
|
|
||
| edges = {} | ||
| edges["n1:e1:n1"] = {} | ||
| edges["n1:e1:n1"]["format"] = {} | ||
| edges["n1:e1:n1"]["format"]["name"] = edge_fmt | ||
| edges["n1:e1:n1"]["format"]["delimiter"] = edge_fmt_del |
|
|
||
| # Make sure that the spawned process, mimicing ranks/workers, did | ||
| # not generate any errors or assertion failures | ||
| assert len(sh_dict) == 0, f"Spawned processes reported some errors !!!" |
Contributor
There was a problem hiding this comment.
How does the user know which errors were encountered?
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "world_size, num_chunks, num_parts", [[1, 1, 4], [4, 1, 4]] |
Contributor
There was a problem hiding this comment.
Why do we keep num_chunks to 1? Can we add one case for [4,4,4]?
Comment on lines
+88
to
+90
| data = np.arange(10, dtype=np.int32) | ||
| for _ in range(9): | ||
| data = np.vstack((data, np.arange(10, dtype=np.int32) + 10 * idx)) |
Contributor
There was a problem hiding this comment.
Is this correct?
idx = 0
In [40]: data
Out[40]:
array([[0, 1, 2, 3, 4, 5, 6, 7, 8, 9],
[0, 1, 2, 3, 4, 5, 6, 7, 8, 9],
[0, 1, 2, 3, 4, 5, 6, 7, 8, 9],
[0, 1, 2, 3, 4, 5, 6, 7, 8, 9],
[0, 1, 2, 3, 4, 5, 6, 7, 8, 9],
[0, 1, 2, 3, 4, 5, 6, 7, 8, 9],
[0, 1, 2, 3, 4, 5, 6, 7, 8, 9],
[0, 1, 2, 3, 4, 5, 6, 7, 8, 9],
[0, 1, 2, 3, 4, 5, 6, 7, 8, 9],
[0, 1, 2, 3, 4, 5, 6, 7, 8, 9]], dtype=int32)
idx = 1
In [43]: data
Out[43]:
array([[ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9],
[10, 11, 12, 13, 14, 15, 16, 17, 18, 19],
[10, 11, 12, 13, 14, 15, 16, 17, 18, 19],
[10, 11, 12, 13, 14, 15, 16, 17, 18, 19],
[10, 11, 12, 13, 14, 15, 16, 17, 18, 19],
[10, 11, 12, 13, 14, 15, 16, 17, 18, 19],
[10, 11, 12, 13, 14, 15, 16, 17, 18, 19],
[10, 11, 12, 13, 14, 15, 16, 17, 18, 19],
[10, 11, 12, 13, 14, 15, 16, 17, 18, 19],
[10, 11, 12, 13, 14, 15, 16, 17, 18, 19]], dtype=int32)
In [44]: idx = 2
In [46]: data
Out[46]:
array([[ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9],
[20, 21, 22, 23, 24, 25, 26, 27, 28, 29],
[20, 21, 22, 23, 24, 25, 26, 27, 28, 29],
[20, 21, 22, 23, 24, 25, 26, 27, 28, 29],
[20, 21, 22, 23, 24, 25, 26, 27, 28, 29],
[20, 21, 22, 23, 24, 25, 26, 27, 28, 29],
[20, 21, 22, 23, 24, 25, 26, 27, 28, 29],
[20, 21, 22, 23, 24, 25, 26, 27, 28, 29],
[20, 21, 22, 23, 24, 25, 26, 27, 28, 29],
[20, 21, 22, 23, 24, 25, 26, 27, 28, 29]], dtype=int32)
Comment on lines
+129
to
+134
| data = np.arange(100, 110, dtype=np.int64) | ||
| for _ in range(9): | ||
| data = np.vstack( | ||
| (data, np.arange(100, 110, dtype=np.int64) + 100 * idx) | ||
| ) | ||
| node_feats.append(data) |
Contributor
There was a problem hiding this comment.
Is this correct?
idx = 0
In [48]: data = np.arange(100, 110, dtype=np.int64)
...: for _ in range(9):
...: data = np.vstack(
...: (data, np.arange(100, 110, dtype=np.int64) + 100 * idx)
...: )
...: data
Out[48]:
array([[100, 101, 102, 103, 104, 105, 106, 107, 108, 109],
[100, 101, 102, 103, 104, 105, 106, 107, 108, 109],
[100, 101, 102, 103, 104, 105, 106, 107, 108, 109],
[100, 101, 102, 103, 104, 105, 106, 107, 108, 109],
[100, 101, 102, 103, 104, 105, 106, 107, 108, 109],
[100, 101, 102, 103, 104, 105, 106, 107, 108, 109],
[100, 101, 102, 103, 104, 105, 106, 107, 108, 109],
[100, 101, 102, 103, 104, 105, 106, 107, 108, 109],
[100, 101, 102, 103, 104, 105, 106, 107, 108, 109],
[100, 101, 102, 103, 104, 105, 106, 107, 108, 109]])
In [49]: idx = 1
In [50]: data = np.arange(100, 110, dtype=np.int64)
...: for _ in range(9):
...: data = np.vstack(
...: (data, np.arange(100, 110, dtype=np.int64) + 100 * idx)
...: )
...: data
Out[50]:
array([[100, 101, 102, 103, 104, 105, 106, 107, 108, 109],
[200, 201, 202, 203, 204, 205, 206, 207, 208, 209],
[200, 201, 202, 203, 204, 205, 206, 207, 208, 209],
[200, 201, 202, 203, 204, 205, 206, 207, 208, 209],
[200, 201, 202, 203, 204, 205, 206, 207, 208, 209],
[200, 201, 202, 203, 204, 205, 206, 207, 208, 209],
[200, 201, 202, 203, 204, 205, 206, 207, 208, 209],
[200, 201, 202, 203, 204, 205, 206, 207, 208, 209],
[200, 201, 202, 203, 204, 205, 206, 207, 208, 209],
[200, 201, 202, 203, 204, 205, 206, 207, 208, 209]])
In [51]: idx = 2
In [52]: data = np.arange(100, 110, dtype=np.int64)
...: for _ in range(9):
...: data = np.vstack(
...: (data, np.arange(100, 110, dtype=np.int64) + 100 * idx)
...: )
...: data
Out[52]:
array([[100, 101, 102, 103, 104, 105, 106, 107, 108, 109],
[300, 301, 302, 303, 304, 305, 306, 307, 308, 309],
[300, 301, 302, 303, 304, 305, 306, 307, 308, 309],
[300, 301, 302, 303, 304, 305, 306, 307, 308, 309],
[300, 301, 302, 303, 304, 305, 306, 307, 308, 309],
[300, 301, 302, 303, 304, 305, 306, 307, 308, 309],
[300, 301, 302, 303, 304, 305, 306, 307, 308, 309],
[300, 301, 302, 303, 304, 305, 306, 307, 308, 309],
[300, 301, 302, 303, 304, 305, 306, 307, 308, 309],
[300, 301, 302, 303, 304, 305, 306, 307, 308, 309]])
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Webgraph dataset edge files have '\t' as the delimiter. But the pipeline, by default assumes ' ' as the delimiter because of which it fails to read webgraph files.
Checklist
Please feel free to remove inapplicable items for your PR.
Changes