-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix no lineages error #609
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #609 +/- ##
==========================================
- Coverage 47.15% 47.13% -0.03%
==========================================
Files 75 75
Lines 6457 6460 +3
==========================================
Hits 3045 3045
- Misses 3412 3415 +3 ☔ View full report in Codecov by Sentry. |
Hm, I'm thinking what should happen here is that the match should complete but show no matches rather than fail outright. Maybe we should discuss in the re team slack |
👍 |
if not lineages: | ||
# return no matches | ||
await storage.update_match_state( | ||
internal_match_id, models.ProcessState.COMPLETE, now_epoch_millis(), [] | ||
) | ||
return |
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.
You should do this in perform_gtdb_lineage_match
, otherwise you'll miss some cases since the matcher chooses one of the two *strategy functions
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.
also maybe make this a function and use that function wherever the same code is called
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 moved it to perform_gtdb_lineage_match
. It's just calling storage.update_match_state
. Making this single function call a function?
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.
Fair enough, I guess you'd only be getting rid of a couple args, and the millis function probably shouldn't be abstracted out anyway since it's not mockable and needs to be changed
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 guess it is mockable by monkey patching but I hate that
No description provided.