Fix preflight check and ci

[YuanTingHsieh]

Preflight check tool was hardcoded to test against GRPC communication scheme.
We have added more schemes and now our default is HTTP so we should change accordingly

Description

• Update preflight check
• Remove overseer test
• Update preflight check tests

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]

Greptile Summary

• Updates preflight check tool to support multiple communication schemes (HTTP, HTTPS, TCP, STCP, GRPC) instead of being hardcoded to GRPC, aligning with the new HTTP default
• Removes deprecated overseer test functionality from integration tests and project configurations as part of modernizing the communication architecture
• Refactors preflight check tests to use structured table parsing instead of hardcoded string matching for improved maintainability

Important Files Changed

Filename	Overview
nvflare/tool/package_checker/check_rule.py	Refactored CheckGRPCServerAvailable to CheckServerAvailable with auto-detection of communication schemes from config
nvflare/tool/package_checker/utils.py	Added check_socket_server_running() and get_communication_scheme() functions for multi-scheme connectivity testing
tests/integration_test/preflight_check_test.py	Replaced brittle string matching with structured table parsing and updated expected check names for new scheme-agnostic approach

Confidence score: 4/5

• This PR is relatively safe to merge with the main risk being potential integration issues with the new communication scheme detection
• Score reflects solid implementation but complexity of changes across multiple components dealing with network communication schemes and some inconsistencies in supported scheme lists between different modules
• Pay close attention to the preflight check components and ensure thorough testing of different communication schemes in production-like environments

Sequence Diagram

sequenceDiagram
    participant User as "User"
    participant PreflightTool as "Preflight Check Tool"
    participant PackageChecker as "Package Checker"
    participant CheckRule as "Check Rule"
    participant ConfigReader as "Config Reader"
    participant NetworkChecker as "Network Checker"
    participant ProcessRunner as "Process Runner"

    User->>PreflightTool: "Run preflight check -p package_path"
    PreflightTool->>PackageChecker: "Create appropriate checker (Server/Client/Admin)"
    PackageChecker->>PackageChecker: "should_be_checked()"
    PackageChecker->>PackageChecker: "init_rules()"
    PackageChecker->>ConfigReader: "Read config files (fed_*.json)"
    ConfigReader-->>PackageChecker: "Return configuration"
    PackageChecker->>CheckRule: "Create CheckServerAvailable rule"
    PackageChecker->>CheckRule: "Execute check rules"
    CheckRule->>ConfigReader: "get_communication_scheme()"
    ConfigReader-->>CheckRule: "Return scheme (grpc/http/tcp)"
    alt GRPC/AGRPC scheme
        CheckRule->>NetworkChecker: "check_grpc_server_running()"
    else HTTP/TCP scheme
        CheckRule->>NetworkChecker: "check_socket_server_running()"
    end
    NetworkChecker-->>CheckRule: "Return connectivity status"
    CheckRule-->>PackageChecker: "Return CheckResult"
    PackageChecker->>ProcessRunner: "get_dry_run_command()"
    PackageChecker->>ProcessRunner: "run dry run test"
    ProcessRunner-->>PackageChecker: "Return dry run result"
    PackageChecker-->>PreflightTool: "Return all check results"
    PreflightTool-->>User: "Display preflight check results"

Loading

[greptile-apps]

Additional Comments (1)

1. tests/integration_test/preflight_check_test.py, line 80-90 (link)

   logic: Missing http/https handling. The client_package_checker.py skips GRPC checks for ALL non-GRPC schemes (including http), but this test expects GRPC checks for http. When using http scheme, this test will fail because:

   • Actual: only "Check dry run" (http is not grpc/agrpc)
   • Expected: "Check GRPC server available" + "Check dry run"

_{6 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[Copilot]

Pull request overview

This pull request updates the preflight check tool to dynamically adapt to different communication schemes (HTTP, TCP, GRPC) instead of being hardcoded to GRPC. The changes also remove deprecated overseer test functionality and update the preflight check tests to be scheme-aware.

Key changes:

• Added get_communication_scheme() function to detect the communication scheme from package configuration files
• Updated server and client package checkers to dynamically determine check names based on the actual scheme
• Refactored tests to parse preflight output programmatically and verify checks based on detected scheme

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
tests/integration_test/run_integration_tests.sh	Removed overseer test function and its invocation from the integration test runner
tests/integration_test/preflight_check_test.py	Added scheme-aware helper functions to dynamically determine expected checks; replaced hardcoded output assertions with parsed verification logic
tests/integration_test/data/projects/dummy.yml	Removed dummy overseer agent configuration that is no longer needed
nvflare/tool/package_checker/utils.py	Added get_communication_scheme() function to read scheme from config files; refactored credential loading to only occur when needed
nvflare/tool/package_checker/server_package_checker.py	Updated to dynamically create check names based on communication scheme; renamed function from _get_grpc_host_and_port to _get_fl_host_and_port
nvflare/tool/package_checker/client_package_checker.py	Updated to conditionally add GRPC connectivity check only for GRPC-based schemes; added TODO for future TCP connectivity check

---

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

[greptile-apps]

Additional Comments (2)

1. nvflare/tool/package_checker/utils.py, line 26-27 (link)

   style: Unused imports: Request, Session, and HTTPAdapter are no longer used after removing _create_http_session and _send_request. Only Response is still needed.

2. nvflare/tool/package_checker/server_package_checker.py, line 88 (link)

   style: Inconsistent scheme support: CheckServerAvailable in check_rule.py supports tcp and stcp schemes, but this validation excludes them. Consider adding tcp and stcp here if they are valid for server package checking, or document why they are excluded.

_{7 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (1)

1. nvflare/tool/package_checker/server_package_checker.py, line 88-89 (link)

   logic: Missing tcp and stcp schemes in validation. These are valid builtin schemes (see static_file.py:66: builtin_schemes = ["grpc", "tcp", "http"]) and are supported in CheckServerAvailable in check_rule.py:167. This will cause preflight check to fail for TCP-based FL deployments.

_{7 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[YuanTingHsieh]

/build

[greptile-apps]

Additional Comments (1)

1. nvflare/tool/package_checker/server_package_checker.py, line 88 (link)

   logic: Inconsistent scheme validation. check_rule.py:167 supports ["http", "https", "tcp", "stcp"] for socket-based checks, and tcp/stcp are valid schemes in the codebase (see tcp_driver.py). Consider adding "tcp" and "stcp" here:

_{7 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[YuanTingHsieh]

/build

[greptile-apps]

Additional Comments (1)

1. nvflare/tool/package_checker/server_package_checker.py, line 88-89 (link)

   logic: Inconsistent scheme support: CheckServerAvailable in check_rule.py:167 supports ["tcp", "stcp"], but this check excludes them. Server preflight check will fail with RuntimeError for tcp/stcp schemes, while client preflight check would work.

_{7 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[YuanTingHsieh]

/build

[greptile-apps]

Additional Comments (1)

1. nvflare/tool/package_checker/check_rule.py, line 162 (link)

   style: redundant type casting - port is already converted to int on line 155

   _{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

_{7 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}