Skip to content
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

Classifier: fix duplicate subjects (with ordered subject selection.) #2819

Merged
merged 5 commits into from
Feb 16, 2022

Conversation

eatyourgreens
Copy link
Contributor

In #2392 I added subjects.queue, an array of subject IDs that allows for appending or prepending subjects to the subject queue, and also allows for navigating both forward and backwards through the queue.

I didn't update the tests to test the new array. As a result, it's possible to append duplicate subjects to the queue without any tests failing.

This PR updates the tests to test subjects.queue explicitly, adds a test that appends duplicate subjects and updates the subjects store to pass the new tests.

Package:
lib-classifier

Closes #2818.

Review Checklist

General

  • Are the tests passing locally and on Travis?
  • Is the documentation up to date?

Components

Apps

  • Does it work in all major browsers: Firefox, Chrome, Edge, Safari?
  • Does it work on mobile?
  • Can you yarn panic && yarn bootstrap or docker-compose up --build and app works as expected?

Publishing

  • Is the changelog updated?
  • Are the dependencies updated for apps and libraries that are using the newly published library?

Post-merging

Test subjects.queue after modifying the subject queue, and also after appending duplicate subjects that are already in the queue.
@eatyourgreens eatyourgreens added the bug Something isn't working label Feb 11, 2022
@eatyourgreens eatyourgreens requested a review from a team February 11, 2022 10:43
@goplayoutside3 goplayoutside3 self-assigned this Feb 14, 2022
Copy link
Contributor

@goplayoutside3 goplayoutside3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review

While the changes to specs make sense, I'm still able to replicate the duplicate subject bug as described in #2818 with the same behavior as described here: https://www.zooniverse.org/projects/msalmon/hms-nhs-the-nautical-health-service/talk/4936/2331577?comment=3847847

To test this PR, I ran HMS NHS locally in demo mode, made 10 classifications in the 1891 'Where From' subject set starting from subject 4/276. After clicking 'Done' on subject 13/276, I was shown subject 4/276 with an Already Seen banner. Refreshing the page did not serve a new subject and neither did navigating to the homepage and back to the same workflow.

@eatyourgreens
Copy link
Contributor Author

In demo mode, Designator is always going to show you subject 4, because you haven't submitted a classification for it.

I'm wondering if there's a way to test this without submitting real classifications on a live project.

@snblickhan has a test project with ordered subjects.

@eatyourgreens
Copy link
Contributor Author

@goplayoutside3
Copy link
Contributor

Here you go:

https://frontend.preview.zooniverse.org/projects/blicksam/fem-bug-squashing

This test project does not have ordered subject selection banners like HMS NHS. It looks like I can make more than 10 real classifications to the test project without seeing a duplicate subject, but I have no way of knowing for sure without the SubjectSetProgressBanner. Is this test workflow same flow of data as an engaging crowds project?

@eatyourgreens
Copy link
Contributor Author

That workflow does have sequential subject selection (workflow.prioritized) enabled but not subject set selection (workflow.grouped.)

I wonder if we need to enable both those flags in order to reproduce this bug?

I've classified around 35 subjects on Sam's test project, on staging, without seeing a repeat.

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Feb 15, 2022

I think @snblickhan already pointed this out on the Talk thread, but refreshing the page will only give you a new subject when a workflow uses random selection. With prioritised selection, Designator will always send you the first ordered subject that you haven't classified.

So seeing the same subject when you refresh the page is expected. You'll only get a new subject when your first unclassified subject retires, or when Panoptes registers the current subject as seen by you.

@goplayoutside3
Copy link
Contributor

Good to hear that duplicate subjects don't appear on Sam's test project, and I think I see the same.

Because this bug fix is specific to a Zooniverse project with subject set selection, I think it's important to confirm we don't see duplicate subjects with that flag enabled. Is there a staging project with engaging crowd features used to test for bugs? If not, I can make real classifications on HMS NHS's easier workflows, but I think being able to quickly test a staging project that's a replica of HMS NHS workflows should be a requirement for reviewing its bug fixes going forward.

@eatyourgreens
Copy link
Contributor Author

@snblickhan can you help out Delilah with this?

@eatyourgreens
Copy link
Contributor Author

By the way, I'm currently working on something that will take the pain out of setting up grouped and prioritised workflows for testing etc.
zooniverse/Panoptes-Front-End#6095

@snblickhan
Copy link

@snblickhan can you help out Delilah with this?

Sure thing. I'll create a new workflow with both of these admin flags enabled and we'll see whether that triggers the bug.

@snblickhan
Copy link

OK @eatyourgreens & @goplayoutside3 -- have created new workflow #20917 which has 2 subject sets and includes prioritized subject delivery and set selection. I'll do some test classifications now to see if the bug shows up.

@snblickhan
Copy link

Immediately triggered it -- using subject set 102314, got to 10 classifications in and was immediately sent back to #8/50 with an Already Seen flag.

@goplayoutside3
Copy link
Contributor

Thanks @snblickhan! I ran the project locally with this branch to try out workflow 20917.

While signed out in an incognito browser window:

  • This bug still persists

While signed in:

  • I can classify more than 10 subjects without being sent back to an already seen subject

  • I also tested navigating between subject sets. I was on subject 22/50 of the DNP subject set, went to the BPL subject set, and then back to the DNP subject set and was shown subject 31/50. Is this non-sequential subject behavior expected?

  • I then classified every subject through the set, until the number looped back around from 50/50 to 23/50. From there, I was occasionally shown an Already Seen banner for subjects I don't believe I had classified yet like 27/30. Once I hit 27/30, the next subject shown to me was 30/50.

Testing a different user account

  • Signed into a different user account in a new browser window, I went to the BPL subject set, classified subject 1/16, navigated to the DNP subject set then back to the BPL subject set and I was shown subject 1/16 with an Already Seen banner

  • @eatyourgreens since you mentioned that Designator will always send you the first ordered subject that you haven't classified, I'm documenting the subject order I saw in Sam's project in case this behavior sounds like a larger bug than the 10-classifications one.

@eatyourgreens
Copy link
Contributor Author

We can't track your classification history if you aren't logged in. @snblickhan am I right in saying that we don't expect these prioritised workflows to work for anonymous volunteers?

@eatyourgreens
Copy link
Contributor Author

I also tested navigating between subject sets. I was on subject 22/50 of the DNP subject set, went to the BPL subject set, and then back to the DNP subject set and was shown subject 31/50. Is this non-sequential subject behavior expected?

That’s weird. Do you mind opening an issue for this? It could be a bug in the classifier, preserving the wrong subject across workflows, or a bug in the interaction with Designator. Changing workflow should reset the queue and request a new batch of ten subjects from Designator, starting from the last one that you classified but skipping any subjects that have been retired by other volunteers. I don’t expect the latter to be relevant here, because you’re the only person who has classified on the workflow.

@eatyourgreens
Copy link
Contributor Author

I’m hoping that the extra console logs in #2793 will help to debug changes in the active subject when you switch workflow.

@snblickhan
Copy link

After reading @goplayoutside3's reply, I just tried the same thing (incognito window, not signed in) for workflow 20917 and it triggered the bug after 10 classifications on the DNP set (expected subject 11/50, it instead sent me all the way back to image 1/50 with an Already Seen banner).

@goplayoutside3
Copy link
Contributor

After reading @goplayoutside3's reply, I just tried the same thing (incognito window, not signed in) for workflow 20917 and it triggered the bug after 10 classifications on the DNP set (expected subject 11/50, it instead sent me all the way back to image 1/50 with an Already Seen banner).

I think that is expected if you're testing at https://frontend.preview.zooniverse.org/projects/blicksam/fem-bug-squashing/classify/workflow/20917 because frontend.preview doesn't include this PR's bug fix.

Do we expect prioritised workflows to work for anonymous volunteers who are not signed in? This PR does allow signed-in users to move past 10 classifications, and it could be approved if we're going to track the other unexpected non-sequential behavior I mentioned above in a different issue.

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Feb 15, 2022

Do we expect prioritised workflows to work for anonymous volunteers who are not signed in?

If you aren't logged in, won't you get the first ten subjects on a loop, at least until they retire? Sequential selection relies on you being logged in, so that Panoptes can track which subjects you've seen.

EDIT: @snblickhan how are projects like Davy Notebooks, which use sequential subjects, handling anonymous volunteers? They will all be getting the same ten subjects from Designator, which will be the first ten unretired pages.

For Operation War Diary, we showed subjects in order to volunteers who were logged in, but served random pages to anonymous volunteers. I don't know if Panoptes/Designator would support that, but it's one possible strategy for spreading anonymous effort evenly across subjects.

I think this conversation is straying away from the original bug and into discussing how the backend should work for sequential subject selection.

@eatyourgreens
Copy link
Contributor Author

There's a selection bias in the bug reports, in that they're coming from Talk and anonymous volunteers can't comment on Talk.

@eatyourgreens
Copy link
Contributor Author

Reading back, it seems there's a few different things going on here, particularly if you change workflows on a project like HMS NHS.

This PR is specifically focussed on fixing #2818: when you're logged in, the last three subjects in every batch of ten, from the API, are repeated in your queue because the minimum queue size is three subjects.

I'd like to get that fix merged and deployed. It seems like the scope of the issue is starting to creep to include other bugs in subject selection.

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Feb 16, 2022

As part of the IIIF work (zooniverse/Panoptes-Front-End#6095), I've set up a grouped, prioritised workflow with two volumes from the British Library. I'm not seeing the duplicate subject bug on these subject sets, which is odd. I'd at least expect the last two subjects to repeat in each batch of ten from the API.
https://frontend.preview.zooniverse.org/projects/eatyourgreens/-project-testing-ground/classify/workflow/3586?env=staging&demo=true

@eatyourgreens
Copy link
Contributor Author

I've just found out from @camallen, on Slack, that Designator has a five minute cache, during which time it will always send you new subject IDs.

So, to reproduce this bug, you have to wait at least 5 minutes between requests for fresh subjects from Panoptes. That explains why we didn't see it on Sam's quick Yes/No workflow, but volunteers have seen it on HMS NHS.

That cache also explains why we're seeing the first subject change if you leave a workflow, or set, then come back to it within 5 minutes.

@snblickhan
Copy link

So, to reproduce this bug, you have to wait at least 5 minutes between requests for fresh subjects from Panoptes. That explains why we didn't see it on Sam's quick Yes/No workflow, but volunteers have seen it on HMS NHS.

Super interesting. This is a good lesson for me to always create test workflows that match the projects where bugs are reported. I'd sell my soul for a Copy Workflow button that works across projects...

Copy link
Contributor

@goplayoutside3 goplayoutside3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading back, it seems there's a few different things going on here, particularly if you change workflows on a project like HMS NHS.

This PR is specifically focussed on fixing #2818: when you're logged in.

In that scope, this PR does fix the bug. @eatyourgreens I'll let you open an issue for the Designator-related subject sequences. If you'd like me to document the sequence of subjects I'm shown on projects/blicksam/fem-bug-squashing, please let me know!

@github-actions github-actions bot added the approved This PR is approved for merging label Feb 16, 2022
@eatyourgreens
Copy link
Contributor Author

Let's get this out and see if it fixes the problem for HMS NHS volunteers.

@eatyourgreens eatyourgreens merged commit 869e932 into master Feb 16, 2022
@eatyourgreens eatyourgreens deleted the classifier-duplicate-subjects branch February 16, 2022 16:51
@eatyourgreens
Copy link
Contributor Author

@goplayoutside3 I've opened an issue on Panoptes for the subject queue advancing when you leave a workflow and come back.
zooniverse/panoptes#3789

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This PR is approved for merging bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Engaging Crowds: duplicate pages being served for HMS NHS
3 participants