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

[WIP] Fix Identify accepting empty results #2231

Closed
wants to merge 3 commits into from

Conversation

bnkai
Copy link
Collaborator

@bnkai bnkai commented Jan 11, 2022

The identify task accepts empty results from a scraper and thus doesnt move on to the next scraper in line.
I added an extra check of the scraped scene so that if it is empty the next scraper will be used.
Also when a scraper errors out just try the next one instead of returning out

Steps to reproduce the original issue/bug

  • Select a few scenes to identify ( we need scenes that the first source won't have) and add some sources
    sources
  • Hit identify
  • Even though the first scraper doesnt return any results for some of the scenes it wont continue to the next source

@bnkai bnkai added the bug Something isn't working label Jan 11, 2022
@bnkai
Copy link
Collaborator Author

bnkai commented Jan 11, 2022

I removed the 2 scrape error tests from the identify_test because of the when a scraper errors out just try the next one instead of returning out change. Imho if a scraper source errors out we should just skip to the next not error out. If thats not ok let me know to revert the specific change and the tests.

@WithoutPants
Copy link
Collaborator

I think that this checking of a valid scrape should be an expected behaviour of the source.Scraper.ScrapeScene method. That is, ScrapeScene should not be returning empty scenes.

Also, I don't think that the error tests should be removed, but should be rewritten to accomodate the new behaviour.

@bnkai
Copy link
Collaborator Author

bnkai commented Jan 17, 2022

I think that this checking of a valid scrape should be an expected behaviour of the source.Scraper.ScrapeScene method. That is, ScrapeScene should not be returning empty scenes.

Also, I don't think that the error tests should be removed, but should be rewritten to accomodate the new behaviour.

Moved the validation so that ScrapeScene returns nil if the scrape result is empty (i assume thats what should not be returning empty scenes meant ?)

Tests were adjusted and re-added with the following logic
When a scraper (source) errors out in an identify task the scrape error is ignored so we can skip to the next source. That is an error in scrape doesnt mean an error in identify

Update: There is an issue with the tagger when no results are returned from the scraper as ScrapeSingleScene should not return null results according to the schema, will investigate further

@bnkai bnkai added the backend Pull requests that update Go code label Jan 17, 2022
@bnkai bnkai changed the title Fix Identify accepting empty results [W.I.P.] Fix Identify accepting empty results Jan 21, 2022
@bnkai bnkai changed the title [W.I.P.] Fix Identify accepting empty results [WIP] Fix Identify accepting empty results Jan 21, 2022
@bnkai
Copy link
Collaborator Author

bnkai commented Mar 10, 2022

Closing. Superceded by #2375

@bnkai bnkai closed this Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Pull requests that update Go code bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants