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

Refactor controllers to use DialogueKit platforms #181

Conversation

NoB0
Copy link
Contributor

@NoB0 NoB0 commented Jul 5, 2023

What's changed

  • server_rest and server_socket are removed
  • Controller inherits from DialogueKit's Platform
  • Recommended items are now stored as metadata to the recommendation utterance
  • Update of http_data_formatter
  • Update tests for Flask socketio platform
  • Update DialogueKit version to v0.0.9.dev0 to resolve dependencies conflicts

Fixes #180

@NoB0 NoB0 linked an issue Jul 5, 2023 that may be closed by this pull request
@github-actions
Copy link

github-actions bot commented Jul 5, 2023

Current Branch Main Branch
Coverage Badge Coverage Badge

@NoB0 NoB0 marked this pull request as ready for review August 11, 2023 16:03
@NoB0 NoB0 requested review from WerLaj and IKostric August 15, 2023 12:50
Copy link
Contributor

@WerLaj WerLaj left a comment

Choose a reason for hiding this comment

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

LGTM with some minor comments and requests for clarification

moviebot/controller/http_data_formatter.py Show resolved Hide resolved
moviebot/agent/agent.py Outdated Show resolved Hide resolved
moviebot/controller/controller_flask_rest.py Outdated Show resolved Hide resolved
moviebot/controller/controller_flask_rest.py Outdated Show resolved Hide resolved
moviebot/controller/controller_flask_rest.py Outdated Show resolved Hide resolved
moviebot/controller/controller_flask_rest.py Show resolved Hide resolved
moviebot/controller/controller_flask_socket.py Outdated Show resolved Hide resolved
moviebot/controller/controller_flask_socket.py Outdated Show resolved Hide resolved
moviebot/controller/controller_flask_socket.py Outdated Show resolved Hide resolved
moviebot/controller/controller_flask_socket.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@IKostric IKostric left a comment

Choose a reason for hiding this comment

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

LGTM. There are some minor inline comments.

requirements.txt Outdated Show resolved Hide resolved
moviebot/agent/agent.py Outdated Show resolved Hide resolved
moviebot/agent/agent.py Show resolved Hide resolved
self.socketio.on_namespace(ChatNamespace("/", self))
self.socketio.run(self.app, host=host, port=port)

def display_agent_utterance(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems this method is already defined in FlaskSocketPlatform.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is correct however here we use different classes Message and Response that are tailored for our use case.

moviebot/controller/controller_flask_rest.py Outdated Show resolved Hide resolved
@NoB0 NoB0 merged commit 4fec288 into main Aug 31, 2023
4 checks passed
@NoB0 NoB0 deleted the feature/180-Refactor-controller-package-using-platform-from-DialogueKit branch August 31, 2023 12:28
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.

Refactor controller package using platform from DialogueKit
3 participants