Make Recipe job storage private#4903
Conversation
There was a problem hiding this comment.
Pull request overview
This PR completes the internal cleanup started in #4873 by making the generated FedJob an internal implementation detail of Recipe (renaming Recipe.job to Recipe._job) and updating internal callers/tests so customization flows through supported Recipe APIs rather than direct job access.
Changes:
- Rename Recipe’s stored job from
jobto_joband update Recipe internals/utilities accordingly. - Update framework-specific recipes (e.g., XGBoost, sklearn, edge) and extensive unit/integration tests to use
_jobinternally. - Add/adjust tests to confirm
Recipeno longer exposes a publicjobattribute and update error messaging to avoid directing users to internals.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit_test/recipe/utils_test.py | Updates recipe/job access and error expectations to _job. |
| tests/unit_test/recipe/swarm_recipe_test.py | Switches assertions and job map access to _job. |
| tests/unit_test/recipe/spec_test.py | Adds coverage asserting Recipe has _job and no public job; updates internal accesses. |
| tests/unit_test/recipe/server_memory_gc_rounds_test.py | Uses _job to inspect server controller config. |
| tests/unit_test/recipe/scaffold_recipe_test.py | Updates deploy map access and assertions to _job. |
| tests/unit_test/recipe/numpy_lr_recipe_test.py | Updates job assertions to _job. |
| tests/unit_test/recipe/flower_recipe_test.py | Updates expected internal job storage to _job. |
| tests/unit_test/recipe/fedstats_recipe_test.py | Updates expected internal job storage to _job. |
| tests/unit_test/recipe/fedopt_recipe_test.py | Updates job assertions/deploy map access to _job. |
| tests/unit_test/recipe/fedavg_recipe_test.py | Updates helpers and assertions to use _job deploy map/component ids. |
| tests/unit_test/recipe/fed_task_recipe_test.py | Updates deploy map/export usage to _job. |
| tests/unit_test/recipe/eval_recipe_test.py | Updates job assertions to _job. |
| tests/unit_test/recipe/edge_recipe_test.py | Updates job assertions and helper calls to _job. |
| tests/unit_test/recipe/cyclic_recipe_test.py | Updates deploy map access and assertions to _job. |
| tests/unit_test/app_opt/xgboost/xgboost_recipe_test.py | Updates deploy map access to _job. |
| tests/unit_test/app_opt/sklearn/sklearn_recipe_test.py | Updates job assertions to _job. |
| tests/unit_test/app_opt/pt/recipes/fed_eval_recipe_test.py | Updates job assertions/name checks to _job. |
| tests/integration_test/tools/export_recipe_job.py | Updates integration export helper to avoid recipe.job and rely on Recipe/internal storage. |
| tests/integration_test/slow/experiment_tracking_recipes_test.py | Updates file attachment calls to _job. |
| tests/integration_test/fast/study_session_test.py | Switches to recipe.execute(env) and reads job id from returned Run. |
| nvflare/recipe/utils.py | Updates helpers/utilities to resolve the backing job via _job and adjusts error wording. |
| nvflare/recipe/spec.py | Renames stored job to _job and updates all internal job interactions accordingly. |
| nvflare/edge/tools/edge_fed_buff_recipe.py | Updates environment processing path to use _job. |
| nvflare/app_opt/xgboost/recipes/vertical.py | Removes public self.job assignment; passes configured job into Recipe without exposing it. |
| nvflare/app_opt/xgboost/recipes/histogram.py | Same as above for histogram recipe. |
| nvflare/app_opt/xgboost/recipes/bagging.py | Same as above for bagging recipe. |
| nvflare/app_opt/sklearn/recipes/svm.py | Updates server component wiring to use _job. |
| nvflare/app_opt/sklearn/recipes/kmeans.py | Updates server component wiring to use _job. |
| nvflare/app_common/np/recipes/fedavg.py | Updates deploy map inspection and client wiring to use _job. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
da10a4b to
5201f8a
Compare
Greptile SummaryThis PR makes generated Recipe jobs an internal implementation detail. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (7): Last reviewed commit: "Preserve per-site apps when adding NumPy..." | Re-trigger Greptile |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4903 +/- ##
==========================================
+ Coverage 61.77% 61.79% +0.02%
==========================================
Files 985 985
Lines 95805 95771 -34
==========================================
+ Hits 59182 59185 +3
+ Misses 36623 36586 -37
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.
One non-blocking design question.
YuanTingHsieh
left a comment
There was a problem hiding this comment.
One blocking issue found on re-review.
Makes Recipe job storage private.
Description
Follow-up to #4873, which migrated public examples and documentation away from direct
recipe.jobaccess.This PR completes the cleanup by making the generated
FedJoban internal Recipe implementation detail:Recipe.jobtoRecipe._job.self.jobassignments from XGBoost recipes.jobattribute.No
recipe.jobreferences remain in the repository. No runtime behavior changes are intended; users should customize recipes through the supported Recipe APIs.Testing
510 passed, 23 skipped96 passed2 passed./runtest.sh --skip-install -s: passedTypes of changes
./runtest.sh.