Skip to content

Doc and config changes from 2.7 branch#4023

Merged
chesterxgchen merged 9 commits intoNVIDIA:mainfrom
chesterxgchen:from_27_changes
Jan 23, 2026
Merged

Doc and config changes from 2.7 branch#4023
chesterxgchen merged 9 commits intoNVIDIA:mainfrom
chesterxgchen:from_27_changes

Conversation

@chesterxgchen
Copy link
Collaborator

Cherry picks

689315c - simplify the tests and doc string
2f4f2ae - fix a few mistakes
32d4880 - Expand recipe spec to add_server_config() and add_client_config() functions...
3abd08a - [2.7] Update product feature docs [skip ci] (#4018)

A few sentences describing the changes proposed in this pull request.

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.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 23, 2026

Greptile Overview

Greptile Summary

This PR cherry-picks changes from the 2.7 branch, primarily enhancing the Recipe API with configuration methods and script validation capabilities.

Key Changes:

  • Added add_server_config() and add_client_config() methods to Recipe class for top-level configuration management with proper type validation
  • Introduced _collect_non_local_scripts() utility function to identify absolute script paths that don't exist locally (for production environments)
  • Enhanced FedApp._add_resource() to accept absolute paths for pre-installed production scripts
  • Implemented environment-specific script validation: SimEnv/PocEnv raise errors for non-local scripts, while ProdEnv logs warnings
  • Added comprehensive test coverage for new functionality
  • Updated documentation including new release notes for v2.7.2

Review Findings:

  • The previous review thread correctly identified that the import was initially missing, but this has been resolved - _collect_non_local_scripts now exists in nvflare/recipe/utils.py:377
  • All code changes follow good practices with proper error handling and type validation
  • Tests are thorough and cover edge cases including the production scenario of pre-installed scripts

Confidence Score: 5/5

  • This PR is safe to merge with no significant risks
  • This is a clean cherry-pick from the 2.7 branch containing well-tested enhancements. The code includes proper type validation, comprehensive test coverage, and follows existing patterns. The previously reported import issue has been resolved. All changes are backward-compatible additions to the Recipe API
  • No files require special attention

Important Files Changed

Filename Overview
nvflare/recipe/spec.py Added add_server_config() and add_client_config() methods with proper type validation, plus enhanced docstring for process_env()
nvflare/recipe/utils.py Added _collect_non_local_scripts() utility function for script validation, fixed recipe name handling in error message
tests/unit_test/recipe/spec_test.py New comprehensive test file covering _collect_non_local_scripts(), environment validation, and new config methods
nvflare/job_config/api.py Enhanced _add_resource() to accept absolute paths that don't exist locally (for pre-installed production scripts)
nvflare/recipe/sim_env.py Added script validation in deploy() using _collect_non_local_scripts() - raises error for non-local scripts
nvflare/recipe/prod_env.py Added warning logs for non-local scripts in deploy() (assumes pre-installed on production systems)
nvflare/recipe/poc_env.py Added script validation in deploy() using _collect_non_local_scripts() - raises error for non-local scripts
tests/unit_test/job_config/fed_job_test.py Added tests for FedApp._add_resource() covering various scenarios including absolute paths that don't exist locally

Sequence Diagram

sequenceDiagram
    participant User
    participant Recipe
    participant FedJob
    participant ExecEnv
    participant Utils
    
    User->>Recipe: add_server_config(config)
    Recipe->>Recipe: validate isinstance(config, dict)
    alt Invalid type
        Recipe-->>User: raise TypeError
    end
    Recipe->>FedJob: to_server(config)
    
    User->>Recipe: add_client_config(config, clients)
    Recipe->>Recipe: validate isinstance(config, dict)
    alt Invalid type
        Recipe-->>User: raise TypeError
    end
    alt clients is None
        Recipe->>FedJob: to_clients(config)
    else specific clients
        loop for each client
            Recipe->>FedJob: to(config, client)
        end
    end
    
    User->>Recipe: execute(env)
    Recipe->>ExecEnv: deploy(job)
    ExecEnv->>Utils: _collect_non_local_scripts(job)
    Utils->>Utils: check job._deploy_map for absolute paths
    Utils-->>ExecEnv: return non_local_scripts[]
    
    alt SimEnv/PocEnv
        alt non_local_scripts not empty
            ExecEnv-->>User: raise ValueError("scripts must exist locally")
        end
    else ProdEnv
        loop for each non_local_script
            ExecEnv->>ExecEnv: logger.warning("assuming pre-installed")
        end
    end
    
    ExecEnv->>ExecEnv: proceed with deployment
    ExecEnv-->>User: return job_id
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.

1 file reviewed, 1 comment

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.

No files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@chesterxgchen
Copy link
Collaborator Author

/build

chesterxgchen and others added 8 commits January 23, 2026 09:27
…NVIDIA#4017)

1. release notes 
2. add available_recipes.rst
3. simplify user_guide -- move some of the user guides to
programming_guide.rst
 

A few sentences describing the changes proposed in this pull request.

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] 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.
1. add swarm learning configuration
2. add tensor_downloader.rst
3. add update reference for tensor_downloader.rst

Fixes # .

### Description

A few sentences describing the changes proposed in this pull request.

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] 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.
…functions

2. This allows one to specify the top level configs which previous unable to do
3. Update the swarm learning docs to leverage the new API
4. add tests
If file not exists with abs path, we will print warning for production,
raise error for sim and poc
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.

No files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@chesterxgchen
Copy link
Collaborator Author

/Build

@nvkevlu
Copy link
Collaborator

nvkevlu commented Jan 23, 2026

/build

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.

No files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link
Collaborator

@holgerroth holgerroth left a comment

Choose a reason for hiding this comment

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

LGTM

@chesterxgchen chesterxgchen merged commit fa37001 into NVIDIA:main Jan 23, 2026
20 checks passed
@chesterxgchen chesterxgchen deleted the from_27_changes branch January 23, 2026 18:55
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