Replace public Recipe.job usage with purpose-built Recipe APIs#4873
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes public-facing reliance on the internal recipe.job attribute by introducing purpose-built Recipe APIs and migrating examples/docs/research jobs to those APIs, keeping Recipe.job usage confined to internal implementation details.
Changes:
- Added new Recipe/utility APIs:
Recipe.enable_tensor_streaming(...)andadd_final_global_evaluation(...). - Migrated multiple examples/research jobs/docs from
recipe.job.*usage to supported Recipe helpers (add_server_config,add_*_file,export,enable_log_streaming, etc.). - Updated documentation to clarify client-side experiment tracking patterns (including per-site targeting constraints) and revised TensorFlow validator guidance.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| research/brats18/job.py | Switches server config placement from recipe.job.to_server to recipe.add_server_config. |
| research/auto-fl-research/tasks/vlm_med/job.py | Replaces custom final-eval helper (and recipe.job usage) with add_final_global_evaluation. |
| research/auto-fl-research/tasks/cifar10/job.py | Replaces custom final-eval helper (and recipe.job usage) with add_final_global_evaluation. |
| nvflare/recipe/utils.py | Adds add_final_global_evaluation and adjusts cross-site evaluation docs wording. |
| nvflare/recipe/spec.py | Adds Recipe.enable_tensor_streaming(...) to configure matching server/client tensor streamers. |
| nvflare/recipe/init.py | Exposes add_final_global_evaluation via package exports. |
| nvflare/app_opt/tf/tf_validator.py | Updates TFValidator doc example to use the lower-level FedJob API for component placement. |
| examples/hello-world/hello-log-streaming/README.md | Updates Recipe log-streaming example to use recipe.enable_log_streaming("log.txt"). |
| examples/advanced/tensor-stream/README.md | Updates tensor streaming setup docs to use recipe.enable_tensor_streaming(). |
| examples/advanced/tensor-stream/job.py | Uses recipe.enable_tensor_streaming() instead of direct streamer placement. |
| examples/advanced/recipe-k8s/job.py | Migrates file bundling to recipe.add_server_file / recipe.add_client_file. |
| examples/advanced/kaplan-meier-he/job.py | Migrates export from recipe.job.export_job(...) to recipe.export(...). |
| examples/advanced/experiment-tracking/wandb/README.md | Migrates client-side targeting example to add_experiment_tracking(...). |
| examples/advanced/experiment-tracking/README.md | Updates MLflow client-side tracking docs and explains per-site targeting requirements. |
| examples/advanced/experiment-tracking/mlflow/hello-pt-mlflow-client/README.md | Expands client-side MLflow docs and migrates to add_experiment_tracking(...). |
| docs/user_guide/data_scientist_guide/recipe_api.rst | Documents the new enable_tensor_streaming and add_final_global_evaluation helpers. |
| docs/user_guide/data_scientist_guide/job_recipe.rst | Clarifies guidance around mutating generated recipe/job metadata. |
| docs/design/nvflare_cli.md | Updates log-streaming guidance for Recipe jobs to use recipe.enable_log_streaming("log.txt"). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile SummaryThis PR completes the migration of public-facing examples, documentation, and research jobs away from direct
Confidence Score: 5/5Safe to merge; all new Recipe APIs are well-guarded and thoroughly unit-tested. The two new Recipe APIs both have idempotency guards, input validation, and matching unit tests covering error paths. The MLflow defaults are correctly isolated via deep copies so the caller's dict is never mutated. The default MLflow store change is intentional and documented. No logic errors were found in the changed code paths. The integration test helper Important Files Changed
Reviews (8): Last reviewed commit: "Merge branch 'main' into codex/recipe-pu..." | Re-trigger Greptile |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4873 +/- ##
==========================================
+ Coverage 60.99% 61.06% +0.07%
==========================================
Files 980 980
Lines 93754 93818 +64
==========================================
+ Hits 57184 57294 +110
+ Misses 36570 36524 -46
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
YuanTingHsieh
left a comment
There was a problem hiding this comment.
Changes LGTM, make sure you tested these updated examples.
And for some utils can @holgerroth and @ZiyueXu77 comment on if that util makes sense?
otherwise we could just use the recipe.add_server_component or recipe.add_client_component
holgerroth
left a comment
There was a problem hiding this comment.
A few non-blocking suggestions from code review. The overall direction is great — removing recipe.job from public examples is the right cleanup.
There was a problem hiding this comment.
This makes sense. Nice cleanup — getting recipe.job out of the public examples and docs is a good step, and the new enable_tensor_streaming and add_final_global_evaluation APIs are good additions. All my review comments were addressed in the latest commits, including the matching-defaults test for the validation timeout.
Makes Recipe job storage private. ### Description Follow-up to #4873, which migrated public examples and documentation away from direct `recipe.job` access. This PR completes the cleanup by making the generated `FedJob` an internal Recipe implementation detail: - Renames `Recipe.job` to `Recipe._job`. - Updates Recipe internals, utilities, framework-specific recipes, and tests. - Removes temporary public `self.job` assignments from XGBoost recipes. - Updates integration paths to use public Recipe APIs where appropriate. - Adds coverage confirming that Recipe no longer exposes a public `job` attribute. - Rewords errors so they do not direct users toward generated-job internals. No `recipe.job` references remain in the repository. No runtime behavior changes are intended; users should customize recipes through the supported Recipe APIs. ### Testing - Recipe-focused unit tests: `510 passed, 23 skipped` - Recipe utility tests: `96 passed` - Multi-study ProdEnv integration: passed - Per-site export regression test: passed - Server- and client-side MLflow simulations: `2 passed` - `./runtest.sh --skip-install -s`: passed ### 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.
Replaces public Recipe.job usage with purpose-built Recipe APIs.
Description
Builds on #4862 to remove direct
recipe.jobusage from public examples, documentation, and research jobs.recipe.enable_tensor_streaming(...)for configuring matching server and client tensor streamers.add_final_global_evaluation(...)for evaluating a PyTorch recipe’s persisted global model without requesting client model submissions.add_server_file()and targetedadd_client_file().After these changes, there are no remaining
recipe.jobreferences underexamples/,docs/, orresearch/.This PR intentionally does not rename the internal
Recipe.jobattribute to_job. That mechanical/internal migration will be handled in a separate PR to keep this change focused and easier to review.Types of changes
./runtest.sh.