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 storage capability for client logs and allow for use with LogSender and LogReceiver #3077

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

nvkevlu
Copy link
Collaborator

@nvkevlu nvkevlu commented Nov 26, 2024

Add storage capability for client logs and allow for use with LogSender and LogReceiver.

Description

To use this, add this component to the resources on the client site:
{
"id": "log_sender",
"path": "nvflare.app_common.widgets.log_streaming.LogSender",
"args": {}
},

and with this configured on the server:
{
"id": "log_receiver",
"path": "nvflare.app_common.widgets.log_streaming.LogReceiver",
"args": {}
},

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 enabled auto-merge (squash) November 26, 2024 21:31
@nvkevlu
Copy link
Collaborator Author

nvkevlu commented Nov 26, 2024

/build

@nvkevlu
Copy link
Collaborator Author

nvkevlu commented Nov 26, 2024

/build

nvflare/apis/dxo.py Outdated Show resolved Hide resolved
nvflare/apis/impl/job_def_manager.py Outdated Show resolved Hide resolved
nvflare/apis/storage.py Outdated Show resolved Hide resolved
nvflare/app_common/widgets/log_streaming.py Outdated Show resolved Hide resolved
nvflare/app_common/widgets/log_streaming.py Outdated Show resolved Hide resolved
@nvkevlu
Copy link
Collaborator Author

nvkevlu commented Dec 2, 2024

Refactored to make it more configurable

@nvkevlu
Copy link
Collaborator Author

nvkevlu commented Dec 2, 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.

Mostly good, added some questions

nvflare/apis/impl/job_def_manager.py Outdated Show resolved Hide resolved
nvflare/app_common/widgets/log_streaming.py Outdated Show resolved Hide resolved
nvflare/app_common/widgets/log_streaming.py Outdated Show resolved Hide resolved
nvflare/app_common/widgets/log_streaming.py Show resolved Hide resolved
nvflare/app_common/widgets/log_streaming.py Outdated Show resolved Hide resolved
nvflare/app_common/widgets/log_streaming.py Show resolved Hide resolved
Comment on lines +115 to +117
FileStreamer.register_stream_processing(
fl_ctx, channel=channel, topic=LOG_STREAM_EVENT_TYPE, stream_done_cb=self.process_log
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could also say this LogSender and LogReceiver is always using the same channel but for different types of logs they send/receive they use different "topic"?

Now is the same "topic" but different "channel" I guess we just need to explain what we prefer in the docstring

LOG_DATA = "log_data"


class LogSender(Widget):
Copy link
Collaborator

Choose a reason for hiding this comment

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

since this component only works in client, what happen if users put this in server side?
Do we need some checking against that?

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.

2 participants