-
Notifications
You must be signed in to change notification settings - Fork 2
[WIP] New Link Creation Agent processors #310
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
base: master
Are you sure you want to change the base?
Conversation
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.
See comments in the dev channel
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.
There are two things in this new design that I think new to be changed.
(1) Processor are designed to deal with 1 single QueryAnswer. This is bad because we'd need to instantiate one processor for each QueryAnswer and this is a totally unnecessary overhead. And a pointless one, in my opinion, since it doesn't make the design simpler. Processors don't keep any data (or they shouldn't) so the best design here is to instantiate them only once in agent's constructor and use them to process QueryAnswers of multiple requests.
(2) Creation and returning of the created links are split into three methods making the whole process asynchronous. This introduce unnecessary overhead and complexity. I believe the need to tho this is because of the point (1) above. So I think that solving (1) will make it easier to solve (2).
NOTE: to address (1) and (2) we'd need to change the method signature to something like:
virtual virtual std::vector<std::vector<std::string>> process(QueryAnswer* query_answer);
Which kind of merge the functionality of current constructor, process() and get_links().
src/link_creation_agent/service.cc
Outdated
vector<vector<string>> link_tokens; | ||
if (LinkCreationProcessor::get_processor_type(link_template.front()) == | ||
ProcessorType::PROOF_OF_IMPLICATION) { | ||
link_tokens = implication_processor.process(query_answer, link_template); | ||
}else if (LinkCreationProcessor::get_processor_type(link_template.front()) == | ||
ProcessorType::PROOF_OF_EQUIVALENCE) { | ||
link_tokens = equivalence_processor.process(query_answer, link_template); | ||
}else { | ||
link_tokens = link_template_processor.process(query_answer, link_template); | ||
} | ||
this->create_link(link_tokens, *das_client); |
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.
vector<vector<string>> link_tokens;
auto processor_type = \
LinkCreationProcessor::get_processor_type(link_template.front());
if (processor_type == ProcessorType::PROOF_OF_IMPLICATION) {
link_tokens = \
implication_processor.process(query_answer, link_template);
} else if (processor_type == ProcessorType::PROOF_OF_EQUIVALENCE) {
link_tokens = \
equivalence_processor.process(query_answer, link_template);
} else {
link_tokens = \
link_template_processor.process(query_answer, link_template);
}
this->create_link(link_tokens, *das_client);
std::vector<std::vector<std::string>> LinkTemplateProcessor::process( | ||
QueryAnswer* query_answer, std::optional<std::vector<std::string>> config) { | ||
std::vector<shared_ptr<Link>> links; | ||
if(config != std::nullopt){ |
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.
It's an optional
but cannot be nullopt
?
if(config != std::nullopt){ | ||
if (config.value().front() == "LIST") { | ||
LinkCreateTemplateList link_create_template_list(config.value()); | ||
for (auto link_template : link_create_template_list.get_templates()) { | ||
shared_ptr<Link> link = process_template_request(query_answer, link_template); | ||
links.push_back(link); | ||
} | ||
} else { | ||
LinkCreateTemplate link_template(config.value()); | ||
shared_ptr<Link> link = process_template_request(query_answer, link_template); | ||
links.push_back(link); | ||
} | ||
} else{ | ||
throw runtime_error("Invalid link template"); | ||
} |
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 (config == std::nullopt) {
throw runtime_error("Invalid link template");
}
if (config.value().front() == "LIST") {
LinkCreateTemplateList link_create_template_list(config.value());
for (auto link_template : link_create_template_list.get_templates()) {
auto link = \
process_template_request(query_answer, link_template);
links.push_back(link);
}
} else {
LinkCreateTemplate link_template(config.value());
auto link = \
process_template_request(query_answer, link_template);
links.push_back(link);
}
Please, check the functions params for const-ness and referencing. Also, if possible, add |
Missing processor's implementation;