Caesar Reductions work for more Drawing Tool Types

[kieftrav]

Package

lib-classifier

Linked Issue and/or Talk Post

#6038

Describe your changes

Refactor the FreehandLineReductions code pipeline to handle more ToolTypes

How to Review

Load up the classifier locally with the kieftrav/drawing-tools project running this branch. Test each workflow and ensure that a mark is loaded for each of the workflows.

General

• Tests are passing locally and on Github
• Documentation is up to date and changelog has been updated if appropriate
• You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
• FEM works in all major desktop browsers: Firefox, Chrome, Edge, Safari (Use Browserstack account as needed)
• FEM works in a mobile browser

[coveralls]

coverage: 81.855% (+0.02%) from 81.835%
when pulling 691d332 on caesar-drawing-tasks-refactor
into b1121e6 on master.

[shaunanoordin]

PR Review

Package: lib-classifier

This PR updates the Classifier, so it can display pre-computed results (from Caesar) for more Drawing Task tool types. Prior to this PR, only pre-computed results for the Freehand line tool type are available.

Note: unlike "aggregated results" for Transcription tasks, the machine learnt pre-computed results appear as pre-made marks on the Subject which the user can proceed to edit. (i.e. they're not read-only)

Code read looks straightforward:

• "freehand line reductions" have now been renamed to "machine learnt reductions"
• Functionally, the biggest change is in useMachineLearntReductions() (née useFreehandLinereductions()) and Workflow Store's usesMachineLearnt() (née usesFreehandLine()), the latter which now lists the type of acceptable drawing tools in its toolType const.

Testing

Tests will be done with macOS + Chrome 123, on localhost using app-project

Basic test: the additions of this PR should NOT cause any functional changes to "normal" drawing tasks (i.e. drawing tasks that aren't set up to talk to Caesar)

• Test URL: https://local.zooniverse.org:3000/projects/darkeshard/test-project-2022/classify/workflow/3617 (Shaun's test workflow with 1 drawing task and multiple drawing tools, including standard point and rectangle tools)
• I should see the Subject, but NOT see any pre-computed marks (because there aren't any)
• I should be able to make new marks
• I should be able to edit and delete marks
• I should be able to submit Classifications

Main test:

• Test URLs:
  • https://local.zooniverse.org:3000/projects/kieftrav/drawing-tools/classify/workflow/3805 (Travis's test workflow with a Freehand line tool, with previous pre-computed results already set up)
  • https://local.zooniverse.org:3000/projects/kieftrav/drawing-tools/classify/workflow/3789 (...Point tool)
• I should see the Subject, AND see any pre-computed marks (expect at least one)
• I should be able to make new marks
• I should be able to edit and delete new marks AND/OR pre-computed marks
• I should be able to submit Classifications

Status

Testing in progress.

@kieftrav: if it's no trouble, can you please add a small line of context to this PR, so we have a historical record of why this is added, and where/how it's expected to be used? e.g. "Project XYZ (link) used Caesar for pre-computing freehand line marks successfully, and now we want to expand it to more tools, and expect to use it on (link)"

[shaunanoordin]

PR Review (Update)

Tests are complete, and they look good. 👍

• Basic Tests ✔️
  • I had to re-run these tests a few times since staging WF 3617 turned out to be misconfigured for FEM. Whoops! Staging WF3617 is now a 3-step workflow; 1st step: question task, branching to either drawing task (red), drawing ask (blue), or nothing. 2nd step: each drawing task has multiple different tools.
• Main Tests ✔️

Dev Notes

Observation:

• on the test "standard" drawing workflow, the classifier works fine BUT there's a lot of "error" noise in the dev console:

  useCaesarReductions.js:27 Error: GraphQL Error (Code: 404): {"response":{"error":"","status":404,"headers":{"map":{"cache-control":"no-cache","content-type":"text/html"}}},"request":{"query":"{ workflow(id: 3617) { subject_reductions(subjectId: 161661, reducerKey:\"machineLearnt\") { data } } }"}}
  

• This is somewhat expected, as the test "standard" drawing wouldn't have any reduction data on Caesar.
• Consideration 1: I think suppressing the error messages at useCaesarReductions() would be nice. A "status: 404" from Caesar simply indicates there's no data, and not indicative of something broken.
• Consideration 2: is there a better way to define whether a workflow has Caesar reductions, asides from just pinging Caesar and waiting for a 404? (This is a very large can of worms though)
• Consideration 3: considering how many more workflows use non-freehand line drawing tools, will this this PR cause a large increase of pings to Caesar? e.g. Penguin Watch's users might start inadvertently spamming Caesar.

(The considerations above are NOT blocking for this PR, but definitely worth discussing on our next call.)

Status

LGTM 👍

I think there's some improvement that can be had by...

1. adding a safety check to task.activeTool.createMark, and
2. adding some try-catches in useCaesarReductions() to reduce the amount of noise in the console log

...but that second one is definitely a separate small PR.

[eatyourgreens]

This whitelist kind of breaks the idea that plugins/drawingTools should be a self-contained package, and the list itself needs to be maintained by hand whenever new drawing tools are added.

Maybe delegate responsibility to individual Tool objects? Eg. make each tool responsible for its own Caesar reducer.

const taskUsesMachineLearning = task.tools.some(tool => tool.caesarReducer === 'machineLearnt')

[kieftrav]

I see what you're saying.. I like that approach, yet I'd want to have a broader team discussion about more robust Caesar integration. Especially considering Shaun's comments above about knowing if/when to make a Caesar request.

[eatyourgreens]

True. You might also be able to reflect on the task type with getType(task), since TranscriptionTask and DrawingTask are different types, with different reducers. I think that, right now, the task models only allow one reducer per task.

[eatyourgreens]

So something like either a Facade or a Strategy pattern, where you have a generic Caesar client and tools or tasks generate specific strategies for generating client queries.

[goplayoutside3]

@kieftrav commenting here to document what we chatted about yesterday. I think Shaun's point about unnecessarily pinging Caesar for every subject just because a workflow uses a drawing task is something to be concerned about. Agreed it's sort of a can of worms to consider the best way to detect if a workflow requires fetching reductions, the request for this feature means now is the time to consider it. I recommend leaving this PR open while you consider how not to spam Caesar for non-reductions workflows, and if needed, open a PR asking to merge into this one.

I'm curious to hear from others already involved in this Github discussion if handling detection of a reductions-enabled workflow (what's the correct term for this?) is something to be done on the frontend. For example, grab the first subject, ping caesar, assume all subjects in a workflow do or do not have reductions based on that first subject, and then set a property in the MST that says 'hey this workflow should fetch reductions whenever a new subject loads'. Or if this discussion should involve the backend team, etc..

[eatyourgreens]

At the moment, you control Caesar reductions by passing a reducer key to a hook. So useCaesarReductions(‘alice’) will fetch ALICE reductions, but useCaesarReductions() won’t make an API request.

So one way to avoid spamming the API is to make sure the reducer key is only defined for workflows that use Caesar.

Once the request is made, the reductions are passed to subject.setCaesarReductions({ reducer, reductions, subjectId, workflowId }). That method uses types.union to assign the appropriate reductions model for the current subject. I made heavy use of unions in the subject models, because unions make it very easy to write generic actions that delegate to specific models, depending on context. I definitely recommend learning how to use types.union to get quite powerful functionality from MST.

front-end-monorepo/packages/lib-classifier/src/store/subjects/Subject/Subject.js

Lines 137 to 147 in 48b1117

	function setCaesarReductions({ reducer, reductions, subjectId, workflowId }) {
	if (reducer) {
	self.caesarReductions = CaesarReductions.create({
	loadingState: asyncStates.success,
	reducer,
	reductions,
	subjectId,
	workflowId
	})
	}
	}

front-end-monorepo/packages/lib-classifier/src/store/subjects/Subject/Subject.js

Line 10 in 48b1117

const CaesarReductions = types.union(FreehandLineReductions, TranscriptionReductions)

[eatyourgreens]

@shaunanoordin pointed out to me that you could put the reducer key in workflow config, so that it’s only defined for workflows that use Caesar.

[kieftrav]

Since the conversations I've been having around this PR are getting more ADR-defining territory I’d like to clarify some assumptions, provide some potential solutions, and then wrap up with my own thoughts on how to approach the work. My biggest concern is unclarified assumptions about how this feature works in the largest sense (i.e. what is a fully robust implementation, its limitations, etc).

Known:

1. We currently have 2 reducers. alice and machineLearnt. The first is for transcription tasks and the second is for freehandLine tasks
2. If a workflow has either task type, it will ALWAYS ping caesar looking for reductions. This is fine if all workflows containing transcription or freehandLine tasks have reductions. I know this shouldn't be a 100% true assumption for freehandLine tasks but I lack a ton of context on transcription tasks. Maybe we always want to ping caesar for transcription tasks.
3. In order for a different reduction key to work (Alice vs machineLearnt vs X), there needs to be some code written into FEM that does the work that is useful for that reduction key. i.e. if we create an input field for a reductionKey, putting “johns-reducer” will needlessly ping caesar because we haven’t built anything to do with that reduction

And now for some context on specifically how the machineLearnt reducer works. The point of machineLearnt as I understand it is to enable loading marks from caesar for a given subject + step + task + tool + frame. i.e the uploaded data to caesar has to match the data structure of our mark object in order to render properly. It doesn’t do any modification to the data in any way - it simply creates a tight 1-to-1 binding to our internal data structure. As such, there is actually quite a bit of brittleness already with this approach.. Misspell something, put it on the wrong frame, forget to add the step, etc it likely won’t load and it’ll be unclear why to anyone but an internal developer.

Potential Solutions


1. Ping Caesar on the first N subjects of a workflow. If they’re all 404, disable pinging Caesar

Advantages: it’s the simplest solution and likely a couple days work all-in with discussion/testing/review/integration. It’s also “default on” which reduces the number of steps a researcher has to go through to use.

Disadvantages: Still pings caesar unnecessarily.

2. Create boolean on the workflow that enables fetching from caesar regardless of key

Advantages: Second simplest solution requiring changes to the Workflow config. Only pings Caesar if enabled.

Disadvantages: Requires a bit more development work that touches across frontend and potentially backend. If we decide to make it an admin toggle there’s more work there too.

3. Create “dropdown” on the workflow that has 3 options: No Caesar Integration, Alice, or MachineLearnt, with the default being No Caesar Integration.

Advantages: Third simplest solution requiring changes to the Workflow config. Only allows selecting reducers for which code is written. Only pings Caesar if enabled.

Disadvantages: Requires a bit more development work that touches across frontend and backend (my assumption is the dropdown value that gets saved is another column in the db but I don’t know that for sure). If we decide to make it an admin toggle there’s more work there too.

4. Create an “input field” where the researcher can configure the reduction key.

Advantages: Allows the researcher to customize the tie between their data in caesar and the workflow.

Disadvantage: If the reduction key isn’t exactly alice or machineLearnt nothing will be done with the reduction on the frontend.

Final Thoughts

I think in the short term we should implement 1 and 2. If we only do 1, we should have greater clarity on the number of requests to make if there’s an assumption that not every workflow subject will have a corresponding entry in caesar. As it stands, my assumption with the “correct a machine” projects is that there isn’t a point to showing a subject without a corresponding caesar reduction… If that’s a fair assumption, N can be 1.

If we implement 2, I think it’s a good stepping stone towards a broader integration of Caesar within FEM. The dev lift is relatively light, and paired with solution 1, will minimize spamming Caesar. It also keeps the feature relatively minor from a UI-visibility perspective and I don’t think will require too much detail / context explanation.

If we implement 3, I think it makes sense to plan for greater work around documentation, specifically on how to use the expanded machineLearnt workflow to help researchers feel confident that the data uploaded to Caesar matches the underlying data structure of their workflow (i.e. the right step + task + tool + frame details). I have mock data I used for developing the expanded feature, but I’m pretty concerned at the already-brittle connection with this feature.

If we want to implement 4, it isn’t clear to me how this would be helpful without intermediate decoupling steps.. I.E. my assumption around a customized reduction key is to somehow tie into some code that we’ve already written in the frontend without using the key as that decider. If, instead, it’s an indirection meant to customize which reductions are pulled from Caesar (i.e. similar workflows with different reduction data based on that reduction key), then we’ll have to make the frontend-end decouple from the reduction key and focus instead on the tool types + the presence of a reduction key in order to both ping Caesar and run that corresponding tool’s current reduction code.. This does maintain a tight coupling between the tool types and which current frontend reduction data gets run. At least this is my best current understanding of the intention of this feature - appreciate any clarification if I’m missing something!

All that said, if our ultimate goal is 4, then I’d like to have a much deeper conversation about the ideal future caesar-fem integration and how we’re managing the incidental complexity (i.e. brittleness).

P.S. Pinging @lcjohnso for visibility!

[lcjohnso]

I'm in favor of implementing Solution 2 = add boolean key-value pair in workflow.configuration that must be set for requests to be issued out to Caesar.

This eliminates the problem of any Caesar requests running for workflows where that is not relevant (i.e., counting and disabling after N 404 errors still produces higher Caesar request volume, and could lead to unintended consequences in the case that Caesar becomes unreachable for short period of time). Also: any key-value pair can be written to workflow.configuration, so no backend effort should be necessary. But I agree there are a few pieces to the frontend effort: defining and implementing a workflow.configuration key check, and (optionally) adding a form field to the workflow section of the project admin page for convenience (i.e., making it easy to configure this option, rather than forcing teams / ZooAdmins to use the Python Client -- similar to "Configure Training Data" section).

re: user-defined reduction keys -- I'll defer this to offline conversation, as I think there's some boring symantec details at play in this discussion, but suffice to say -- it is totally OK for the configuration to impose specific Caesar reducer keys (i.e., alice and machineLearnt), especially when matched to specific FEM data models or reduction code. However, under the assumption that the input Caesar reduction will typically (if not always) pertain to MachineLearntReductions containing drawing task annotations, except in case of transcription tasks / alice reductions, I'm simply pointing out that you could be more flexible in Caesar reducer naming convention.

[eatyourgreens]

If the reduction key isn’t alice or machineLearnt, the following lines will error with ‘no matching snapshot for types.union’ so you could use that to detect whether a key is supported. Maybe create an empty reduction before making a request, then make the request and fill out the reduction with the response. If the initial create fails, reductions aren’t supported.

front-end-monorepo/packages/lib-classifier/src/hooks/useCaesarReductions.js

Lines 46 to 51 in 8e57e77

	subject.setCaesarReductions({
	reducer: reducerKey,
	subjectId: subject.id,
	workflowId: workflowID,
	reductions
	})

.

[eatyourgreens]

If I can offer a comment on the organisation and design of the code, I'd recommend moving any code that's specific to drawing task reductions into the reductions model (currently called FreehandLineReductions.)

If you look at the TranscriptionReductions model, it contains all the code needed to parse Caesar reductions for transcriptions and get reduced lines by frame etc. Doing the same for drawing tasks would help make the code more manageable, easier to test and easier to debug.

NB. Reductions never change, once loaded, so that's why they are frozen and have no actions.