Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Docker job launcher #3072

Merged
merged 14 commits into from
Dec 4, 2024
Merged

Docker job launcher #3072

merged 14 commits into from
Dec 4, 2024

Conversation

yhwen
Copy link
Collaborator

@yhwen yhwen commented Nov 25, 2024 •
edited
Loading

Fixes # .

Description

  • Implemented the docker job launchers for server and client. Provided the options to launch the job run SJ and CJ into another docker container.
  • Refactored the ClientProcessJobLauncher and ServerProcessJobLauncher for sharing the same codes with the docker job launchers.
  • Replaced the "server" to use the Const.

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.

YuanTingHsieh
YuanTingHsieh reviewed Nov 25, 2024
View reviewed changes
Copy link
Collaborator

@YuanTingHsieh YuanTingHsieh left a comment

Choose a reason for hiding this comment

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

It might be clearer if we separate this into 2 PRs, one for Docker job launcher the other for the "server" change.

nvflare/private/fed/app/deployer/server_deployer.py Show resolved Hide resolved
nvflare/app_common/job_launcher/client_process_launcher.py Outdated Show resolved Hide resolved
nvflare/app_common/job_launcher/server_process_launcher.py Outdated Show resolved Hide resolved
chesterxgchen
chesterxgchen requested changes Nov 27, 2024
View reviewed changes
Copy link
Collaborator

@chesterxgchen chesterxgchen left a comment

Choose a reason for hiding this comment

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

add comments and suggest changes

YuanTingHsieh
YuanTingHsieh reviewed Dec 4, 2024
View reviewed changes
Copy link
Collaborator

@YuanTingHsieh YuanTingHsieh left a comment

Choose a reason for hiding this comment

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

I like the refactoring of "job_launcher_utils", one additional thing we could do in the future is that, we have "utils/xxx" and "fuel/utils/xxx" and "yyy/utils/xxx" we could see how we can consolidate all of them, or review to see if they are in the right place.

chesterxgchen
chesterxgchen approved these changes Dec 4, 2024
View reviewed changes
Copy link
Collaborator

@chesterxgchen chesterxgchen left a comment

Choose a reason for hiding this comment

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

LGTM

@YuanTingHsieh
Copy link
Collaborator

/build

@yhwen yhwen merged commit c6f2521 into NVIDIA:main Dec 4, 2024
20 checks passed
@yhwen yhwen deleted the docker_job_launcher branch December 4, 2024 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YuanTingHsieh YuanTingHsieh YuanTingHsieh left review comments

@chesterxgchen chesterxgchen chesterxgchen approved these changes

@IsaacYangSLA IsaacYangSLA Awaiting requested review from IsaacYangSLA

@yanchengnv yanchengnv Awaiting requested review from yanchengnv

@nvidianz nvidianz Awaiting requested review from nvidianz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yhwen @YuanTingHsieh @chesterxgchen @yanchengnv