-
Notifications
You must be signed in to change notification settings - Fork 15
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
Fix request service handling of eval service #433
Fix request service handling of eval service #433
Conversation
Fixing setup of arguments with values related to communicating with evaluation service, which were incorrectly overwriting dests for analogous args for communicating with data service; also, ensuring the evaluation service SSL dir arg was passed to service manager class.
Protect general execution of RequestService manager class to still be usable even without the evaluation service being online in the Docker stack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Only found two nothing-burgers that aren't necessarily in scope or high priority
@@ -170,6 +175,10 @@ async def listener(self, websocket: WebSocketServerProtocol, path): | |||
event_type = MessageEventType.INVALID if req_message is None else req_message.get_message_event_type() | |||
|
|||
if isinstance(req_message, LaunchEvaluationMessage) or isinstance(req_message, OpenEvaluationMessage): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if isinstance(req_message, (LaunchEvaluationMessage, OpenEvaluationMessage))
instead of if isinstance(req_message, LaunchEvaluationMessage) or isinstance(req_message, OpenEvaluationMessage)
might read better here.
) | ||
self._eval_handler_exception = None | ||
except Exception as e: | ||
self._evaluation_service_handler = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth adding some logging here.
Deals with some issues in how the request service deals with evaluation service connection details, as the latter doesn't actually exist in the
main
deployment stack yet. These are currently keeping the request service from starting successfully.try
block, falling back to aNone
value if something is raisedNone
.