Skip to content

Fix FedJob subdirectory sibling imports - #4959

Open
nvkevlu wants to merge 10 commits into
NVIDIA:mainfrom
nvkevlu:codex/fix-fedjob-subdir-sibling-export
Open

Fix FedJob subdirectory sibling imports#4959
nvkevlu wants to merge 10 commits into
NVIDIA:mainfrom
nvkevlu:codex/fix-fedjob-subdir-sibling-export

Conversation

@nvkevlu

@nvkevlu nvkevlu commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Fix FedJob export of sibling modules for subdirectory scripts.

Description

Fixes a regression from #4885 where a registered script such as src/client.py could no longer export a sibling imported as from net import Net.

Registered external scripts now search their source directory for unqualified imports, while recursively discovered package modules retain the existing collision protection.

Testing

  • Added regression coverage for src/client.py importing src/net.py
  • FedJob exporter tests: 21 passed
  • Broader job and recipe tests: 121 passed
  • PT swarm recipe test passed
  • ./runtest.sh -s passed

Types of changes

  • 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.

Copilot AI review requested due to automatic review settings July 24, 2026 20:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adjusts how FedJobConfig copies external scripts and their dependencies so that unqualified imports in entry-point scripts located in subdirectories can correctly resolve to sibling modules (and adds unit coverage for this behavior).

Changes:

  • Pass an is_external_script flag when copying registered external scripts so unqualified imports can be resolved against the script’s directory even when the derived module name contains dots (e.g., src.client).
  • Add a new unit test covering unqualified sibling imports for scripts in subdirectories and refine an existing test to better reflect package-import behavior.

Reviewed changes

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

File Description
tests/unit_test/job_config/fed_job_config_test.py Adds/updates unit tests for external script import-resolution and copy behavior.
nvflare/job_config/fed_job_config.py Extends _copy_source_file to optionally treat a file as an external entry-point when resolving unqualified imports.

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

Comment thread nvflare/job_config/fed_job_config.py Outdated
@greptile-apps

greptile-apps Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a regression introduced in #4885 where scripts registered from subdirectories (e.g. src/client.py) could no longer have their sibling imports (e.g. from net import Net) resolved during FedJob export. The fix introduces an is_external_script flag and an __init__.py-based heuristic to distinguish flat script directories from proper Python packages.

  • Core logic change (fed_job_config.py): _copy_source_file now accepts is_external_script; a new is_flat_external_script variable enables sibling-directory search only when the script's directory lacks __init__.py, preserving stdlib-collision protection for package scripts.
  • Regression test (fed_job_config_test.py): adds test_copy_ext_script_finds_unqualified_sibling_import_in_subdirectory and strengthens the existing stdlib-protection test by explicitly adding __init__.py to the pkg/ fixture.
  • Integration test (script_runner_test.py): adds an end-to-end GPU simulator test that verifies net.py lands at custom/net.py (not custom/src/net.py) when exported alongside src/poc_executor.py.

Confidence Score: 5/5

Safe to merge — the fix is minimal, well-targeted, and the three-test suite covers the regression case, the package-collision boundary, and end-to-end simulator export.

The __init__.py-presence check correctly reproduces Python's own runtime distinction between flat script directories and packages, making the guard semantically accurate rather than relying on the fragile "." not in module heuristic. The updated stdlib-collision test now explicitly includes pkg/__init__.py, confirming that protection remains intact for proper packages.

Files Needing Attention: No files require special attention.

Important Files Changed

Filename Overview
nvflare/job_config/fed_job_config.py Adds is_external_script flag to _copy_source_file and uses __init__.py presence to correctly gate sibling-directory import resolution; logic is sound and correctly mirrors Python's runtime sys.path behaviour.
tests/unit_test/job_config/fed_job_config_test.py Adds the regression test for subdirectory sibling imports and updates the stdlib-collision test to use pkg/__init__.py, correctly covering the behavioural boundary introduced by the fix.
tests/unit_test/job_config/script_runner_test.py Adds an end-to-end GPU simulator integration test that mocks Popen and asserts the correct export layout; assertions inside the side_effect callback are an unconventional but functional pattern.

Reviews (10): Last reviewed commit: "Merge branch 'main' into codex/fix-fedjo..." | Re-trigger Greptile

@codecov-commenter

codecov-commenter commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.26%. Comparing base (8b9541c) to head (d3bff0c).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4959   +/-   ##
=======================================
  Coverage   64.26%   64.26%           
=======================================
  Files        1030     1030           
  Lines      103578   103580    +2     
=======================================
+ Hits        66560    66570   +10     
+ Misses      37018    37010    -8     
Flag Coverage Δ
unit-tests 64.26% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nvkevlu
nvkevlu enabled auto-merge (squash) July 28, 2026 12:55
Comment thread nvflare/job_config/fed_job_config.py Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants