Fix/cross year second pass issues

[theokaufman]

Fixing some cross year issues that came up in QA:

• Second pass was not running when there were 0 matches on the first pass, because in that case the num_unmatched_students is set to None. I added a condition to account for this
• In cases where the second pass failed to match any students, an unmatched students error was being returned, even if the first pass had a 99% match rate. I added some logic to only enforce_match_threshold if the first pass DID NOT meet the threshold

LMK if anything seems amiss here

[amazon-inspector-ohio]

⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done

[snyk-io-us]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[amazon-inspector-ohio]

✅ I finished the code review, and didn't find any security or code quality issues.

[johncmerfeld]

Hang on, I actually want to think about this a little harder

[johncmerfeld]

What's your thinking vis a vis finally here? If Earthmover failed on the second run, then do we even want to reach this point? It should never be the case that EM failed and we return unmatched records to the user

If Earthmover succeeded on the second run, we shouldn't need finally for this to hit.

Very possible something I just said is incorrect. Which scenario are we in at this line?

[theokaufman]

Here's what I have come to think through some QA issues that popped up. On the second pass we have essentially 3 phases:

1. We check the first run's match rate. If it's above threshold we constrain to use the first run's id type. If it's below, we skip all that and just log a message saying we are rerunning against all ID types.
2. We run earthmover. This is what's in the try. This should definitely happen.
3. We do some cleanup, but again this is contingent on the first run's match rate. If the first run was successful but the second failed, surely we want to return the first run's records to the user. However, if neither run met threshold, we definitely don't want to return anything to the user. So, in the finally we only enforce_match_threshold IF the first run was below threshold.

That's my basic thinking. This came up in QA with a file that had 99% matchrate on the first pass, and 0% on the second pass. Because we enforce_match_threshold regardless, the run errored out, even though nearly every ID could be matched. See the log for this here

[johncmerfeld]

If the first run was successful but the second failed, surely we want to return the first run's records to the user

This is where I disagree.

• If the first pass succeeds and the second fails, then I believe we want to halt. We should return records only if we are confident that the underlying file is fine. If the second pass fails, we never upload the successful output set from the first pass. Thus, we are returning an unmatched_students file to the user that likely will not work when they resubmit it. Better to loudly fail and get to the bottom of why pass 2 did not work.
• I believe that if you remove the try/finally and keep just the if not met_initial_threshold:, you will achieve the fix you described without creating this new limbo state

What do you think?

[theokaufman]

LMK if I am on the same page as you, maybe I am semantically off base - when I say failed, I mean a run with 0 matches, not a run where earthmover crashed

^ Immediately after writing that I understood the issue here. Yes I totally agree with you, no need to wrap this in a try / finally. Thank you for catching this

[johncmerfeld]

I get the confusion. It used to be that a 0-match run would cause Earthmover to fail/crash but we found it useful to be able to distinguish between those cases