LB-372: MessyBrainz: Create artist_credit clusters using artist MBIDs in database #41

kartikeyaSh · 2018-06-13T20:06:32Z

Summary

This PR includes a script for creating artist_credit clusters.

This is a…
- Bug fix
- Feature addition
- Refactoring
- Minor / simple change (like a typo)
- Other

Problem

We need to create clusters and associate MSIDs to MBIDs.

JIRA ticket (optional): LB-372

Solution

Add a script which will create clusters for artists that have artist MBIDs in recording_json table.
This is somewhat similar to what I've written in my proposal

Additional things apart from the proposal:

New table artist_credit_redirect_array which stores artist MBIDs as arrays, and is represented by an artist_credit_cluster_id.
The clusters are created in 2 phases in phase-1 simple cases are handled and in phase-2 anomalies are handled.

Add convert_json_array_to_sorted_uuid_array which converts a given JSON array to a sorted psql array of type UUID and array_sort which returns a sorted UUID array.

Add artist_credit_redirect_array table to store cluster_id, artist_mbids_array pairs.

kartikeyaSh · 2018-06-13T20:07:55Z

Here is a gist to test run: https://gist.github.com/kartikeyaSh/f80ce28b0d29ccee61fbe235f5acddc3
I'll write tests for this soon.

Add functionality to create clusters of artist_credit using artist_mbids present in the recording_json table.

Add artist_credit_redirect_array table

paramsingh · 2018-06-16T17:37:12Z

What exactly do you mean by anomaly here?

kartikeyaSh · 2018-06-17T15:17:51Z

@paramsingh A single MSID pointing to multiple MBIDs. e.g. James Morrison and James Morrison. Another way to put it is: Any artist_credit_cluster_id which appears more than once in artist_credit_redirect_array table is an anomaly. You can see that in the above gist that cluster ID 1ff80901-f7e9-4f2a-ac37-3ed0ce216362 points to 4 different MBIDs in artist_credit_redirect_array.

First fetch all unique cluster_id for a list of artist_gids instead of fetching cluster_id for artist_gid as this may cause duplicate inserts in artist_credit_redirect table.

mayhem

Undo the dropping of that table and then I think we should merge and move on.

mayhem · 2018-06-26T15:26:32Z

admin/sql/updates/2018-06-25-drop-artist-credit-redirect.sql

@@ -0,0 +1,5 @@
+BEGIN;


Not sure this is really needed since we haven't updated production yet -- so that table will not exist there...

paramsingh · 2018-06-16T16:39:36Z

messybrainz/db/artist.py

+
+    Args:
+        connection: the sqlalchemy db connection to be used to execute queries
+        artist_credit_mbids (list): a list of sorted artist MBIDs for which


What will happen if this list is not sorted? We should consider taking any order and sorting it ourselves inside the function.

paramsingh · 2018-06-16T17:20:22Z

messybrainz/db/artist.py

+
+
+def create_artist_credit_clusters_without_considering_anomalies(connection):
+    """Creates cluster for artist_credit without considering anamolies.


Please specify in the comment what you mean by anomalies here (and in other places).

Also anamolies -> anomalies

paramsingh · 2018-06-18T10:14:50Z

messybrainz/db/artist.py

@@ -102,3 +103,297 @@ def fetch_and_store_artist_mbids_for_all_recording_mbids():
                pass

        return num_recording_mbids_processed, num_recording_mbids_added
+
+
+def fetch_distinct_artist_credit_mbids(connection):


this name should be fixed, there is no indication in the function name that it only fetches those mbids which do not have msids in the artist_credit_cluster table.

paramsingh · 2018-06-18T10:20:43Z

messybrainz/db/artist.py

@@ -1,6 +1,7 @@

 import brainzutils.musicbrainz_db.recording as mb_recording
 import json
+import uuid


is this import being used somewhere?

paramsingh · 2018-06-26T15:32:42Z

admin/sql/updates/2018-06-25-drop-artist-credit-redirect.sql

@@ -0,0 +1,5 @@
+BEGIN;
+
+DROP TABLE IF EXISTS artist_credit_redirect CASCADE;


Just move this to the other update script, keeping multiple schema change scripts in one PR doesn't really make sense.

paramsingh · 2018-06-26T15:37:59Z

messybrainz/db/tests/test_artist.py

+
+
+    def test_link_artist_mbids_to_artist_credit_cluster_id(self):
+        """Tests if artist MBIDs are linked to correctly. Links are created in


linked to cluster

Rename artist_credit_redirect to artist_credit_redirect and alter the column to store artist MBIDs to array of UUIDs.

Rename fetch_distinct_artist_credit_mbids to fetch_unclustered_distinct_artist_credit_mbids. This gives indication that only those MBIDs are fetched which have not been clustered.

paramsingh · 2018-06-30T07:12:15Z

messybrainz/db/common.py

+       to multiple MBIDs in entity_redirect table).
+
+    Args:
+        connection: the sqlalchemy db connection to be used to execute queries.


taking a connection as input here and then creating a connection in the function as well.

done:). removed creating a connection in the function.

paramsingh · 2018-06-30T07:16:06Z

From IRC

iliekcomputers: as you mentioned in #44 I wrote generic functions. I'm not sure If I need to write tests for those functions cz those get tested when testing create_artist_credit_clusters, create_artist_credit_clusters_without_anomalies........ And I can't test those functions without using an entity like artist/release.

I don't think we need tests for the generic functions. They'll only get called by each of the entity clustering functions and tested there.

These functions are common to both artist and release when creating clusters.

Use function declared in db_common to create clusters.

paramsingh

🥇

This makes it easy to understand how clusters are formed.

mayhem

The logging output option should also be added to the other clustering functions in manage.py, but we can do that in the other PRs. For now, let's merge!

kartikeyaSh added 3 commits June 14, 2018 00:35

Don't use undeclared variable

8bc2264

Add two functions to postgres

0cfd93a

Add convert_json_array_to_sorted_uuid_array which converts a given JSON array to a sorted psql array of type UUID and array_sort which returns a sorted UUID array.

Add a new table

cfb01b8

Add artist_credit_redirect_array table to store cluster_id, artist_mbids_array pairs.

kartikeyaSh changed the title ~~Artist cluster~~ LB-372: MessyBrainz: Create artist_credit clusters using artist MBIDs in database Jun 13, 2018

kartikeyaSh force-pushed the artist_cluster branch from fd45542 to b2b44f1 Compare June 14, 2018 18:18

kartikeyaSh added 4 commits June 16, 2018 00:34

Add functionality to create artist_credit clusters

a7993ff

Add functionality to create clusters of artist_credit using artist_mbids present in the recording_json table.

Add schema update

9407e3f

Add artist_credit_redirect_array table

Add test data for artist clusters

97d7732

Add tests for creating artist_credit clusters

4631d20

kartikeyaSh force-pushed the artist_cluster branch from b2b44f1 to 4631d20 Compare June 15, 2018 20:15

Change the way how anomalies are clustered

fc91ae4

First fetch all unique cluster_id for a list of artist_gids instead of fetching cluster_id for artist_gid as this may cause duplicate inserts in artist_credit_redirect table.

mayhem suggested changes Jun 26, 2018

View reviewed changes

kartikeyaSh force-pushed the artist_cluster branch from 4aecf0a to fc91ae4 Compare June 26, 2018 15:33

paramsingh reviewed Jun 26, 2018

View reviewed changes

kartikeyaSh added 4 commits June 28, 2018 12:03

Alter and rename artist_credit_redirect_array

4314198

Rename artist_credit_redirect to artist_credit_redirect and alter the column to store artist MBIDs to array of UUIDs.

Fix comments and remove unused imports

d5c4d96

Sort list before querying the database

21c5b50

Rename fetch_distinct_artist_credit_mbids

8c7af9a

Rename fetch_distinct_artist_credit_mbids to fetch_unclustered_distinct_artist_credit_mbids. This gives indication that only those MBIDs are fetched which have not been clustered.

kartikeyaSh force-pushed the artist_cluster branch from f762c15 to 8c7af9a Compare June 28, 2018 07:14

Alphabetize tables

14842f4

paramsingh reviewed Jun 30, 2018

View reviewed changes

kartikeyaSh added 2 commits June 30, 2018 13:23

Add generic functions common to entities

3a41721

These functions are common to both artist and release when creating clusters.

Use generic functions

a9ccca8

Use function declared in db_common to create clusters.

kartikeyaSh force-pushed the artist_cluster branch from c8c64cb to a9ccca8 Compare June 30, 2018 07:54

paramsingh approved these changes Jun 30, 2018

View reviewed changes

Add logging while creating clusters

a25e7cc

This makes it easy to understand how clusters are formed.

Use logging instead of print

2cef8e9

mayhem approved these changes Jul 2, 2018

View reviewed changes

mayhem merged commit fc1da45 into metabrainz:master Jul 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LB-372: MessyBrainz: Create artist_credit clusters using artist MBIDs in database #41

LB-372: MessyBrainz: Create artist_credit clusters using artist MBIDs in database #41

kartikeyaSh commented Jun 13, 2018

kartikeyaSh commented Jun 13, 2018

paramsingh commented Jun 16, 2018

kartikeyaSh commented Jun 17, 2018

mayhem left a comment

mayhem Jun 26, 2018

paramsingh Jun 16, 2018

paramsingh Jun 16, 2018

paramsingh Jun 26, 2018

paramsingh Jun 18, 2018

paramsingh Jun 18, 2018

paramsingh Jun 26, 2018

paramsingh Jun 26, 2018

paramsingh Jun 30, 2018

kartikeyaSh Jun 30, 2018

paramsingh commented Jun 30, 2018

paramsingh left a comment

mayhem left a comment



		def create_artist_credit_clusters_without_considering_anomalies(connection):
		"""Creates cluster for artist_credit without considering anamolies.

		@@ -0,0 +1,5 @@
		BEGIN;

		DROP TABLE IF EXISTS artist_credit_redirect CASCADE;



		def test_link_artist_mbids_to_artist_credit_cluster_id(self):
		"""Tests if artist MBIDs are linked to correctly. Links are created in

LB-372: MessyBrainz: Create artist_credit clusters using artist MBIDs in database #41

LB-372: MessyBrainz: Create artist_credit clusters using artist MBIDs in database #41

Conversation

kartikeyaSh commented Jun 13, 2018

Summary

Problem

Solution

Additional things apart from the proposal:

kartikeyaSh commented Jun 13, 2018

paramsingh commented Jun 16, 2018

kartikeyaSh commented Jun 17, 2018

mayhem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paramsingh commented Jun 30, 2018

paramsingh left a comment

Choose a reason for hiding this comment

mayhem left a comment

Choose a reason for hiding this comment