oppia / oppia-android

A free, online & offline learning platform to make quality education accessible for all.
https://www.oppia.org
Apache License 2.0
309 stars 509 forks source link

Classifier failures result in a broken experience in-exploration #2973

Open BenHenning opened 3 years ago

BenHenning commented 3 years ago

Is your feature request related to a problem? Please describe. We need to have error handling for when something breaks in the classifier flow. For example, if a classifier type mismatches a failure is logged in the console and the submit button gets stuck, leaving the user unable to proceed. We should let the user know something is broken, and provide them some path of remediation (though they may not be able to progress through the exploration).

Describe the solution you'd like I'm thinking that a dialog indicating something went wrong would be helpful, and not freezing the submit button so that they can try a different answer. They always have the choice to exit the exploration & try again.

Describe alternatives you've considered We could alternatively silently fail, but that doesn't help the user much since they don't receive any feedback for their answer which would result in a broken experience.

Additional context To repro this locally, you might need to intentionally break the classifier flow (e.g. by submitting a different type of UserAnswer for a particular interaction).

BenHenning commented 3 months ago

@seanlip I think we'd benefit from having some product input on the ideal user experience here. For context, the error case we're trying handle is when something fails internally within the answer classifier framework and we can't recover. Currently, the the submit button freezes and the user isn't able to progress, but we can at least show an error and ensure the user is still able to try other answers.

Given that this is covering a category of possible failures, what sort of error messaging do you think would be reasonably informative to the user on what's going on and what they can try next? We're also not sure what form this error messaging should take (e.g. a modal, in-line error, or something else). Do you have thoughts or opinions?

seanlip commented 3 months ago

Can you give an example on how such a failure can occur? If the learner repeats the action, will it still fail?

adhiamboperes commented 3 months ago

@seanlip, we encountered this during the 0.11 pre-release testing e.g. https://github.com/oppia/oppia-android/issues/5050

seanlip commented 3 months ago

Just a note, I had a chat with @BenHenning about this, and we decided that the right thing to do here was to crash in a way that was developer-visible. Automatic retry is not going to be helpful if the action will keep failing, and picking a random answer group isn't great either since it gives the learner wrong feedback. In cases where we become aware of this, we should prioritize fixing the actual bug and releasing a patch quickly (which should hopefully be easier as we get automatic releases set up).

/cc @BenHenning in case he'd like to add/correct anything above.

BenHenning commented 3 months ago

+1 to Sean's comment and I think this issue now has these specific outcomes:

Please follow up if any additional clarity is needed @masclot, hopefully this helps.

masclot commented 3 months ago

The classifier is already throwing an unchecked exception here: AnswerClassificationController.kt#L80

Example exception:

    java.lang.IllegalStateException: Failed when classifying answer # org.oppia.android.app.model.InteractionObject@8bb4a
    bool_value: true for interaction FractionInput
        at org.oppia.android.domain.classify.AnswerClassificationController.classifyAnswer(AnswerClassificationController.kt:81)
        at org.oppia.android.domain.classify.AnswerClassificationController.classify(AnswerClassificationController.kt:39)
        at org.oppia.android.domain.exploration.ExplorationProgressController$submitAnswerImpl$2.invokeSuspend(ExplorationProgressController.kt:623)
        at org.oppia.android.domain.exploration.ExplorationProgressController$submitAnswerImpl$2.invoke(Unknown Source:10)
        at org.oppia.android.domain.exploration.ExplorationProgressController.tryOperation(ExplorationProgressController.kt:793)
        at org.oppia.android.domain.exploration.ExplorationProgressController.tryOperation$default(ExplorationProgressController.kt:789)
        at org.oppia.android.domain.exploration.ExplorationProgressController.submitAnswerImpl(ExplorationProgressController.kt:604)
        at org.oppia.android.domain.exploration.ExplorationProgressController$createControllerCommandActor$1.invokeSuspend(ExplorationProgressController.kt:478)
        at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
        at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1137)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:637)
        at java.lang.Thread.run(Thread.java:1012)

I don't think anybody is catching it. The reason the app does not crash is because the exception happens in a background thread, instead of the main thread. Would you rather have a new, more specific exception, or IllegalStateException is fine?

masclot commented 3 months ago

The actual check, and original exception, is here: GenericRuleClassifier.kt#L28