Added Firebase Realtime Database storage functionality in tracker store #8681
Conversation
Merging resolved conflicts and formatted files into main
updated poetry lock
…to RasaHQ-main
Merging changes from Rasa-HQ-main
|
|
|
Thanks for submitting a pull request 🚀 @amn41 will take a look at it as soon as possible ✨ |
|
This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
There was a problem hiding this comment.
@Thirunayan22 thank you for your contribution 💯 and my apologies for getting back so late.
I've left some comments that would be best addressed once you can pull the latest changes from main and resolve any conflicts (considering the long time passed, perhaps the best course of action would be to replicate this contribution in a fresh new PR targeting the latest main).
Please note that we also require rigorous unit test coverage, as well as a changelog entry to give details of your improvement.
| self.database_url = database_url | ||
| self.db_storage_path = db_storage_path | ||
|
|
||
| self.cred = credentials.Certificate(self.key_file_path) |
There was a problem hiding this comment.
| self.cred = credentials.Certificate(self.key_file_path) | |
| self.credentials = credentials.Certificate(self.key_file_path) |
| self.db_storage_path = db_storage_path | ||
|
|
||
| self.cred = credentials.Certificate(self.key_file_path) | ||
| init_app = firebase_admin.initialize_app( |
There was a problem hiding this comment.
I would store this as an instance attribute:
| init_app = firebase_admin.initialize_app( | |
| self.db = firebase_admin.initialize_app( |
| serialised_tracker = FirebaseRealtimeTrackerStore.serialise_tracker(tracker) | ||
|
|
||
| user_message_id = f"user_{int(time())}" | ||
| user_message = tracker.latest_message.text |
There was a problem hiding this comment.
We need to check that the latest message which is of type Optional[UserUttered] is not None:
| user_message = tracker.latest_message.text | |
| user_message = tracker.latest_message | |
| if user_message is not None: | |
| message_text = user_message.text | |
| else: | |
| message_text = None |
| bot_message_id = f"bot_{int(time())}" | ||
| bot_message = self.get_latest_bot_message(serialised_tracker) | ||
|
|
||
| if bot_message != None: |
There was a problem hiding this comment.
| if bot_message != None: | |
| if bot_message is not None: |
| self.push_conversation_db( | ||
| ref_path=f"{self.db_storage_path}/{tracker.sender_id}", | ||
| message_id=user_message_id, | ||
| message=user_message, |
There was a problem hiding this comment.
This method call should be extracted outside this if statement, by checking first that message_text is not None, because regardless of whether there is a bot response, the user message should be saved, right?
| """ | ||
| Supporting multiple outputs by bot dispatcher | ||
| """ | ||
| serialised_tracker = json.loads(serialised_tracker) |
There was a problem hiding this comment.
| serialised_tracker = json.loads(serialised_tracker) | |
| deserialised_tracker = json.loads(serialised_tracker) |
You could also re-use the deserialise_tracker method?
| else: | ||
| return None | ||
|
|
||
| def push_conversation_db(self, ref_path, message_id, message) -> None: |
There was a problem hiding this comment.
Missing type annotations for args, please add.
| def push_conversation_db(self, ref_path, message_id, message) -> None: | ||
|
|
||
| ref = db.reference(ref_path) | ||
| ref.update({message_id: message}) |
There was a problem hiding this comment.
I am not familiar at all with the firebase_admin library, but I have a hunch a call to push ref to the firebase db is missing here? Please investigate and add a few unit tests to check your assumptions.
| @@ -1257,6 +1349,14 @@ def _create_from_endpoint_config( | |||
| tracker_store = DynamoTrackerStore( | |||
| domain=domain, event_broker=event_broker, **endpoint_config.kwargs | |||
| ) | |||
| elif endpoint_config.type.lower() == "firebase-realtime": | |||
| tracker_store = FirebaseRealtimeTrackerStore( | |||
| database_url=endpoint_config.url, | |||
There was a problem hiding this comment.
Please maintain the order of args, first should be domain, followed by database_url, then event_broker etc.
| domain: Domain, | ||
| firebase_key_file_path: Text, | ||
| db_storage_path: Text, | ||
| database_url: Text, |
There was a problem hiding this comment.
Please consider using the parameter name host instead of database_url to maintain code consistency.
|
Hi @ancalita , no worries, and thanks a lot for reviewing the PR 💯 , I was also waiting for the feedback on this. As you noted, I will make the changes and get back to you. |
Proposed changes:
Status (please check what you already did):
black(please check Readme for instructions)