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

Refactor AWS and Azure cloud scripts of server and client #2275

Merged
merged 1 commit into from
Jan 20, 2024
Merged

Refactor AWS and Azure cloud scripts of server and client #2275

merged 1 commit into from
Jan 20, 2024

Conversation

IsaacYangSLA
Copy link
Collaborator

@IsaacYangSLA IsaacYangSLA commented Jan 11, 2024 •
edited
Loading

Fixes # .

Refactor and remove sections on AWS server, AWS client and Azure server and Azure client.

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.

@IsaacYangSLA
Copy link
Collaborator Author

/build

chesterxgchen
chesterxgchen requested changes Jan 12, 2024
View reviewed changes
Copy link
Collaborator

@chesterxgchen chesterxgchen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

this may need additional changes

@IsaacYangSLA IsaacYangSLA force-pushed the template_cleanup_2 branch 2 times, most recently from cd8bfc5 to e6899e8 Compare January 12, 2024 17:00
chesterxgchen
chesterxgchen reviewed Jan 12, 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.

can we further refactor the gen_xxx functions
for example
def gen_server():
gen_config()
perpare_startup_dir()
prepare_certificates()
gen_start_stop_scripts()
for csp in provide_cloud_provides:
gen_csp_start_scripts(csp)
gen_csp_stop_scripts(csp)

all aws related scripts go to aws_file
all azure related scripts go to azure file
include template replacements etc.

@IsaacYangSLA IsaacYangSLA force-pushed the template_cleanup_2 branch 5 times, most recently from 0b96d5e to 26773de Compare January 17, 2024 23:39
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 17, 2024
View reviewed changes
nvflare/lighter/utils.py Dismissed Show dismissed Hide dismissed
@IsaacYangSLA
Copy link
Collaborator Author

can we further refactor the gen_xxx functions for example def gen_server(): gen_config() perpare_startup_dir() prepare_certificates() gen_start_stop_scripts() for csp in provide_cloud_provides: gen_csp_start_scripts(csp) gen_csp_stop_scripts(csp)

all aws related scripts go to aws_file all azure related scripts go to azure file include template replacements etc.

There are four combinations for CSP's AND participant types, azure+server, azure+client, aws+server, aws+client. Therefore, one statement for one combination seems good enough and there seems no need to create a function for that.

chesterxgchen
chesterxgchen reviewed Jan 18, 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 few questions and comments

nvflare/dashboard/application/blob.py Outdated Show resolved Hide resolved
nvflare/dashboard/application/blob.py Outdated Show resolved Hide resolved
nvflare/dashboard/application/blob.py Outdated Show resolved Hide resolved
nvflare/lighter/tplt_utils.py Outdated Show resolved Hide resolved
@IsaacYangSLA
AWS server and client OK
a1cf0f0 
Azure template updated

Refactor sub_start.sh on both server and client

Address PR comments

Address a few PR comments
chesterxgchen
chesterxgchen approved these changes Jan 20, 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

@IsaacYangSLA
Copy link
Collaborator Author

/build

1 similar comment
@IsaacYangSLA
Copy link
Collaborator Author

/build

@IsaacYangSLA IsaacYangSLA merged commit a0e8523 into NVIDIA:main Jan 20, 2024
16 checks passed
@IsaacYangSLA IsaacYangSLA deleted the template_cleanup_2 branch July 31, 2024 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chesterxgchen chesterxgchen chesterxgchen approved these changes

@SYangster SYangster Awaiting requested review from SYangster

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

Successfully merging this pull request may close these issues.
3 participants
@IsaacYangSLA @chesterxgchen @SYangster