Implementation plan: Catalog Data Cleaning #3848

krysal · 2024-02-29T03:23:04Z

Fixes

Description

Since the project proposal is clear with what we want to achieve in this case, and in a past attempt, there were more doubts with the technical details than with the proposal itself, here I directly add an implementation plan with the most straightforward path identified.

This proposal also represents work that @obulat pushed last year; it's partly possible due to her effort and the @stacimc's precedent in the batched_update DAG. Kudos to both!

This discussion follows the Openverse decision-making process. Information about this process can be found on the Openverse documentation site.

Requested reviewers or participants will be following this process. If you are being asked to give input on a specific detail, you do not need to familiarize yourself with and follow the process.

Current round

This discussion is currently in the Decision round.

The deadline for review of this round is March 12, 2024.

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.

github-actions · 2024-02-29T14:32:53Z

Full-stack documentation: https://docs.openverse.org/_preview/3848

Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again.

You can check the GitHub pages deployment action list to see the current status of the deployments.

New files ➕:

AetherUnbound

Thanks for drafting this @krysal! I have a number of editorial suggestions and questions as part of the clarification round.

.../projects/proposals/data_normalization/20240227-implementation_plan_catalog_data_cleaning.md

AetherUnbound · 2024-03-02T00:47:52Z

.../projects/proposals/data_normalization/20240227-implementation_plan_catalog_data_cleaning.md

+If confirmed the time is reduced to zero, optionally the cleaning steps can be
+removed, or leave them in case we want to perform a similar cleaning effort
+later.


I think we can maybe leave a cleaning "shim" in for the future, but the current cleaning logic that gets called should be removed.

How will look a cleaning shim in your opinion?

I was thinking we could perhaps leave this section in, even if clean_image_data is a noop:

openverse/ingestion_server/ingestion_server/ingest.py

Lines 326 to 333 in 9b4f727

if downstream_table == "image":

# Step 5: Clean the data

log.info("Cleaning data...")

clean_image_data(downstream_table)

log.info("Cleaning completed!")

slack.status(

downstream_table, "Data cleaning complete | _Next: Elasticsearch reindex_"

)

Now that I say it though, it seems silly 😅 we probably don't need to keep that either!

I kind of think we do need to have this shim, because I'm pretty sure we will find something to clean, at least in the short run. Would be nice to have some criteria to determine when to remove the shim. Maybe, when the second stage of the data normalization project is done?

With this shim in place, the functions that clean tags and URLs in the cleanup.py would be removed, and this dictionary would have empty values:

openverse/ingestion_server/ingestion_server/cleanup.py

Lines 151 to 167 in 9b4f727

_cleanup_config = {

"tables": {

"image": {

"sources": {

# Applies to all sources.

"*": {

"fields": {

"tags": CleanupFunctions.cleanup_tags,

"url": CleanupFunctions.cleanup_url,

"creator_url": CleanupFunctions.cleanup_url,

"foreign_landing_url": CleanupFunctions.cleanup_url,

}

}

}

}

}

}

I was debating between the two options, too.

Would be nice to have some criteria to determine when to remove the shim. Maybe, when the second stage of the data normalization project is done?

That's a great idea! I agree with it 💯

.../projects/proposals/data_normalization/20240227-implementation_plan_catalog_data_cleaning.md

obulat

Great plan, @krysal !

Could we also add some notes on how we keep track of the cleaned up data, and how we report on it? This will probably be useful for the person who runs the DAGs, and maybe the MSR runner?

.../projects/proposals/data_normalization/20240227-implementation_plan_catalog_data_cleaning.md

krysal · 2024-03-05T21:01:51Z

Could we also add some notes on how we keep track of the cleaned up data, and how we report on it? This will probably be useful for the person who runs the DAGs, and maybe the MSR runner?

@obulat Good note! I see you're already thinking about re-using the DAG outside of cleaning tasks 😄 I added that step to the DAG specification. Thanks for bringing it up and for the quick review!

krysal · 2024-03-05T22:11:49Z

@AetherUnbound @obulat I made the corrections from your excellent suggestions. This is ready for another review 📑

AetherUnbound

Just a couple more comments, no further input after this from me on the clarification round! 🙂

.../projects/proposals/data_normalization/20240227-implementation_plan_catalog_data_cleaning.md

obulat

I think we can go on to the next stage.

.../projects/proposals/data_normalization/20240227-implementation_plan_catalog_data_cleaning.md

obulat · 2024-03-07T08:07:39Z

.../projects/proposals/data_normalization/20240227-implementation_plan_catalog_data_cleaning.md

+If confirmed the time is reduced to zero, optionally the cleaning steps can be
+removed, or leave them in case we want to perform a similar cleaning effort
+later.


I kind of think we do need to have this shim, because I'm pretty sure we will find something to clean, at least in the short run. Would be nice to have some criteria to determine when to remove the shim. Maybe, when the second stage of the data normalization project is done?

With this shim in place, the functions that clean tags and URLs in the cleanup.py would be removed, and this dictionary would have empty values:

openverse/ingestion_server/ingestion_server/cleanup.py

Lines 151 to 167 in 9b4f727

_cleanup_config = {

"tables": {

"image": {

"sources": {

# Applies to all sources.

"*": {

"fields": {

"tags": CleanupFunctions.cleanup_tags,

"url": CleanupFunctions.cleanup_url,

"creator_url": CleanupFunctions.cleanup_url,

"foreign_landing_url": CleanupFunctions.cleanup_url,

}

}

}

}

}

}

.../projects/proposals/data_normalization/20240227-implementation_plan_catalog_data_cleaning.md

Co-authored-by: Madison Swain-Bowden <[email protected]> Co-authored-by: Olga Bulat <[email protected]> Fix and add links Add suggested extra issue and adjust the Expected Outcomes Include smart_open in the Tools & packages section Apply editorial suggestions

krysal · 2024-03-07T21:01:34Z

Document updated. I moved the discussion to the Decision round 🥁

AetherUnbound

No blocking objections, the plan looks good! 😄

.../projects/proposals/data_normalization/20240227-implementation_plan_catalog_data_cleaning.md

openverse-bot · 2024-03-12T00:12:21Z

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

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

Excluding weekend¹ days, this PR was ready for review 4 day(s) ago. PRs labelled with medium urgency are expected to be reviewed within 4 weekday(s)².

@krysal, 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 operation 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. ↩

obulat

Excited to get this project started!

.../projects/proposals/data_normalization/20240227-implementation_plan_catalog_data_cleaning.md

Co-authored-by: Olga Bulat <[email protected]> Co-authored-by: Madison Swain-Bowden <[email protected]>

krysal · 2024-03-12T16:47:43Z

Thank you folks! ✨

krysal force-pushed the rfc/data-cleaning branch from 933cbd3 to 28bbd68 Compare February 29, 2024 14:25

krysal force-pushed the rfc/data-cleaning branch from 28bbd68 to 10bd0a4 Compare February 29, 2024 21:33

krysal requested review from AetherUnbound and obulat February 29, 2024 21:37

krysal marked this pull request as ready for review February 29, 2024 21:47

krysal requested a review from a team as a code owner February 29, 2024 21:47

AetherUnbound reviewed Mar 2, 2024

View reviewed changes

obulat reviewed Mar 3, 2024

View reviewed changes

krysal marked this pull request as draft March 4, 2024 15:20

krysal force-pushed the rfc/data-cleaning branch 3 times, most recently from 7c0d57b to 3d70e64 Compare March 5, 2024 19:28

krysal marked this pull request as ready for review March 5, 2024 22:09

krysal requested review from AetherUnbound and obulat March 5, 2024 22:09

krysal mentioned this pull request Mar 6, 2024

Data normalization #430

Closed

AetherUnbound reviewed Mar 7, 2024

View reviewed changes

.../projects/proposals/data_normalization/20240227-implementation_plan_catalog_data_cleaning.md Outdated Show resolved Hide resolved

obulat reviewed Mar 7, 2024

View reviewed changes

obulat assigned krysal Mar 7, 2024

krysal force-pushed the rfc/data-cleaning branch from eb6a419 to 483be63 Compare March 7, 2024 17:01

krysal added 2 commits March 7, 2024 13:38

Add Data Cleaning implementation plan

376d883

Add reviewers

e39a2cf

krysal force-pushed the rfc/data-cleaning branch 5 times, most recently from 0532dae to d17e708 Compare March 7, 2024 20:34

krysal force-pushed the rfc/data-cleaning branch from d17e708 to 9e3d2cb Compare March 7, 2024 20:49

krysal requested review from AetherUnbound and obulat March 7, 2024 21:01

AetherUnbound approved these changes Mar 11, 2024

View reviewed changes

AetherUnbound reviewed Mar 11, 2024

View reviewed changes

.../projects/proposals/data_normalization/20240227-implementation_plan_catalog_data_cleaning.md Outdated Show resolved Hide resolved

obulat approved these changes Mar 12, 2024

View reviewed changes

.../projects/proposals/data_normalization/20240227-implementation_plan_catalog_data_cleaning.md Outdated Show resolved Hide resolved

.../projects/proposals/data_normalization/20240227-implementation_plan_catalog_data_cleaning.md Outdated Show resolved Hide resolved

Apply suggestions from code review

5c5e771

Co-authored-by: Olga Bulat <[email protected]> Co-authored-by: Madison Swain-Bowden <[email protected]>

krysal merged commit d7d4208 into main Mar 12, 2024
37 checks passed

krysal deleted the rfc/data-cleaning branch March 12, 2024 16:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation plan: Catalog Data Cleaning #3848

Implementation plan: Catalog Data Cleaning #3848

krysal commented Feb 29, 2024 •

edited

Loading

github-actions bot commented Feb 29, 2024

AetherUnbound left a comment

AetherUnbound Mar 2, 2024

krysal Mar 5, 2024

AetherUnbound Mar 7, 2024

obulat Mar 7, 2024

krysal Mar 7, 2024

obulat left a comment

krysal commented Mar 5, 2024

krysal commented Mar 5, 2024

AetherUnbound left a comment

obulat left a comment

obulat Mar 7, 2024

krysal commented Mar 7, 2024

AetherUnbound left a comment

openverse-bot commented Mar 12, 2024

obulat left a comment

krysal commented Mar 12, 2024

	if downstream_table == "image":
	# Step 5: Clean the data
	log.info("Cleaning data...")
	clean_image_data(downstream_table)
	log.info("Cleaning completed!")
	slack.status(
	downstream_table, "Data cleaning complete \| _Next: Elasticsearch reindex_"
	)

	_cleanup_config = {
	"tables": {
	"image": {
	"sources": {
	# Applies to all sources.
	"*": {
	"fields": {
	"tags": CleanupFunctions.cleanup_tags,
	"url": CleanupFunctions.cleanup_url,
	"creator_url": CleanupFunctions.cleanup_url,
	"foreign_landing_url": CleanupFunctions.cleanup_url,
	}
	}
	}
	}
	}
	}

Implementation plan: Catalog Data Cleaning #3848

Implementation plan: Catalog Data Cleaning #3848

Conversation

krysal commented Feb 29, 2024 • edited Loading

Fixes

Description

Current round

Developer Certificate of Origin

github-actions bot commented Feb 29, 2024

AetherUnbound left a comment

Choose a reason for hiding this comment

AetherUnbound Mar 2, 2024

Choose a reason for hiding this comment

krysal Mar 5, 2024

Choose a reason for hiding this comment

AetherUnbound Mar 7, 2024

Choose a reason for hiding this comment

obulat Mar 7, 2024

Choose a reason for hiding this comment

krysal Mar 7, 2024

Choose a reason for hiding this comment

obulat left a comment

Choose a reason for hiding this comment

krysal commented Mar 5, 2024

krysal commented Mar 5, 2024

AetherUnbound left a comment

Choose a reason for hiding this comment

obulat left a comment

Choose a reason for hiding this comment

obulat Mar 7, 2024

Choose a reason for hiding this comment

krysal commented Mar 7, 2024

AetherUnbound left a comment

Choose a reason for hiding this comment

openverse-bot commented Mar 12, 2024

Footnotes

obulat left a comment

Choose a reason for hiding this comment

krysal commented Mar 12, 2024

krysal commented Feb 29, 2024 •

edited

Loading