Skip to content

[FEA] Support Length in Loaders#485

Open
alexbarghi-nv wants to merge 3 commits into
rapidsai:mainfrom
alexbarghi-nv:support-len
Open

[FEA] Support Length in Loaders#485
alexbarghi-nv wants to merge 3 commits into
rapidsai:mainfrom
alexbarghi-nv:support-len

Conversation

@alexbarghi-nv

@alexbarghi-nv alexbarghi-nv commented Jul 3, 2026

Copy link
Copy Markdown
Member

Supports length in loaders, when the user passes a seed list. Closes #483

@alexbarghi-nv alexbarghi-nv requested a review from a team as a code owner July 3, 2026 01:25
@alexbarghi-nv alexbarghi-nv self-assigned this Jul 3, 2026
@alexbarghi-nv alexbarghi-nv added feature request New feature or request non-breaking Introduces a non-breaking change labels Jul 3, 2026
@greptile-apps

greptile-apps Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Adds __len__ support to NodeLoader and LinkLoader when the loader is constructed with an explicit seed list. Each class saves the original argument before processing, exposes a helper property to determine if explicit seeds were provided, and implements __len__ using ceiling/floor division based on drop_last.

  • NodeLoader._has_explicit_input_nodes correctly handles all InputNodes variants including the plain-string edge-type case.
  • LinkLoader._has_explicit_edge_label_index is missing a str guard: passing a plain string edge type (e.g. \"knows\") returns True instead of False, so len(loader) silently returns a resolved count rather than raising ValueError.
  • Tests cover tensor, drop_last, and 3-tuple string paths, but not the plain-string case for the link loader.

Confidence Score: 4/5

The node loader changes are clean. The link loader has a defect where passing a string edge type to edge_label_index causes len() to return silently instead of raising.

The link loader's _has_explicit_edge_label_index property doesn't guard against a plain str value (a valid InputEdges type in PyG). When a user passes a string edge type, the method returns True and __len__ computes a count from resolved edges instead of raising the documented ValueError. The node loader handles this correctly.

python/cugraph-pyg/cugraph_pyg/loader/link_loader.py — the _has_explicit_edge_label_index property needs a string check.

Important Files Changed

Filename Overview
python/cugraph-pyg/cugraph_pyg/loader/link_loader.py Adds __len__ and _has_explicit_edge_label_index; missing str guard in the property causes len() to silently return a value instead of raising when a string edge type is passed.
python/cugraph-pyg/cugraph_pyg/loader/node_loader.py Adds __len__ and _has_explicit_input_nodes; correctly handles all InputNodes cases including the string type check.
python/cugraph-pyg/cugraph_pyg/tests/loader/test_neighbor_loader.py New tests cover tensor, drop_last, and edge-type-only paths; lacks a test for edge_label_index as a plain string in the link loader, which is the uncovered buggy case.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["len(loader) called"] --> B{_has_explicit_*?}
    B -- "False" --> C["raise ValueError"]
    B -- "True" --> D["num_seeds = input_data.node/row.numel()"]
    D --> E{drop_last?}
    E -- "Yes" --> F["return num_seeds // batch_size"]
    E -- "No" --> G["return ceil(num_seeds / batch_size)"]

    subgraph NodeLoader
        N1{input_nodes type}
        N1 -- "None or str" --> N2["False"]
        N1 -- "(str, None)" --> N3["False"]
        N1 -- "(str, tensor)" --> N4["True"]
        N1 -- "tensor" --> N5["True"]
    end

    subgraph LinkLoader
        L1{edge_label_index type}
        L1 -- "None" --> L2["False"]
        L1 -- "str BUG" --> L3["True should be False"]
        L1 -- "Tensor" --> L4["True"]
        L1 -- "3-tuple of str" --> L5["False"]
        L1 -- "(type, None)" --> L6["False"]
        L1 -- "(type, tensor)" --> L7["True"]
    end
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["len(loader) called"] --> B{_has_explicit_*?}
    B -- "False" --> C["raise ValueError"]
    B -- "True" --> D["num_seeds = input_data.node/row.numel()"]
    D --> E{drop_last?}
    E -- "Yes" --> F["return num_seeds // batch_size"]
    E -- "No" --> G["return ceil(num_seeds / batch_size)"]

    subgraph NodeLoader
        N1{input_nodes type}
        N1 -- "None or str" --> N2["False"]
        N1 -- "(str, None)" --> N3["False"]
        N1 -- "(str, tensor)" --> N4["True"]
        N1 -- "tensor" --> N5["True"]
    end

    subgraph LinkLoader
        L1{edge_label_index type}
        L1 -- "None" --> L2["False"]
        L1 -- "str BUG" --> L3["True should be False"]
        L1 -- "Tensor" --> L4["True"]
        L1 -- "3-tuple of str" --> L5["False"]
        L1 -- "(type, None)" --> L6["False"]
        L1 -- "(type, tensor)" --> L7["True"]
    end
Loading

Reviews (2): Last reviewed commit: "Merge branch 'main' into support-len" | Re-trigger Greptile

Comment thread python/cugraph-pyg/cugraph_pyg/tests/loader/test_neighbor_loader.py Outdated
)

neg_sampling = torch_geometric.sampler.NegativeSampling.cast(neg_sampling)
self.__edge_label_index = edge_label_index

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This line seems to keep the edge_label_index alive for the loader's lifetime. Would that be expensive considering the loader also creates another copy with clone()? Could we compute if it has explicit labels within init, and only store a boolean?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature request New feature or request non-breaking Introduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEA] Support __len__ in Loaders

2 participants