Update Flickr large batch handling #1047

stacimc · 2023-03-14T23:47:15Z

Fixes

Fast follow for Flickr backfill.
Related to WordPress/openverse#1285.

Description

This PR attempts to handle situations where the Flickr API returns excessively large batches of data. The logic should be documented pretty thoroughly in the code, but here's a refresher:

The Flickr API will only return 4,000 unique records for any given set of query params; after that, it will just infinitely return duplicates. Consequently, we have to try to query the API in such a way that we get batches with less than 4,000 records each. Up until now, we have been doing this using the TimeDelineatedProviderDataIngester to break the day into small time intervals for ingestion.

There is a limit to the granularity of data by time interval, though -- once you reduce the interval size to about 5 minutes, the number of records stays the same. For example, given a 5-min interval with > 4k records, if you search any 5 second interval within this range, you will still be returned >4k records. So reducing the timespan only works up to a certain point. However, we can still try to reduce the size of the result set by querying for one license type at a time, instead of all 8 license types at once.

This PR detects these large batch intervals during ingestion, adds them to an array for later processing, and skips ingestion for that batch. After 'regular' ingestion completes, each of these large intervals are reprocessed 8 times, once for each license type. It's still possible for a 5-min interval to contain more than 4k records for a single license type, but in this case there's nothing more we can do, so we process the first 4,000 results and then continue.

Notes

It is impossible to guarantee that we will get all records, but this should dramatically increase what we're able to ingest.
Notably, this situation is most likely to arise on ingestion days with a lot of data, so making this improvement will result in a large data increase.
After merging this update, I'll re-run the failed days in production.

Testing Instructions

Try running the flickr DAG locally. In particular, run it for one of the days that failed in production using the DagRun conf options.

I tried a manual run with the conf:

{
    "date":"2023-02-26"
}

This day failed in production after ingesting 7,253 records. Tested locally against this branch, the run succeeded after about 10 minutes and ingested 56,927 records.

Checklist

My pull request has a descriptive title (not a vague title like
Update index.md).
My pull request targets the default branch of the repository (main) or
a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible
errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

… a single ts pair

…cords as possible

stacimc · 2023-03-14T23:48:20Z

openverse_catalog/dags/providers/provider_api_scripts/time_delineated_provider_data_ingester.py

@@ -235,9 +250,9 @@ def get_should_continue(self, response_json) -> bool:
                " been fetched. Consider reducing the ingestion interval."
            )
            if self.should_raise_error:
-                raise AirflowException(error_message)
+                raise Exception(error_message)


It's necessary to change this to an Exception, because AirflowExceptions are specifically not handled by our ingestion error handling logic (meaning they can never be skipped).

@stacimc Is this feedback worth adding to the code?

# Must be an `Exception` instead of an `AirflowException` to allow skipping.

Or something like that.

Or potentially even a custom, Exception-derived exception subclass?

Our own error handling in the super class will re-raise this as an IngestionError already.

This isn't a problem with this specific exception so much as a general point about error handling in the ingester classes: we probably shouldn't be raising AirflowExceptions ourselves anyway.

openverse-bot · 2023-03-22T00:00:04Z

Based on the high urgency of this PR, the following reviewers are being gently reminded to review this PR:

@AetherUnbound
@obulat
This reminder is being automatically generated due to the urgency configuration.

Excluding weekend¹ days, this PR was updated 2 day(s) ago. PRs labelled with high urgency are expected to be reviewed within 2 weekday(s)².

@stacimc, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings.

Specifically, Saturday and Sunday. ↩
For the purpose of these reminders we treat Monday - Friday as weekdays. Please note that the that generates these reminders runs at midnight UTC on Monday - Friday. This means that depending on your timezone, you may be pinged outside of the expected range. ↩

zackkrida · 2023-03-22T18:25:40Z

openverse_catalog/dags/providers/provider_api_scripts/flickr.py

+            logger.error(
+                f"{detected_count} records retrieved, but there is a"
+                f" limit of {self.max_unique_records}."
+            )


This log was so useful when observing this PR in airflow. Before even reading the code it gave a great understanding of what was happening 👍

zackkrida

Really cool. On my local run I picked up 46,732 records. As always, phenomenal documentation 😍

obulat

This is such a clever idea! Hope we can get most of the images now.
Thank you for amazing documentation and tests.

AetherUnbound

Incredible documentation, thanks for getting this all down in the various docstrings/comments! I hope this will be useful for other folks who encounter issues with the Flickr API 😄

AetherUnbound · 2023-03-24T23:17:12Z

openverse_catalog/dags/providers/provider_api_scripts/time_delineated_provider_data_ingester.py

@@ -235,9 +250,9 @@ def get_should_continue(self, response_json) -> bool:
                " been fetched. Consider reducing the ingestion interval."
            )
            if self.should_raise_error:
-                raise AirflowException(error_message)
+                raise Exception(error_message)


Or potentially even a custom, Exception-derived exception subclass?

stacimc added 3 commits March 14, 2023 15:29

Prevent serialization errors when query params include datetimes

04b2864

Refactor TimeDelineated base class to allow calling ingest_records on…

cdd9d37

… a single ts pair

Update Flickr to split large batches by license and ingest as many re…

9062313

…cords as possible

stacimc added 🟧 priority: high Stalls work on the project or its dependents ✨ goal: improvement Improvement to an existing user-facing feature 💻 aspect: code Concerns the software code in the repository 🧱 stack: catalog Related to the catalog and Airflow DAGs labels Mar 14, 2023

stacimc requested a review from a team as a code owner March 14, 2023 23:47

stacimc self-assigned this Mar 14, 2023

stacimc requested review from AetherUnbound and obulat March 14, 2023 23:47

openverse-bot added this to Needs review in Openverse PRs Mar 14, 2023

stacimc commented Mar 14, 2023

View reviewed changes

zackkrida reviewed Mar 22, 2023

View reviewed changes

zackkrida approved these changes Mar 22, 2023

View reviewed changes

obulat approved these changes Mar 24, 2023

View reviewed changes

Openverse PRs automation moved this from Needs review to Reviewer approved Mar 24, 2023

AetherUnbound approved these changes Mar 24, 2023

View reviewed changes

stacimc merged commit 7d0ce7f into main Mar 27, 2023
29 checks passed

Openverse PRs automation moved this from Reviewer approved to Merged! Mar 27, 2023

stacimc deleted the update/flickr-large-batch-handling branch March 27, 2023 20:30

AetherUnbound mentioned this pull request Mar 28, 2023

Wikimedia reingestion workflow time out WordPress/openverse#1269

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Flickr large batch handling #1047

Update Flickr large batch handling #1047

stacimc commented Mar 14, 2023 •

edited by zackkrida

stacimc Mar 14, 2023

zackkrida Mar 22, 2023

AetherUnbound Mar 24, 2023

stacimc Mar 27, 2023

openverse-bot commented Mar 22, 2023

zackkrida Mar 22, 2023

zackkrida left a comment

obulat left a comment

AetherUnbound left a comment

AetherUnbound Mar 24, 2023

Update Flickr large batch handling #1047

Update Flickr large batch handling #1047

Conversation

stacimc commented Mar 14, 2023 • edited by zackkrida

Fixes

Description

Notes

Testing Instructions

Checklist

Developer Certificate of Origin

stacimc Mar 14, 2023

Choose a reason for hiding this comment

zackkrida Mar 22, 2023

Choose a reason for hiding this comment

AetherUnbound Mar 24, 2023

Choose a reason for hiding this comment

stacimc Mar 27, 2023

Choose a reason for hiding this comment

openverse-bot commented Mar 22, 2023

Footnotes

zackkrida Mar 22, 2023

Choose a reason for hiding this comment

zackkrida left a comment

Choose a reason for hiding this comment

obulat left a comment

Choose a reason for hiding this comment

AetherUnbound left a comment

Choose a reason for hiding this comment

AetherUnbound Mar 24, 2023

Choose a reason for hiding this comment

stacimc commented Mar 14, 2023 •

edited by zackkrida