SOLR-61: Bring Recording to search parity #40

samj1912 · 2017-10-15T09:59:00Z

See https://tickets.metabrainz.org/browse/SOLR-61 for the list of errors

mayhem · 2017-10-15T11:13:08Z

Here as well, having links to see what is going on helps.

mayhem · 2017-10-15T11:17:40Z

This PR is really hard to follow since so many things changed and the ticket also lists a pile of things that are wrong. And the outputs from the two calls in the ticket are also really hard to compare.

samj1912 · 2017-10-15T12:23:14Z

sir/wscompat/convert.py



 def convert_format(obj):
-    return models.format(id=str(obj.id))
+    return models.format(valueOf_=obj.name)


For fixing medium format(format)
See https://musicbrainz.org/ws/2/recording?query=format:CD&limit=1 vs
http://35.193.119.46:8983/solr/recording/select?q=format:CD&rows=1

Expected- <format>CD</format> Result- <format id="1"/>

samj1912 · 2017-10-15T12:24:45Z

sir/wscompat/convert.py



 def convert_release_status(obj):
    """
    :type obj: :class:`mbdata.models.ReleaseStatus`
    """
-    return models.status(id=str(obj.id))
+    return models.status(valueOf_=obj.name)


For fixing release status output(format)
See https://musicbrainz.org/ws/2/recording?query=status:official&limit=1 vs
http://35.193.119.46:8983/solr/recording/select?q=format:CD&rows=1

Expected- <status>Official</status> Result- <status id="1"/>

samj1912 · 2017-10-15T12:25:01Z

sir/wscompat/convert.py



 def convert_gender(obj):
-    return models.gender(valueOf_=str(obj.name.lower()))
+    return models.gender(valueOf_=obj.name.lower())


Simply removes the unnecessary str type conversion

samj1912 · 2017-10-15T12:26:45Z

sir/wscompat/convert.py

@@ -992,19 +1020,19 @@ def convert_release_group_primary_type(obj):
    """
    :type obj: :class:`mbdata.models.ReleaseGroupPrimaryType`
    """
-    return models.primary_type(id=str(obj.id))
+    return models.primary_type(valueOf_=obj.name)


Fixes primary type output of a release (pt)

see https://musicbrainz.org/ws/2/recording?query=primarytype:album&limit=1
vs
http://35.193.119.46:8983/solr/recording/select?q=primarytype:album&rows=1

<primary-type>Album</primary-type> vs <primary-type id="1"/>

samj1912 · 2017-10-15T12:29:05Z

sir/wscompat/convert.py

-
+        for release in recording.release_list.release:
+            if release.artist_credit == recording.artist_credit:
+                release.set_artist_credit(None)


Fixes label (ac)

See

https://musicbrainz.org/ws/2/recording?query=rid:4b3003ee-dbd6-48de-8dfe-073e1c7c9cf1&limit=1

vs
http://35.193.119.46:8983/solr/recording/select?q=rid:4b3003ee-dbd6-48de-8dfe-073e1c7c9cf1&rows=1

release output. Lot of extra unnecessary information.

samj1912 · 2017-10-15T12:34:43Z

sir/wscompat/convert.py

@@ -483,7 +509,7 @@ def convert_release_group_for_release(obj):

    if obj.type is not None:
        rg.set_primary_type(convert_release_group_primary_type(obj.type))
-        rg.set_type(obj.type.name)
+        rg.set_type(calculate_type(obj.type, obj.secondary_types))


Fixes labels st and type
see the output for type of the release in
https://musicbrainz.org/ws/2/recording?query=reid:e5da47bc-9a97-4682-8294-ef7458cd3984&limit=1
and
http://35.193.119.46:8983/solr/recording/select?q=reid:e5da47bc-9a97-4682-8294-ef7458cd3984&rows=1
for release id e5da47bc-9a97-4682-8294-ef7458cd3984

samj1912 · 2017-10-15T12:38:16Z

sir/wscompat/convert.py

@@ -445,7 +471,7 @@ def convert_release_from_track(obj):
    # same as the recording artist credit, but we've already built it so just
    # set it
    release.set_artist_credit(convert_artist_credit(rel.artist_credit,
-                                                    include_aliases=True))
+                                                    include_aliases=False))


Aliases are not needed in the output for related release artist credit

See
https://musicbrainz.org/ws/2/recording?query=reid:0722a81b-dc18-4c19-9d36-fc28cd450a63&limit=1
vs
http://35.193.119.46:8983/solr/recording/select?q=reid:0722a81b-dc18-4c19-9d36-fc28cd450a63&rows=1

You are looking at the output of artist credit for the release 0722a81b-dc18-4c19-9d36-fc28cd450a63

samj1912 · 2017-10-15T12:38:52Z

sir/wscompat/convert.py

@@ -50,6 +58,28 @@ def datetime_to_string(obj):
    return obj.strftime(TIME_FORMAT)


+def calculate_type(primary_type, secondary_types):


related to fixing label type in the ticket

samj1912 · 2017-10-15T12:40:00Z

sir/wscompat/convert.py

@@ -128,8 +158,7 @@ def convert_name_credit(obj, include_aliases=True):
    """
    :type obj: :class:`mbdata.models.ArtistCreditName`
    """
-    nc = models.name_credit(name=obj.name,
-                            artist=convert_artist_simple(obj.artist,
+    nc = models.name_credit(artist=convert_artist_simple(obj.artist,


related to fixing label name in the ticket
No need for name output in artist credit
see the output of artist-credit in
https://musicbrainz.org/ws/2/recording?query=rid:4b3003ee-dbd6-48de-8dfe-073e1c7c9cf1&limit=1
vs
http://35.193.119.46:8983/solr/recording/select?q=rid:4b3003ee-dbd6-48de-8dfe-073e1c7c9cf1&rows=1

samj1912 · 2017-10-15T12:41:07Z

sir/wscompat/convert.py

@@ -325,9 +354,6 @@ def convert_medium(obj):
    if obj.format is not None:
        m.set_format(convert_format(obj.format))

-    dl = models.disc_list(count=len(obj.cdtocs))


No disc list is shown in the current ws output

see
https://musicbrainz.org/ws/2/recording?query=rid:4b3003ee-dbd6-48de-8dfe-073e1c7c9cf1&limit=1
vs
http://35.193.119.46:8983/solr/recording/select?q=rid:4b3003ee-dbd6-48de-8dfe-073e1c7c9cf1&rows=1

Specifically

<disc-list count="1"/>

is not there in current output so removed it.

This is used by convert_release as well, where http://musicbrainz.org/ws/2/release?query=heino has a disc-list attribute.

Yes, I have fixed it properly in the PR for release.

samj1912 · 2017-10-15T12:42:07Z

sir/wscompat/convert.py

@@ -359,7 +385,7 @@ def convert_medium_list(obj):
    """
    :type obj: :class:`[mbdata.models.Medium]`
    """
-    ml = models.medium_list(count=len(obj))
+    ml = models.medium_list()


Related to label count on the ticket.
Current medium output does not have a count attribute
see
https://musicbrainz.org/ws/2/recording?query=rid:4b3003ee-dbd6-48de-8dfe-073e1c7c9cf1&limit=1
vs
http://35.193.119.46:8983/solr/recording/select?q=rid:4b3003ee-dbd6-48de-8dfe-073e1c7c9cf1&rows=1

this translates to a bad json output (similar to the tag PR we merged #38 )

mayhem

Thank you for explaining these -- looks pretty sane!

samj1912 · 2017-10-15T17:31:19Z

Fixed the commits into something more manageable for future git history/ git bisect.

mineo · 2017-10-16T15:37:15Z

sir/wscompat/convert.py

@@ -50,6 +58,28 @@ def datetime_to_string(obj):
    return obj.strftime(TIME_FORMAT)


+def calculate_type(primary_type, secondary_types):


I'd add lru_cache tu this function because (compared to the amount of recordings) the number of primary / secondary type combinations is very small (and constant).

mineo · 2017-10-16T15:43:07Z

sir/wscompat/convert.py

@@ -325,9 +354,6 @@ def convert_medium(obj):
    if obj.format is not None:
        m.set_format(convert_format(obj.format))

-    dl = models.disc_list(count=len(obj.cdtocs))


This is used by convert_release as well, where http://musicbrainz.org/ws/2/release?query=heino has a disc-list attribute.

Release type has to be handled specially when the primary type is 'Album' and the secondary type list has values as defined in `SECONDARY_TYPE_ORDER`

samj1912 requested review from mayhem and mineo October 15, 2017 09:59

samj1912 commented Oct 15, 2017

View reviewed changes

mayhem approved these changes Oct 15, 2017

View reviewed changes

samj1912 force-pushed the solr61 branch from 527d4c0 to add2a50 Compare October 15, 2017 17:29

samj1912 mentioned this pull request Oct 15, 2017

SOLR-63: Bring Release Group to current search parity #42

Merged

samj1912 requested a review from mwiencek October 16, 2017 14:26

mineo reviewed Oct 16, 2017

View reviewed changes

samj1912 force-pushed the solr61 branch from add2a50 to c220fb9 Compare October 18, 2017 09:07

mwiencek approved these changes Oct 18, 2017

View reviewed changes

samj1912 added 7 commits October 18, 2017 19:44

Fix format output

1286085

Remove unnecessary type conversion

ebcb7d5

Fix release status output

55bf73a

Fix primary type output of a release

8cd71ed

Include related release artist credit only if its different

3adb7b1

Remove alias list from related release artist credit

b65d738

Remove medium list count as it leads to buggy JSON output

5472fbd

samj1912 added 5 commits October 18, 2017 19:44

Remove disc list from result output

807f1d1

Remove name tag from artist name credit output

58487f1

Fix logic for setting release type

e4c7861

Release type has to be handled specially when the primary type is 'Album' and the secondary type list has values as defined in `SECONDARY_TYPE_ORDER`

Add lru_cache to calculate_type

ffc2e8f

Fix calculate_type to utilize LRU cache properly and add tests

0cb62b4

samj1912 force-pushed the solr61 branch from c220fb9 to 0cb62b4 Compare October 18, 2017 18:20

samj1912 requested a review from mwiencek October 18, 2017 18:21

samj1912 merged commit 5788de8 into metabrainz:master Oct 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-61: Bring Recording to search parity #40

SOLR-61: Bring Recording to search parity #40

samj1912 commented Oct 15, 2017 •

edited

mayhem commented Oct 15, 2017

mayhem commented Oct 15, 2017

samj1912 Oct 15, 2017 •

edited

samj1912 Oct 15, 2017 •

edited

samj1912 Oct 15, 2017

samj1912 Oct 15, 2017 •

edited

samj1912 Oct 15, 2017

samj1912 Oct 15, 2017

samj1912 Oct 15, 2017

samj1912 Oct 15, 2017

samj1912 Oct 15, 2017

samj1912 Oct 15, 2017

mineo Oct 16, 2017

samj1912 Oct 17, 2017

samj1912 Oct 15, 2017

mayhem left a comment

samj1912 commented Oct 15, 2017

mineo Oct 16, 2017

samj1912 Oct 18, 2017

mineo Oct 16, 2017

		@@ -50,6 +58,28 @@ def datetime_to_string(obj):
		return obj.strftime(TIME_FORMAT)


		def calculate_type(primary_type, secondary_types):

SOLR-61: Bring Recording to search parity #40

SOLR-61: Bring Recording to search parity #40

Conversation

samj1912 commented Oct 15, 2017 • edited

mayhem commented Oct 15, 2017

mayhem commented Oct 15, 2017

samj1912 Oct 15, 2017 • edited

Choose a reason for hiding this comment

samj1912 Oct 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samj1912 Oct 15, 2017 • edited

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

mayhem left a comment

Choose a reason for hiding this comment

samj1912 commented Oct 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samj1912 commented Oct 15, 2017 •

edited

samj1912 Oct 15, 2017 •

edited

samj1912 Oct 15, 2017 •

edited

samj1912 Oct 15, 2017 •

edited