-
Notifications
You must be signed in to change notification settings - Fork 6
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 realtime inference consumer #90
base: dev
Are you sure you want to change the base?
Conversation
Release 0.3.0
Release 0.4.0
- Includes various implementations includeing appserver notification actions and email user. - Conditions evaluation using json path (currently locally, later read jsonpath from appconfig too).
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.
Yes this looks like a very good start! At some point the threshold may need to come from radar-app-config service instead of from a fixed value. This may also be a good entry point to enable / disable the condition for a given project (e.g. to only process records for certain projects).
.getProperties() | ||
.getOrDefault( | ||
"management_portal_token_url", | ||
"https://radar-cns-platform.rosalind.kcl.ac.uk/managementportal/api/ouath/token"); |
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.
Preferably keep this out of the code. If you want to provide defaults, it would be better to define a base URL somewhere, and use that plus the default path as the default.
new EmailSender( | ||
(String) props.getOrDefault("host", "localhost"), | ||
(Integer) props.getOrDefault("port", 25), | ||
(String) props.getOrDefault("from", "[email protected]"), |
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.
This should not have a default.
objectMapper = new ObjectMapper(); | ||
|
||
if (tokenUrl == null || tokenUrl.isEmpty()) { | ||
logger.warn("MP Token url was not provided. Will use unauthenticated requests to appserver."); |
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.
Is this ever the case? It would be better to exclude this.
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.
Unlikely in a production environment. The appserver does support a no-security mode where unauthenticated requests can be made (particularly useful for dev environments). I will remove it once I have tested it with the appserver.
Yes definitely can create impl to read the config from the app-config.
Yes a good idea, will have a think through how it can be done, since then conditions will need to be accessed based on the project in the record in real-time. EDIT: Actually made projects a part of conditions and actions, so you can specify a projects list to filter- ...
conditions:
- name: 'LocalJsonPathCondition'
projects: ['radar-test']
...
If no projects are provided, works as default on all projects. But now I am wondering how this will be achieved when we move to app-config - will the conditions have the projects key? Or will it be on the request path like |
* <p>To be used with the model-invocation-endpoint and KSQL API_INFERENCE function to evaluate and | ||
* take action on incoming results from realtime inference on data. | ||
*/ | ||
public class RealtimeInferenceConsumer implements KafkaMonitor { |
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 I understand it correctly, all of these Consumers are realtime. We can remove that word from the class name I think.
Why not reuse/extend from existing AbstractKafkaMonitor?
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.
I think the adjective realtime was added for inference and not for consumers. So these are consumers for realtime inference as compared to batch or historic inference. But I agree if it seems confusing best to remove it. AbstractKafkaMonitor was not extended just to allow more flexibility (like for now a different constructor spec) but can definitely extend from AbstractKafkaMonitor if we need functionality already present there.
.map(c -> createConsumer(handler.getRadarProperties(), c))); | ||
} | ||
|
||
private static KafkaMonitor createConsumer( |
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.
I would call it InferenceMonitor, InferenceMonitorFactory for consistency
} | ||
} | ||
|
||
public Map<String, Object> createMessage( |
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.
you are referring to the create operation that is done on the service that receives the request. Right? I think this should be either sendMessage or addMessage.
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.
yes good point
Also replaced all deprecated code
- Add realtime consumer to docker builds for integration tests - Fix starting for docker application when no schemas in schema registry - Updates classes and code based on expected API spec of appserver - Reuse classes wherever possible - Fix timezone conversions and add jitter and tolerance to the time calculations - Simplify configs and string formatting - Handle unhandled exceptions
- Add more properties and fixes - update fields and time calculations based on app expectations
- Add smtp to docker-compose.yml - Also catch all exceptions from action execution, so other actions can proceed.
Finalise realtime inference
…usly - Make delay configurable
- This is to prevent duplicate notifications
@mpgxvii, is it possible for you to review this when you have some free bandwidth? I think you plan to use this in the near future, so might be good to review and familiarise, unless you plan to use something else? |
Realtime Consumers are consumers for the results from the invocation of a model from the realtime analysis pipeline (see model-builder, model-invocation-endpointand ksqldb). The results from the pipeline are in JSON and the invocation is called by a KSQL function which gets inference results then puts the results in a kafka topic. These consumers are meant to consume data from this topic.
3 terms used -
Action
: Any task to be performed is an action. This can be sending a notification or an emailor just logging something.
Condition
: This is a predicate that will be evaluated on the incoming data. For instance, thiscan be used to evaluate if the ML model run crossed a threshold of some sort.
RealtimeInferenceConsumer
: This is the base consumer for realtime results topics. Each consumercan be configured with a set of Conditions and Actions. If all the conditions evaluate to be
true, then all the actions are triggered.
Currently, only one Condition is provided which evaluates a JSONPath expression. This should be sufficient for most simple use cases.
More complex use cases can create concrete implementations of the
Condition
interface or
ConditionBase
abstract class. Currently, the JsonPath expression is read from theconfiguration file and hence is static for a particular consumer and topic. Later, we can also
provide this through AppConfig so it can be dynamic based on userId or projectId.
The supported config (for instance an intervention using aRMT app) is of the format -
Note: The
properties
section is specific to each Action and Condition. Please take a look at thecondition and action docs for the keys supported. If the
projects
orsubjects
key is notspecified the action and condition will be used on all projects or subjects respectively.