Conversation
Greptile SummaryThis PR implements Phase 1 of multi-study support for NVFlare — study metadata plumbing from user-facing APIs through to job persistence and runtime launchers, with full backward compatibility and no access-control changes. The study is established once at session login time, propagated via the signed session token, and used server-side to scope Key changes:
One minor observation worth addressing: when the server rejects a login due to an invalid study name, the rejection is silent on the server side (no log) and the client receives the generic Confidence Score: 5/5Safe to merge — implementation is complete, well-tested, and fully backward compatible; only a minor observability gap remains. All remaining findings are P2 (observability/style). No P0 or P1 issues were found. The study validation is applied at both client and server. Backward compatibility is preserved for legacy jobs and legacy session tokens. The previously flagged issues (sys.exit(1) on invalid study, props-merge in _do_default) are both resolved. Test coverage is thorough across unit, integration, and admin-shell paths. nvflare/fuel/hci/server/login.py — the invalid-study rejection path should add a server-side warning log and ideally a more descriptive client-facing error. Important Files Changed
Sequence DiagramsequenceDiagram
participant User as Admin/API User
participant Client as AdminAPI/AdminClient
participant Server as LoginModule (Server)
participant Session as SessionManager
participant JobCmds as JobCommandModule
User->>Client: new_secure_session(study="cancer-research")
Client->>Client: validate study (name_check)
Client->>Server: _cert_login headers={study: "cancer-research", ...}
Server->>Server: validate study (name_check)
Server->>Session: create_session(active_study="cancer-research")
Session-->>Server: session w/ token (encodes study as "t")
Server-->>Client: LOGIN OK + token
Note over Client,Server: Session now scoped to "cancer-research"
User->>Client: submit_job("/path/to/job")
Client->>Server: submit_job command (uses existing token)
Server->>Server: read active_study from conn props
Server->>JobCmds: meta["study"] = "cancer-research"
JobCmds-->>Client: job_id
User->>Client: list_jobs()
Client->>Server: list_jobs command
Server->>Server: requested_study = conn.ACTIVE_STUDY ("cancer-research")
Server->>JobCmds: filter: get_job_meta_study(job) == "cancer-research"
JobCmds-->>Client: only "cancer-research" jobs
Note over Server,JobCmds: Legacy job (no study field) → get_job_meta_study → "default"
Note over Server,Session: Token decode: user.get("t", DEFAULT_STUDY) for backward compat
Reviews (12): Last reviewed commit: "Merge branch 'main' into feature/multist..." | Re-trigger Greptile |
|
/build |
|
/build |
chesterxgchen
left a comment
There was a problem hiding this comment.
Looks good overall,
but I did not see Job API changes, I thought by specify study, the deploy_map will be generated assign the app with associated study to specified sites, this might trigger some Job API change to reflect this requirements
I was going to look into this in phase2. For this it assumes the same sites are part of all studies. |
|
/build |
|
/build |
…phase1-pr # Conflicts: # nvflare/app_opt/job_launcher/k8s_launcher.py
|
/build |
|
/build |
1 similar comment
|
/build |
|
/build |
|
/build |
|
/build |
1 similar comment
|
/build |
|
/build |
Summary
list_jobsby session study, persist study on submit, preserve the source study on clone, and normalize legacy jobs without study metadata todefault