Adding no-BYOC support for Flower integration#4514
Adding no-BYOC support for Flower integration#4514d0uwe wants to merge 3 commits intoNVIDIA:mainfrom
Conversation
Greptile SummaryThis PR adds a "pre-deployed mode" to NVFlare's Flower integration, allowing the Flower app to live at a server-side path ( Confidence Score: 5/5Safe to merge — only a single P2 style issue (unused import) was found; all functional paths are correctly guarded and well tested. No P0 or P1 issues found. The feature is implemented correctly across the call stack, validation is in place, and the new test suite covers BYOC mode, pre-deployed mode, mutual-exclusion errors, and export integration. The only finding is an unused nvflare/app_opt/flower/executor.py (unused import); nvflare/job_config/fed_job_config.py (pre-existing Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[FlowerJob / FlowerRecipe] -->|flower_content set| B[BYOC Mode]
A -->|flower_app_path set| C[Pre-deployed Mode]
A -->|both or neither| D[ValueError raised]
B --> E[to_server: copies app into job ZIP custom/]
B --> F[to_clients: copies app into job ZIP custom/]
E --> G[FlowerServerApplet\nuses custom_dir from workspace]
F --> H[FlowerClientApplet\nuses custom_dir from workspace]
C --> I[FlowerController\npasses flower_app_path to applet]
I --> J[FlowerServerApplet\nvalidates path exists on server\nsets flower_app_dir = flower_app_path]
J --> K[Clients receive app\nfrom server via FAB]
Reviews (2): Last reviewed commit: "updating tests for flower no-BYOC" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| config_dir = os.path.join(job_dir, app_name, CONFIG) | ||
| custom_dir = os.path.join(job_dir, app_name, CUSTOM) | ||
| os.makedirs(config_dir, exist_ok=True) | ||
| os.makedirs(custom_dir, exist_ok=True) | ||
| # custom_dir will be created on-demand if custom code is added. |
There was a problem hiding this comment.
custom_dir not created before _copy_file_sources
Removing the unconditional os.makedirs(custom_dir, exist_ok=True) leaves custom_dir uncreated for jobs whose all components come from FL_PACKAGES (no user-defined classes), have no ext_scripts, and no ext_dirs, but do have a file_sources entry with app_folder_type="custom", dest_dir=None, and a file (not a directory) as the source. In _copy_file_sources, when dest_dir is None and os.path.isfile(src_path) is True, shutil.copy(src_path, dest_file) is called where dest_file is inside custom_dir. If custom_dir does not yet exist, shutil.copy raises FileNotFoundError. The shutil.copytree (directory) branch is safe because dirs_exist_ok=True creates the destination, but the file-copy branch has no such fallback.
| config_dir = os.path.join(job_dir, app_name, CONFIG) | |
| custom_dir = os.path.join(job_dir, app_name, CUSTOM) | |
| os.makedirs(config_dir, exist_ok=True) | |
| os.makedirs(custom_dir, exist_ok=True) | |
| # custom_dir will be created on-demand if custom code is added. | |
| os.makedirs(config_dir, exist_ok=True) | |
| os.makedirs(custom_dir, exist_ok=True) | |
| # custom_dir created eagerly; _copy_file_sources needs it for file (non-dir) sources. |
There was a problem hiding this comment.
How should we handle this? I noticed that if the custom_dir gets created, there always is an empty custom dir, which then triggers the byoc check, even though the directory is empty.
There was a problem hiding this comment.
The right fix is to add os.makedirs(dest_path, exist_ok=True) just before the shutil.copy call in _copy_file_sources. This way custom_dir is only created when there's actual content going into it — no empty directory, no spurious BYOC trigger.
The dest_dir is not None branch already does this correctly. The dest_dir is None (file-copy) branch just needs the same treatment:
| config_dir = os.path.join(job_dir, app_name, CONFIG) | |
| custom_dir = os.path.join(job_dir, app_name, CUSTOM) | |
| os.makedirs(config_dir, exist_ok=True) | |
| os.makedirs(custom_dir, exist_ok=True) | |
| # custom_dir will be created on-demand if custom code is added. | |
| def _copy_file_sources(self, config_dir, custom_dir, file_sources): | |
| for s in file_sources: | |
| # s is a tuple of (src_path, dest_dir) | |
| src_path, dest_dir, app_folder_type = s | |
| if app_folder_type == "config": | |
| target_dir = config_dir | |
| else: | |
| target_dir = custom_dir | |
| if dest_dir: | |
| dest_path = os.path.join(target_dir, dest_dir) | |
| if not os.path.exists(dest_path): | |
| os.makedirs(dest_path, exist_ok=True) | |
| else: | |
| dest_path = target_dir | |
| if os.path.isfile(src_path): | |
| base_name = os.path.basename(src_path) | |
| dest_file = os.path.join(dest_path, base_name) | |
| os.makedirs(dest_path, exist_ok=True) | |
| shutil.copy(src_path, dest_file) | |
| else: | |
| # this is a dir | |
| shutil.copytree(src_path, dest_path, dirs_exist_ok=True) |
shutil.copytree with dirs_exist_ok=True already handles directory creation, so that branch is fine. Only the file-copy branch needs the guard. config_dir is always pre-created so the makedirs is only a real operation when target_dir is custom_dir.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
holgerroth
requested review from
IsaacYangSLA,
YuanTingHsieh,
holgerroth,
nvidianz and
pcnudde
|
@d0uwe Thanks for the contribution. I think the feature is useful, especially for production Flower deployments where the app is pre-installed and should not be packaged into the NVFlare job ZIP. But I’m concerned about describing this as “no-BYOC.” With this PR, Flower app code is no longer carried through NVFlare Could we frame this as “server-predeployed Flower app mode” instead of “no-BYOC,” and add an explicit policy/guardrail for it? For example, Flower FAB execution could require a separate site opt-in when BYOC/custom-code execution is disabled. Ideally, I’m supportive of the use case, but I think we should preserve NVFlare’s security contract: disabling BYOC should not silently allow another dynamic code path to clients unless the site explicitly opts into that trust model. @IsaacYangSLA, @nvidianz, @pcnudde for viz |
2 participants
Description
This PR adds support for pre-deployed Flower application code in NVFlare's Flower integration, eliminating the need for BYOC (Bring Your Own Code) authorization in production environments.
Key Changes
New Feature: Pre-deployed Mode
flower_app_pathparameter toFlowerJob,FlowerRecipe,FlowerController,FlowerExecutor, andFlowerServerAppletflower_app_path, the Flower app code is not packaged into the job ZIP (nocustom/directory)Backwards Compatibility
flower_contentparameter continues to work exactly as before (BYOC mode)flower_content): Package Flower app in job ZIP (requires BYOC authorization)flower_app_path): Server-only deployment (no BYOC needed)Types of changes
./runtest.sh.