Skip to content

Preserve edge IDs for featureless graph sampling - #789

Merged
kmontemayor2-sc merged 12 commits into
mainfrom
kmontemayor/guard-missing-edge-ids
Oct 7, 2026
Merged

kmontemayor2-sc merged 12 commits into
mainfrom
kmontemayor/guard-missing-edge-ids

Conversation

@kmontemayor2-sc

@kmontemayor2-sc kmontemayor2-sc commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Restore edge-ID sampling for featureless graph partitions without requiring callers to switch to with_edge=False. Retain implicit edge IDs by default, with an explicit memory-saving opt-out for callers that do not need them.

Failure and fix

  1. Partitions without explicit IDs, weights, or edge features enter the compact graph construction path.
  2. That topology previously set _edge_ids = None. Existing callers requesting edge IDs could therefore reach the native sampler with a null pointer and crash. Absence of edge features did not make those sampling requests invalid.
  3. The builder now retains original COO-position IDs in CSR order. A native guard also rejects nonempty ID requests when IDs were explicitly omitted, while allowing empty results.
    For example:
    Input COO edges: (1,7), (0,5), (1,6), (0,4)
    Implicit edge IDs: 0, 1, 2, 3

Sampling nodes [0,1]:
Neighbors: [4,5,6,7]
Counts: [2,2]
Edge IDs: [3,1,2,0]
The featureless-partition regression test exercises this through actual dataset construction and native sampling.

API and memory

  • retain_edge_ids=True preserves compatibility and costs 8 bytes per edge. Callers using with_edge=False can explicitly disable retention.
  • Task-config construction reads this option from trainer or inferencer arguments.
  • Explicit IDs, features, and weights retain their existing construction path.
  • build_csr_from_coo returns CsrBuildResult with named indptr, indices, and optional edge_ids fields. Direct tuple-unpacking callers must migrate to named fields.
  • Construction writes into shared output tensors with bounded chunk temporaries.

Verification

63 focused CSR and topology tests passed, along with scoped lint, formatting, and type checks. The featureless edge-ID regression was also rerun independently and passed. Full-suite validation is deferred to CI.

@kmontemayor2-sc kmontemayor2-sc changed the title Guard CPU random edge sampling without edge IDs Preserve edge IDs for featureless graph sampling Oct 6, 2026
@kmontemayor2-sc

Copy link
Copy Markdown
Collaborator Author

/all_test

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

GiGL Automation

@ 17:02:10UTC : 🔄 E2E Test started.

@ 18:24:30UTC : ❌ Workflow failed.
Please check the logs for more details.

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

GiGL Automation

@ 17:12:40UTC : 🔄 Python Unit Test started.

@ 19:01:54UTC : ✅ Workflow completed successfully.

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

GiGL Automation

@ 17:14:23UTC : 🔄 Scala Unit Test started.

@ 17:24:48UTC : ✅ Workflow completed successfully.

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

GiGL Automation

@ 17:15:15UTC : 🔄 Lint Test started.

@ 17:24:13UTC : ✅ Workflow completed successfully.

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

GiGL Automation

@ 17:16:27UTC : 🔄 C++ Unit Test started.

@ 17:18:28UTC : ✅ Workflow completed successfully.

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

GiGL Automation

@ 17:21:21UTC : 🔄 Integration Test started.

@ 19:04:38UTC : ✅ Workflow completed successfully.

@mkolodner-sc mkolodner-sc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks Kyle! Approving to unblock proved my comments are addressed.

Comment thread gigl/distributed/dataset_factory.py
Comment thread gigl/utils/csr.py
Comment thread gigl/scripts/patches/0003-glt-reject-missing-cpu-random-edge-ids.patch Outdated
@dsaini2-sc

Copy link
Copy Markdown
Collaborator

Thanks Kyle, I have a question -- why can't the default be to retain_edge_ids=False? loaders only set with_edge when the graph has edge features (see base_dist_loader.py:464), and those graphs already take GLT's build with edge ids. Your 0003 guard now anyways turns any other caller's request into a clear error rather than a crash. With True by default, every featureless graph pays 8 B/edge: with int32 columns from #788 the stored graph goes from 4 to 12 B/edge, and the build peak from 1.5x to 2.5x. A caller that does need edge ids could pass True explicitly. wdyt?

@dsaini2-sc

Copy link
Copy Markdown
Collaborator

also, #788 was just merged, so we will need to rebase this

kmonte and others added 2 commits October 7, 2026 02:49
Merge the 0003 GLT patch into 0001, because both rewrite the same
SampleWithEdge region in random_sampler.cc. Add a note that featureless
graphs should set retain_edge_ids to False.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sing-edge-ids

# Conflicts:
#	gigl/utils/csr.py
#	tests/unit/utils/csr_test.py
@kmontemayor2-sc

Copy link
Copy Markdown
Collaborator Author

> why can't the default be to retain_edge_ids=False

Because there are some usages of with_edge being set to True explicitly, and I do not want to break those usages.

We will migrate those usages to use this opt-out, and then we can swag the flag after.

#788 should be merged in now :)

@dsaini2-sc

Copy link
Copy Markdown
Collaborator

Because there are some usages of with_edge being set to True explicitly
ahh, ok -- makes sense. Changes lgtm! thanks!

@kmontemayor2-sc
kmontemayor2-sc marked this pull request as ready for review October 7, 2026 17:06
@kmontemayor2-sc
kmontemayor2-sc added this pull request to the merge queue Oct 7, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 7, 2026
@kmontemayor2-sc
kmontemayor2-sc added this pull request to the merge queue Oct 7, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 7, 2026
@kmontemayor2-sc
kmontemayor2-sc added this pull request to the merge queue Oct 7, 2026
Merged via the queue into main with commit d4b11b3 Oct 7, 2026
7 checks passed
@kmontemayor2-sc
kmontemayor2-sc deleted the kmontemayor/guard-missing-edge-ids branch October 7, 2026 23:42
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.

5 participants