[2.8] Refresh stale integration tests

[YuanTingHsieh]

Summary

• Backport PR Refresh stale integration tests #4505 to the 2.8 branch.
• Refresh stale integration tests to match tightened recipe/API contracts.
• Provide required model/per-site fixtures for recipe smoke tests.
• Update preflight expectations/status handling and remove the embedded hello-pt pytest payload test.
• Carry over the related PyTorch persistor config/schema documentation clarifications from the source PR stack.

Validation

• PYTHONPYCACHEPREFIX=/tmp/nvflare_pycache python3 -m py_compile nvflare/app_opt/pt/model_persistence_format_manager.py nvflare/app_opt/pt/utils.py nvflare/tool/package_checker/package_checker.py nvflare/tool/preflight_check.py tests/integration_test/experiment_tracking_recipes_test.py tests/integration_test/preflight_check_test.py tests/integration_test/xgb_histogram_recipe_test.py tests/integration_test/xgb_vertical_recipe_test.py tests/unit_test/app_opt/pt/pt_param_validation_test.py tests/unit_test/tool/preflight_output_test.py
• python3 -m json.tool tests/integration_test/data/jobs/hello-pt/app/config/config_fed_server.json
• python3 -m json.tool tests/integration_test/data/apps/pt_init_client/app/config/config_fed_server.json
• bash -n tests/integration_test/run_integration_tests.sh
• git diff --check upstream/2.8...HEAD

[Copilot]

Pull request overview

Backport to the 2.8 branch that refreshes stale integration/unit tests and aligns preflight checking + PyTorch recipe fixtures/documentation with tightened contracts (notably around preflight status handling and PyTorch update schema expectations).

Changes:

• Replace numeric preflight checker return codes with a CheckStatus enum and update CLI/test expectations accordingly.
• Refresh integration recipe smoke tests (XGBoost vertical/histogram, experiment tracking) with required per-site/model fixtures and a more robust preflight runner setup.
• Update docs and test job/app configs to match updated PyTorch persistor/schema requirements and CLI naming (preflight-check).

Reviewed changes

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

Show a summary per file

File	Description
nvflare/tool/package_checker/package_checker.py	Introduces CheckStatus enum and refactors dry-run check return semantics.
nvflare/tool/preflight_check.py	Updates preflight CLI to interpret CheckStatus and map to exit codes/output.
tests/unit_test/tool/preflight_output_test.py	Updates/extends unit tests for new preflight status semantics and cleanup behavior.
tests/integration_test/run_integration_tests.sh	Ensures localhost0/localhost1 aliases exist before running preflight integration tests.
tests/integration_test/preflight_check_test.py	Loosens check assertions to allow additional checks and improves subprocess/pty handling.
tests/integration_test/xgb_vertical_recipe_test.py	Adds per-site config helper and passes per-site fixtures required by tightened recipe contracts.
tests/integration_test/xgb_histogram_recipe_test.py	Adds per-site config helper and aligns SimEnv usage with explicit client lists.
tests/integration_test/experiment_tracking_recipes_test.py	Supplies a model instance by importing the example’s model.py for recipe execution.
tests/integration_test/data/jobs/hello-pt/app/custom/test_custom.py	Removes an embedded pytest payload that was being collected unintentionally.
tests/integration_test/data/jobs/hello-pt/app/config/config_fed_server.json	Adds required persistor.args.model config for PyTorch persistor.
tests/integration_test/data/apps/pt_init_client/app/config/config_fed_server.json	Adds required persistor.args.model config for PyTorch persistor.
nvflare/app_opt/pt/utils.py	Clarifies docstring: extra keys may be ignored locally; server-side schema validation rejects unknown keys.
nvflare/app_opt/pt/model_persistence_format_manager.py	Clarifies update semantics: server checkpoint defines accepted client update key schema.
tests/unit_test/app_opt/pt/pt_param_validation_test.py	Renames tests and clarifies intent around partial updates vs rejecting unknown client keys.
docs/user_guide/nvflare_cli/preflight_check.rst	Updates CLI spelling (preflight-check) and documents exit code meanings + updated check names/messages.
docs/user_guide/data_scientist_guide/job_recipe.rst	Documents PyTorch update schema expectations (subset allowed; new keys rejected).

---

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

[greptile-apps]

Greptile Summary

This backport from main to the 2.8 branch refreshes stale integration tests and tightens API contracts across the preflight-check and PT model-persistence paths. The int-based return codes from PackageChecker.check() and check_dry_run() are replaced with a CheckStatus enum, fixing a latent bug where a successful dry run (ret_code=1) was previously treated as a failure by preflight_check.py.

• Enum-based CheckStatus replaces 0/1/2 return codes, making the cleanup logic in preflight_check.py unambiguous and closing a gap where PASS_WITH_CLEANUP was incorrectly classified as a failure.
• Recipe smoke tests now pass required model= (FedAvg) and per_site_config= (XGBoost) arguments that the tightened recipe contracts require, and the embedded hello-pt pytest payload test is removed in favour of top-level unit tests.
• Preflight integration tests relax the strict "no extra checks" assertion to allow dynamically added checks (e.g., Check dry run) without false failures, and capture the pty exit code correctly via os.waitstatus_to_exitcode.

Confidence Score: 5/5

Safe to merge — changes are test and documentation refreshes with a well-scoped enum refactor that fixes an existing misclassification of successful dry runs.

The enum refactor is straightforward and fully covered by the new unit tests. The integration test changes tighten expected state without removing meaningful coverage. No production logic paths were altered beyond the preflight-check plumbing.

No files require special attention, though the python vs python3 usage in run_integration_tests.sh is worth a second look for portability.

Important Files Changed

Filename	Overview
nvflare/tool/package_checker/package_checker.py	Introduces CheckStatus enum replacing int codes; removes finally-based return in check_dry_run with explicit returns per code path; adds except Exception handler that correctly returns FAIL but does not kill a started process.
nvflare/tool/preflight_check.py	Updated to use CheckStatus enum; fixes the latent bug where ret_code == 1 (successful dry run) was previously mapped to fail status.
tests/integration_test/preflight_check_test.py	Replaces subprocess.check_output with subprocess.run+check=False; captures PTY exit code correctly via os.waitstatus_to_exitcode; relaxes extra-check assertion; adds expect_success parameter.
tests/integration_test/run_integration_tests.sh	Adds ensure_localhost_aliases to auto-populate /etc/hosts; uses an EXIT trap for cleanup. The inline python command may not be available on systems where only python3 exists.
tests/integration_test/experiment_tracking_recipes_test.py	Adds _make_model() helper that dynamically imports model.py; passes required model= argument to FedAvg recipe constructor.
tests/integration_test/xgb_histogram_recipe_test.py	Extracts per-site config into _make_horizontal_per_site_config helper; switches SimEnv from num_clients= to clients=list(per_site_config).
tests/integration_test/xgb_vertical_recipe_test.py	Extracts per-site config into _make_vertical_per_site_config; adds missing per_site_config= to unit-style recipe constructor tests.
tests/unit_test/tool/preflight_output_test.py	Updates mock return values from bare ints to CheckStatus enum members; adds two new tests covering FAIL-without-cleanup and PASS_WITH_CLEANUP paths.
tests/unit_test/app_opt/pt/pt_param_validation_test.py	Renames two test functions to better describe client-vs-server schema semantics; adds docstrings clarifying intent.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[preflight_check.py check_packages] --> B{should_be_checked?}
    B -- No --> C[check_status = PASS]
    B -- Yes --> D[p.check]
    D --> E{all rules passed?}
    E -- No --> F[status = FAIL]
    E -- Yes --> G[check_dry_run]
    G --> H[run_command_in_subprocess]
    H -- timeout --> I[SIGTERM, PASS_WITH_CLEANUP]
    H -- returncode 0 --> J[PASS_WITH_CLEANUP]
    H -- returncode nonzero --> K[FAIL_WITH_CLEANUP]
    H -- Exception --> L[FAIL]
    F --> M[check returns status]
    I --> M
    J --> M
    K --> M
    L --> M
    C --> N{check_status?}
    M --> N
    N -- PASS_WITH_CLEANUP --> O[stop_dry_run force=False]
    N -- FAIL_WITH_CLEANUP --> P[stop_dry_run force=True]
    N -- PASS or FAIL --> Q[no cleanup]
    O --> R[output_ok / exit 0 or 1]
    P --> R
    Q --> R

Loading

_{Reviews (2): Last reviewed commit: "Merge branch '2.8' into codex/refresh-in..." | Re-trigger Greptile}

[YuanTingHsieh]

/build