Skip to content

Conversation

@ZiyueXu77
Copy link
Collaborator

Fixes # .

Description

GNN exmaple with job template convert to recipe using FedAvgRecipe

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Quick tests passed locally by running ./runtest.sh.
  • In-line docstrings updated.
  • Documentation updated.

Copilot AI review requested due to automatic review settings December 12, 2025 19:13
@ZiyueXu77 ZiyueXu77 changed the title Convert gnn example to recipe Convert job_template to recipe for GNN example Dec 12, 2025
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 12, 2025

Greptile Summary

Converted GNN example from job_template to recipe-based approach using FedAvgRecipe. Reorganized code structure by moving files from code/ to root and utils/ directories, renamed pyg_sage.py to model.py, and updated all imports accordingly. The new job.py provides two functions (create_protein_job and create_finance_job) that configure and return FedAvgRecipe instances for protein classification and financial transaction classification tasks.

Key improvements:

  • Simplified job creation using NVFlare's recipe API
  • Improved client ID derivation from site names (e.g., "site-1" -> 1)
  • Fixed import typo from old code (process_ellipitc -> process_elliptic)
  • Added file locking in protein client to prevent concurrent download issues
  • Updated documentation and notebook to reflect new recipe-based workflow
  • Reorganized directory structure for better clarity (code/ -> root and utils/)

The changes maintain backward compatibility in functionality while modernizing the implementation approach.

Confidence Score: 5/5

  • This PR is safe to merge with no issues found
  • Clean refactoring from job_template to recipe-based approach with proper code reorganization, improved error handling (file locking), and fixed import typo from the old code. All changes are well-structured and maintain functional equivalence.
  • No files require special attention

Important Files Changed

Filename Overview
examples/advanced/gnn/job.py New recipe-based job creation using FedAvgRecipe for both protein and finance tasks, well-structured and comprehensive
examples/advanced/gnn/client_finance.py Refactored FL client for finance task with improved site name handling and corrected import typo from old code
examples/advanced/gnn/client_protein.py Refactored FL client for protein task with improved site name handling and file locking for dataset downloads
examples/advanced/gnn/README.md Updated documentation to reflect recipe-based approach with clear usage instructions

Sequence Diagram

sequenceDiagram
    participant User
    participant job.py
    participant FedAvgRecipe
    participant SimEnv/ProdEnv
    participant Server
    participant Client1
    participant Client2

    User->>job.py: Run with --task_type (protein/finance)
    job.py->>FedAvgRecipe: create_protein_job() or create_finance_job()
    FedAvgRecipe->>FedAvgRecipe: Configure initial_model, train_script, train_args
    FedAvgRecipe->>FedAvgRecipe: Add IntimeModelSelector to server
    FedAvgRecipe-->>job.py: Return configured recipe
    job.py->>job.py: Export job to job_dir
    job.py->>SimEnv/ProdEnv: Create environment with client names
    job.py->>FedAvgRecipe: Execute recipe with environment
    
    loop For each round (num_rounds)
        Server->>Client1: Send global model (flare.receive)
        Server->>Client2: Send global model (flare.receive)
        Client1->>Client1: Load model, train for epochs_per_round
        Client2->>Client2: Load model, train for epochs_per_round
        Client1->>Client1: Evaluate on global model
        Client2->>Client2: Evaluate on global model
        Client1->>Server: Send updated weights + metrics (flare.send)
        Client2->>Server: Send updated weights + metrics (flare.send)
        Server->>Server: Aggregate using FedAvg
        Server->>Server: Model selection based on validation metric
    end
    
    FedAvgRecipe-->>job.py: Return run result
    job.py-->>User: Display job status and result
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

9 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR converts the GNN example from using traditional NVFlare job templates to a modern recipe-based approach using FedAvgRecipe. The conversion provides a cleaner, more programmatic API for creating federated GNN training jobs for two tasks: protein classification (PPI dataset) and financial transaction classification (Elliptic++ dataset).

Key Changes

  • Introduced recipe-based job creation using FedAvgRecipe with task-specific functions (create_protein_job() and create_finance_job())
  • Added new utility files for data processing and local training baselines
  • Updated documentation and notebook to reflect the new recipe-based workflow

Reviewed changes

Copilot reviewed 5 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
utils/process_elliptic.py New data preprocessing utility for Elliptic++ dataset
utils/graphsage_protein_local.py New local training script for protein classification baseline
utils/graphsage_finance_local.py Updated imports to use new model location
model.py New SAGE model definition for finance task
job.py New recipe-based job creation with task-specific functions
client_protein.py New FL client implementation for protein classification
client_finance.py Updated imports to use new utilities location
README.md Updated documentation to explain recipe-based approach
gnn_examples.ipynb Updated notebook with recipe-based workflow instructions
Comments suppressed due to low confidence (2)

examples/advanced/gnn/utils/graphsage_finance_local.py:24

  • The import uses a typo: "process_ellipitc" should be "process_elliptic" (missing 'p' in "elliptic"). This needs to match the corrected function name in process_elliptic.py.
    examples/advanced/gnn/client_finance.py:27
  • The import uses a typo: "process_ellipitc" should be "process_elliptic" (missing 'p' in "elliptic"). This needs to match the corrected function name in process_elliptic.py.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

9 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@chesterxgchen
Copy link
Collaborator

@YuanTingHsieh Ziyue is not coming back to fix until next year. You can take over to fix them if you are free

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (2)

  1. examples/advanced/gnn/client_finance.py, line 96-103 (link)

    logic: if on line 98 should be elif to avoid overwriting train_data_sub when client_id == 1

    Currently when client_id == 1, line 97 sets train_data_sub, but then line 98's if condition also evaluates to true and overwrites it again.

  2. examples/advanced/gnn/utils/graphsage_finance_local.py, line 97-104 (link)

    logic: if on line 99 should be elif to avoid overwriting train_data_sub when client_id == 1

9 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (2)

  1. examples/advanced/gnn/client_finance.py, line 96-103 (link)

    style: Missing elif causes client_id == 0 branch to fall through. When client_id is 0, the code sets train_data_sub = train_data on line 97, but then line 98's if args.client_id == 1 is evaluated (not elif), and finally line 101's elif is checked. This works but is inconsistent.

  2. examples/advanced/gnn/utils/graphsage_finance_local.py, line 97-104 (link)

    style: Missing elif causes client_id == 0 branch to fall through. When client_id is 0, the code sets train_data_sub = train_data on line 98, but then line 99's if args.client_id == 1 is evaluated (not elif), and finally line 102's elif is checked. This works but is inconsistent.

9 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 2, 2026

Greptile's behavior is changing!

From now on, if a review finishes with no comments, we will not post an additional "statistics" comment to confirm that our review found nothing to comment on. However, you can confirm that we reviewed your changes in the status check section.

This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR".

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (1)

  1. examples/advanced/gnn/client_finance.py, line 136-151 (link)

    logic: evaluate function is defined inside the epoch loop (line 123), causing it to be redefined every epoch. Move the function definition outside the loop

9 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@ZiyueXu77
Copy link
Collaborator Author

/build

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.

3 participants