add extractor name to prediction request for new workflow #186
Conversation
There was a problem hiding this comment.
Some small fixes but I have a couple of questions:
-
Q: OOC, Will there ever be a case when there is more than 1 context with the same
active_subject_set_id? I think looking at the query for context on#run, there is an underlying assumption that there will not be a context with the same active_subject_set_id/pool_subject_set_id. Totally fine for now since the goal for right now is to get kade and bajor flow to work for 2 workflows, but something to think about as more workflows are added to use the kade bajor flow. cc @lcjohnso for input as well -
I would probably add a couple of more tests, especially for the long query on Context
- A test if there is no context coming from the query (i.e. Context.where yadda yadda is empty). Test what expected behavior. I think what ends up happening is workflow_name is nil and prediction job is submitted using cosmic_dawn
- A test where context is found by pool_subject_set_id
- A test where context is found by active_subject_set_id
- One case where there is only 1 relevant context
- Another case where there are 2 relevant contexts (1 context with the right active_subject_set_id and another with the right pool_subject_set_id) => it should pick the context with the right active_subject_set_id.
^^ These might be overkill given the goal is to get this working for 2 cases for now. I'll leave this up to your discretion @Tooyosi (And you can double check with @lcjohnso as well).
If you do decide to add these tests, you can do a check to see if the workflow_name that is being sent to bajor client is the expected workflow_name.
| @@ -14,7 +14,15 @@ def initialize(prediction_job, bajor_client = Bajor::Client.new) | |||
|
|
|||
| def run | |||
| begin | |||
| bajor_job_url = bajor_client.create_prediction_job(prediction_job.manifest_url) | |||
| subject_set_id = prediction_job.subject_set_id | |||
| context = Context.where(active_subject_set_id: subject_set_id).or(Context.where(pool_subject_set_id: subject_set_id)).order(Arel.sql("CASE WHEN active_subject_set_id = #{subject_set_id} THEN 0 ELSE 1 END")) | |||
There was a problem hiding this comment.
This query looks a little long, you could break into multiple lines for readability:
context = Context
.where(active_subject_set_id: subject_set_id)
.or(Context.where(pool_subject_set_id: subject_set_id))
.order(Arel.sql("CASE WHEN active_subject_set_id = ? THEN 0 ELSE 1 END", subject_set_id))
| workflow_name = nil | ||
| if context.first.present? | ||
| workflow_name = context.first.extractor_name | ||
| end |
There was a problem hiding this comment.
This probably could be simplified to something like:
workflow_name = context.first&.extractor_name
| @@ -5,11 +5,12 @@ | |||
| RSpec.describe Batch::Prediction::CreateJob do | |||
| describe '#run' do | |||
| let(:manifest_url) { 'https://manifest-host.zooniverse.org/manifest.csv' } | |||
| let(:context){Context.first} | |||
There was a problem hiding this comment.
should we declare fixtures?
eg.
describe '#run' do
fixtures :contexts
end
Final part of setup for new workflow usage. Pass in the workflow name for bajor predictions request