Conversation
Quentinchampenois
force-pushed
the
feat/third_party_service
branch
from
c57f7c9 to
93f74f0
Compare
alecslupu
left a comment
alecslupu
left a comment
There was a problem hiding this comment.
Solid job overall ( haven't played with it, but is beautiful reading it)
No changes to be made, just various comments, that you may find useful.
| class GenericSpamAnalyzerJob < Decidim::Ai::SpamDetection::GenericSpamAnalyzerJob | ||
| def perform(reportable, author, locale, fields) | ||
| @author = author | ||
| organization_host = reportable.organization.host |
There was a problem hiding this comment.
Actually this may be needed in decidim, to fully support multi tenant hosts.
| def perform(reportable, author, locale, fields) | ||
| @author = author | ||
| organization_host = reportable.organization.host | ||
| klass = reportable.class.to_s |
There was a problem hiding this comment.
I am not sure that we would really need the spam class as input data. I think a spam message is spam regardless of the place is posted. But nevertheless, i would like to find out why you need it.
| return unless overall_score >= Decidim::Ai::SpamDetection.resource_score_threshold | ||
|
|
||
| Decidim::CreateReport.call(form, reportable) | ||
| rescue StandardError => e |
There was a problem hiding this comment.
As i was saying, as we want to make sure the the retry works on sidekiq, is not recommended to catch "StandardError".
| @author = reportable | ||
| organization_host = reportable.organization.host | ||
| klass = reportable.class.to_s | ||
| classifier.classify(reportable.about, organization_host, klass) |
There was a problem hiding this comment.
Makes sense to have the organization host.
| # Configuring Third Party jobs | ||
| Decidim::Ai::SpamDetection.user_spam_analyzer_job = "Decidim::Ai::SpamDetection::ThirdParty::UserSpamAnalyzerJob" | ||
| Decidim::Ai::SpamDetection.generic_spam_analyzer_job = "Decidim::Ai::SpamDetection::ThirdParty::GenericSpamAnalyzerJob" |
There was a problem hiding this comment.
Maybe we do not need to register a custom analyzer after the changes are added to main repo.
| autoload :Base, "decidim/ai/spam_detection/strategy/base" | ||
| autoload :Bayes, "decidim/ai/spam_detection/strategy/bayes" | ||
| autoload :ThirdParty, "decidim/ai/spam_detection/strategy/third_party" | ||
| autoload :Scaleway, "decidim/ai/spam_detection/strategy/scaleway" |
There was a problem hiding this comment.
Maybe we could add the required services as a gem, in a "Decidim::Ai::SpamDetection::Scaleway" namespace.
| private | ||
|
|
||
| def system_log(message, level: :info) | ||
| Rails.logger.send(level, message) | ||
| end |
There was a problem hiding this comment.
This is a change that should be in decidim (I already see the change is present in Bayes strategy).
| module Strategy | ||
| # Scaleway third-party strategy | ||
| # doc: https://www.scaleway.com/en/docs/managed-inference/quickstart/ | ||
| class Scaleway < ThirdParty |
There was a problem hiding this comment.
This could become: "Decidim::Ai::SpamDetection::Scaleway::Strategy".
| module Ai | ||
| module SpamDetection | ||
| module Strategy | ||
| class ThirdParty < Base |
There was a problem hiding this comment.
This could become: "Decidim::Ai::SpamDetection::ThirdParty::Strategy".
| @@ -0,0 +1,22 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| module Decidim | |||
There was a problem hiding this comment.
If the host changes are added to the main repo, this class is not needed anymore.
2 participants
Description
Delegates classification logic to a third party AI system
Note
This PR contains a Scaleway ThirdParty strategy which can be used with a custom AI FaaS