-
Notifications
You must be signed in to change notification settings - Fork 30
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
Caesar Reductions work for more Drawing Tool Types #6039
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.
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éeuseFreehandLinereductions()
) and Workflow Store'susesMachineLearnt()
(néeusesFreehandLine()
), the latter which now lists the type of acceptable drawing tools in itstoolType
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)"
packages/lib-classifier/src/hooks/useMachineLearntReductions.js
Outdated
Show resolved
Hide resolved
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.
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...
- adding a safety check to
task.activeTool.createMark
, and - 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.
packages/lib-classifier/src/store/WorkflowStore/Workflow/Workflow.js
Outdated
Show resolved
Hide resolved
...tViewer/components/InteractionLayer/components/DrawingToolMarks/DrawingToolMarksConnector.js
Outdated
Show resolved
Hide resolved
@@ -81,11 +81,13 @@ const Workflow = types | |||
return anyTranscriptionTasks | |||
}, | |||
|
|||
get usesFreehandLineTool() { | |||
get usesMachineLearnt() { | |||
const toolTypes = ['circle', 'ellipse', 'freehandLine', 'line', 'point', 'polygon', 'rectangle', 'rotateRectangle']; |
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 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')
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 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.
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.
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.
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.
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.
…le for the following drawing toolTypes: circle, ellipse, freehandLine, line, point, polygon, rectangle, rotateRectangle.
c04631e
to
691d332
Compare
@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.. |
At the moment, you control Caesar reductions by passing a reducer key to a hook. So 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 front-end-monorepo/packages/lib-classifier/src/store/subjects/Subject/Subject.js Lines 137 to 147 in 48b1117
|
@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. |
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:
And now for some context on specifically how the Potential Solutions
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.
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.
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.
Advantages: Allows the researcher to customize the tie between their data in caesar and the workflow. Disadvantage: If the reduction key isn’t exactly Final ThoughtsI 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 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! |
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., |
If the reduction key isn’t front-end-monorepo/packages/lib-classifier/src/hooks/useCaesarReductions.js Lines 46 to 51 in 8e57e77
|
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 If you look at the NB. Reductions never change, once loaded, so that's why they are frozen and have no actions. |
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
yarn panic && yarn bootstrap
ordocker-compose up --build
and FEM works as expected