Skip to content

Remove TODO from MovieLens Example#422

Open
alexbarghi-nv wants to merge 4 commits intorapidsai:mainfrom
alexbarghi-nv:remove-todo
Open

Remove TODO from MovieLens Example#422
alexbarghi-nv wants to merge 4 commits intorapidsai:mainfrom
alexbarghi-nv:remove-todo

Conversation

@alexbarghi-nv
Copy link
Member

Removes the TODO for adding temporal sampling from the MovieLens example, since it has already been added.

@alexbarghi-nv alexbarghi-nv requested a review from a team as a code owner March 6, 2026 00:24
@alexbarghi-nv alexbarghi-nv self-assigned this Mar 6, 2026
@alexbarghi-nv alexbarghi-nv added bug Something isn't working non-breaking Introduces a non-breaking change labels Mar 6, 2026
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 6, 2026

Greptile Summary

This PR removes a stale TODO comment about temporal sampling and replaces it with a clarifying comment explaining why test_loader intentionally omits time_attr/edge_label_time. The temporal sampling feature is already in use on train_loader (lines 459–460), so the TODO was outdated.

  • Removes # TODO enable temporal sampling when it is available in cuGraph-PyG — the feature is already implemented
  • Adds a comment clarifying that test_loader will emit a warning per epoch because it does not use time attributes; this is expected since lazy graph creation causes both loaders to share the same graph

Confidence Score: 5/5

  • Safe to merge — comment-only change with no logic modifications.
  • No code logic is altered; only a stale TODO is removed and an explanatory comment is added. The existing temporal sampling implementation in train_loader is unchanged.
  • No files require special attention.

Important Files Changed

Filename Overview
python/cugraph-pyg/cugraph_pyg/examples/movielens_mnmg.py Removes a stale TODO (temporal sampling is already implemented in train_loader via edge_label_time/time_attr) and adds an explanatory comment about the expected warning on test_loader which intentionally omits time attributes.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Data Preparation] --> B[Build feature_store / graph_store]
    B --> C[train_loader\nedge_label_time = time_train - 1\ntime_attr = 'time'\nTemporal sampling enabled]
    B --> D[test_loader\nNo time_attr / edge_label_time\nShares same lazy graph → warning expected]
    C --> E[Training Loop]
    D --> F[Evaluation Loop]
Loading

Last reviewed commit: d4974f0

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

Labels

bug Something isn't working non-breaking Introduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant