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

DataBus #2285

Merged
merged 10 commits into from
Feb 5, 2024
Merged

DataBus #2285

merged 10 commits into from
Feb 5, 2024

Conversation

chesterxgchen
Copy link
Collaborator

Description

As part of the series PRs related to In-Process Client API Executor and WFCommAPI. This is the 1st PR for common utiliies
MessageBus is an singleton message store and publish (callbacks) structure that other can subscribe (callbacks) and receive message in callback when message is published. The event Manager can be built on top of messagebus.
The message can also send to/receive from message store for given topic.

The messagebus allows two components from strong coupling to loose coupling. For example, instead comp1 set a proprty directly on comp2. We can use messagebus, where comp1 send a message to messagebus, the comp2 receive the message from messagebus. comp2 is unaware who send the message and comp1 is unware who received the message.

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.

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.

We need to discuss the general design of the data model and how the data is intended to be used.

nvflare/fuel/message/message_bus.py Outdated Show resolved Hide resolved
nvflare/fuel/message/message_bus.py Outdated Show resolved Hide resolved
nvflare/fuel/message/message_bus.py Outdated Show resolved Hide resolved
nvflare/fuel/message/message_bus.py Outdated Show resolved Hide resolved
nvflare/fuel/message/message_bus.py Outdated Show resolved Hide resolved
nvflare/fuel/message/event_manger.py Outdated Show resolved Hide resolved
nvflare/fuel/message/message_bus.py Outdated Show resolved Hide resolved
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.

Suggest add docstrings, one method naming.

nvflare/fuel/message/data_bus.py Outdated Show resolved Hide resolved
nvflare/fuel/message/data_bus.py Outdated Show resolved Hide resolved
nvflare/fuel/message/event_manager.py Outdated Show resolved Hide resolved
nvflare/fuel/utils/function_utils.py Outdated Show resolved Hide resolved
nvflare/fuel/message/data_bus.py Outdated Show resolved Hide resolved
nvflare/fuel/message/data_bus.py Outdated Show resolved Hide resolved
@chesterxgchen chesterxgchen changed the title Message bus DataBus Jan 23, 2024
@chesterxgchen chesterxgchen force-pushed the message_bus branch 2 times, most recently from a70904b to e880033 Compare January 24, 2024 18:12
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.

See my comments. Mainly about the separation of the data storage and pub/sub interface.

nvflare/fuel/message/__init__.py Outdated Show resolved Hide resolved
nvflare/fuel/message/data_bus.py Outdated Show resolved Hide resolved
nvflare/fuel/message/data_bus.py Outdated Show resolved Hide resolved
nvflare/fuel/message/data_bus.py Outdated Show resolved Hide resolved
nvflare/fuel/message/data_bus.py Outdated Show resolved Hide resolved
nvflare/fuel/message/data_bus.py Outdated Show resolved Hide resolved
nvflare/fuel/message/data_bus.py Outdated Show resolved Hide resolved
nvflare/fuel/message/data_bus.py Outdated Show resolved Hide resolved
nvflare/fuel/message/event_manager.py Outdated Show resolved Hide resolved
nvflare/fuel/utils/function_utils.py Outdated Show resolved Hide resolved
@chesterxgchen
Copy link
Collaborator Author

/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 very good in general. Suggest to change some method names.

nvflare/fuel/data_event/data_bus.py Outdated Show resolved Hide resolved
nvflare/fuel/data_event/data_bus.py Outdated Show resolved Hide resolved
chesterxgchen and others added 9 commits February 5, 2024 10:14
address PR comments
formatting
* check invalid input directory

* check invalid input directory

add doc string

add doc string

rename receive_messages() to receive_message()

change doc str to google doc string style

rebase and formats
1) remove data store scope/topic
2) add pub_sub interface and let databus implements the inferface
3) remove function_utils.py and unit tests for another PR
1) remove data store scope/topic
2) add pub_sub interface and let databus implements the inferface
3) remove function_utils.py and unit tests for another PR
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

@SYangster
Copy link
Collaborator

/build

@SYangster
Copy link
Collaborator

/build

@YuanTingHsieh YuanTingHsieh merged commit 8cb03ed into NVIDIA:main Feb 5, 2024
16 checks passed
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