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.

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

Add ability for clients to send error log to server #3057

Merged
merged 11 commits into from
Nov 21, 2024

Conversation

nvkevlu
Copy link
Collaborator

@nvkevlu nvkevlu commented Nov 6, 2024

Renames the error log file to error_log.txt and adds a way to transmit the error logs from clients to the server.

Description

Renames the error log file to error_log.txt and adds a way to transmit the error logs from clients to the server. Currently should_report_error_log is set to False. This can be set to True to test the feature for now, and in the future we will want to add a way to configure this for each site.

On the server, the error log is simply printed, but future PRs will modify this to save the error log in the proper place.

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.

@nvkevlu nvkevlu marked this pull request as ready for review November 12, 2024 20:18
@nvkevlu
Copy link
Collaborator Author

nvkevlu commented Nov 12, 2024

/build

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.

Thanks!

Changes mostly LGTM, please sync up with @SYangster as I know you both are working on some logging structure.

nvflare/private/fed/server/fed_server.py Outdated Show resolved Hide resolved
nvflare/private/fed/client/client_executor.py Outdated Show resolved Hide resolved
SYangster
SYangster previously approved these changes Nov 15, 2024
Copy link
Collaborator

@SYangster SYangster 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

@YuanTingHsieh
Copy link
Collaborator

/build

@nvkevlu nvkevlu marked this pull request as draft November 16, 2024 00:18
@nvkevlu nvkevlu marked this pull request as ready for review November 19, 2024 21:27
@nvkevlu
Copy link
Collaborator Author

nvkevlu commented Nov 19, 2024

/build

@nvkevlu nvkevlu enabled auto-merge (squash) November 20, 2024 16:00
@nvkevlu
Copy link
Collaborator Author

nvkevlu commented Nov 20, 2024

/build

Copy link
Collaborator

@yanchengnv yanchengnv left a comment

Choose a reason for hiding this comment

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

Looks good in general. Please remove the channel definition that is not needed.

nvflare/fuel/f3/cellnet/defs.py Outdated Show resolved Hide resolved
@nvkevlu
Copy link
Collaborator Author

nvkevlu commented Nov 20, 2024

/build

@nvkevlu nvkevlu requested a review from yanchengnv November 20, 2024 23:01
Copy link
Collaborator

@yanchengnv yanchengnv left a comment

Choose a reason for hiding this comment

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

LGTM

@nvkevlu nvkevlu merged commit 82fe663 into NVIDIA:main Nov 21, 2024
20 checks passed
@nvkevlu nvkevlu deleted the send_client_errors branch November 21, 2024 20:56
@YuanTingHsieh YuanTingHsieh mentioned this pull request Nov 26, 2024
6 tasks
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.

4 participants