#metabrainz

/

      • Sophist-UK has quit
      • Sophist-UK joined the channel
      • BrainzGit
        [listenbrainz-server] release 03v-2022-08-16.0 has been published by 14github-actions[bot]: https://github.com/metabrainz/listenbrainz-serv...
      • Lotheric_ has quit
      • aerozol
        tandy1000: so much later sorry, but better late than never. Or so I hear!
      • The project’s on pause I think? Forum post: https://community.metabrainz.org/t/musicbrainz-...
      • Web version (seems pretty far along huh): https://musicbrainz-web.web.app/
      • Lotheric_ joined the channel
      • elomatreb[m]: Would you mind peeping at the figma and letting me know if I’ve addressed your points? If any others come to mind post them on the ticket any time. https://www.figma.com/file/ln8XiFLit634KC3YkUW2...
      • tandy1000
        <aerozol> "tandy: so much later sorry..." <- no worries, i meant the artist page redesign!
      • CatQuest
        lol I like hitbug is an anagram of github
      • lucifer
        yuzie: it was an issue with imports. i changed the import to match how React is imported in existing components and the test works now.
      • alastairp
        morning
      • lucifer waves
      • this changes a key "average_rating" from a number to a dict
      • in the case that we have no data, we used to return None. now that it's a dict, do you think we should still return None, or an empty dict, or a dict with the relevant fields, but None for values?
      • lucifer
        if there's some precedent in CB api, probably better to stick with that. otherwise i think None makes sense.
      • alastairp
        I don't think we have any specific pattern for this
      • lucifer
        instead of empty dict, count = 0, rating = None may be more sensible.
      • alastairp
        yeah, that's what I was thinking. it seems likely that as soon as the "average_rating" key is there, you're going to want to read into it. it seems that we're going to have to have another check at some time to see if there is no rating, but the question is where to do that
      • mmm, rating=None vs rating=0 is interesting
      • I don't think it's possible to rate something 0
      • lucifer
        yeah, iirc then we can only rate 1-5 then returning rate = 0 is probably fine but may be confusing sometimes.
      • alastairp
        yeah, I think None is correct because it explicitly indicates the absence
      • ansh
        Hi!
      • alastairp
        hi ansh :)
      • just looking through your stuff
      • lucifer
        alastairp: let me know if you have some time afterwards, wanted to discuss some points about misc stuff.
      • alastairp
        lucifer: sure, let's do it now
      • lucifer
        great.
      • when using kombu with librabbitmq, it segfaults if eventlet.monkey_patch is not invoked before making the connection. https://github.com/metabrainz/listenbrainz-serv...
      • we didn't notice it earlier because websockets already was doing this before we moved it to kombu.
      • alastairp
        ah, right. seems reasonable to put it in, then
      • lucifer
        issue is likely in librabbitmq, because i didn't encounter the issue yet when using it in spark request consumer where librabbitmq is not installed so its falling back to pyampq.
      • alastairp
        ah
      • is putting it in timescale_writer enough? will this cause problems in other places too? Do we need to put it somewhere more global?
      • lucifer
        we'll need to put it in each consumer. ts writer, spark reader, mbid mapper etc.
      • alastairp
        we don't need it in the webserver?
      • lucifer
        it should probably be fine. but will need to check if it interacts with uwsgi before putting it in the api.
      • alastairp
        yeah, good point
      • lucifer
        my main concern was that it isn't documented that this should happen or that monkey_patching is needed for use.
      • alastairp
      • lucifer
        i checked librabbitmq github, the main repo has received a few more commits recently but no release so will try installing from source to see if that helps.
      • we are currently using a forked version because main repo was unmaintained for many months.
      • alastairp
        right, it's not clear reading these pages how everything fits together and why this is specifically needed
      • lucifer
        indeed
      • alastairp
      • but to me this implies that you can choose a threading/events library too?
      • lucifer
        unsure. but it does seem to special behaviour for eventlet. https://docs.celeryq.dev/projects/kombu/en/stab...
      • alastairp
        yeah, right
      • lucifer
        oh
      • presence of eventlet disables librabbitmq
      • and it falls back to py-amqp. so we've never been using it in reality.
      • ansh
        alastairp: I am aware that the tests are failing in CB#453 and CB#455. It is because I added the part to show the relationships in their respective PR's only, because opening a separate PR for it was making it a bit complex.
      • BrainzBot
      • alastairp
        ansh: ok, no problem. These PRs look good, I was going to suggest to you that we merge them all into a single one and then deploy it on beta to test.
      • that should fix the tests, right?
      • let me finish looking through them, and we can do it today
      • lucifer
        but the thread is somewhat old, i'll manually set the library to be used and see what happens.
      • alastairp
        lucifer: ah, interesting
      • Pratha-Fish
        Freso: Hey, sorry I couldn't respond yesterday. I was travelling, so the connection just yeeted out of the blue
      • On the positive side, I am back from travelling! Trying my best to resume work at full force now 👌
      • alastairp
        Pratha-Fish: hi, how was your trip?
      • Pratha-Fish
        alastairp: It was great!
      • ansh
        alastairp: Yes, it would fix the tests
      • Pratha-Fish
        Did yoga for a few days :)
      • I suspect they had jammers at the yoga center for some reasons. So I was mostly out of touch in those few days
      • alastairp
        🤨
      • Pratha-Fish
      • It's some kind of yoga place that encourages disconnecting and meditating for a few days, so I guess it made sense
      • It was kinda peaceful without internet NGL
      • alastairp
        ansh: those PRs look fine. Do you want to go ahead and merge 453 and 455 into 452? then we can release to beta
      • I see a few new files that you created have tab indentation instead of spaces, can you check those and fix the necessary files?
      • ansh
        Sounds good
      • I'll fix the indentation issues
      • alastairp
        Pratha-Fish: do you want to talk about what you're up to? What you're planning on doing, and if you think that we should extend the finish date of the project?
      • lucifer
        alastairp: another thing i was thinking about MsB future. since we now only have track_name, artist_name and release_name as fields. we shouldn't even need a hash.
      • alastairp
        hmm. just a unique index over the 3 columns?
      • lucifer
        yup
      • fwiw, we can't have a unique index in PG because of historical duplicates but can try to emulate the same manually.
      • alastairp
        right
      • lucifer
        should i go ahead with implementing this?
      • alastairp
        yeah, sounds good. how do you plan to work around the duplicates?
      • lucifer
        i think we can keep the duplicates around. we can create a non-unique index. for incoming listens, do a where = 3 fields to see if a msid already exists for that data, if so return it otherwise insert a new one.
      • alternatively we can add a msid_redirect table but not sure if its worth it.
      • ansh
        alastairp: I have fixed indentation issues in both the PRs. Now we can merge them in order, first 453 and then 455. But there would be some merge conflicts I see because of the new way to filter reviews on the reviews page.
      • alastairp
        ansh: because they are all on your fork, you should be able to do the merge yourself. Can you merge them into #452 yourself, fix the conflicts, and update the sql migration files so that there is just 1 for the 3 new entities?
      • lucifer: as long as that's as easy/fast as the hash column, that sounds fine
      • I don't think a redirect table is useful
      • ansh
        Okay, I'll merge them
      • lucifer
        makes sense 👍
      • alastairp -> lunch
      • Pratha-Fish is back with coffee
      • Pratha-Fish
        alastairp: yes definitely
      • rozlav joined the channel
      • ansh
        alastairp: I've merged both the PR's into 452.
      • BrainzGit
        [listenbrainz-server] 14amCap1712 opened pull request #2099 (03master…no-decode-ws): Do not try to decode message in ListensDispatcher https://github.com/metabrainz/listenbrainz-serv...
      • lucifer
        alastairp: oh and another thing i realised was that we could probably retire the mbid_mapping_metadata once we start updating mb_metadata_cache table regularly.
      • this would be beneficial in multiple ways, one that afair we don't update mbid_mapping_metadata (unless a new listen matches the exact mbid resulting in an upsert). so the mbid_mapping_metadata isn't updated automatically after MB edits whereas the mb_metadata_cache will be so more accurate data.
      • rozlav has quit
      • secondly, mb_metadata_cache has more recordings in it. there are some use cases in LB where it can be helpful (LB#2098)
      • BrainzBot
        Reduce logging severity for mbid item not found in mbid_metadata: https://github.com/metabrainz/listenbrainz-serv...
      • alastairp
        ansh: thanks
      • lucifer: yes, using mb_metadata_cache makes sense, as long as we can select the data out of it as quickly as the mapping metadata table (it's much much larger, so I'm not sure if that'll have an effect on speed)
      • ansh
        alastairp: for CB#457, I made the change to return ratings: null, count: 0
      • BrainzBot
        feat: Return average ratings count in API: https://github.com/metabrainz/critiquebrainz/pu...
      • alastairp
        ansh: ok, great
      • ansh: what does this # column mean?
      • ansh
        Its just the loop counter
      • alastairp
        I don't think that it's necessary in these lists, otherwise it seems that there is some importance to this ordering (I don't think that there is, is there?)
      • however, I guess works in a series might make sense to have it?
      • ansh
        Yep, in series it would
      • alastairp
        ansh: a few other things I noticed: I don't think that "Ordering Type" is useful in series search, I'd remove that column.
      • ansh: https://bookbrainz.org/series/e6f48cbd-26de-4c2... I notice here that the items are in order, but in http://localhost:8200/series/e6f48cbd-26de-4c2e... they're in a different order
      • tell me if you would prefer these comments in the PRs
      • ansh
        Maybe then we could replace by the series total Items count?
      • alastairp
        sure, that sounds fine. We don't always have to have 4 columns though, maybe 3 is enough in some cases
      • ansh
        Let me check the BB code again to see how it is sorted
      • alastairp
        does the "list of works" on an author page have paging? I see some examples that have ~30 works. what's the maximum number of works for an author in the bb database?
      • ansh
        Not right now
      • alastairp
        ok 👍
      • ansh
        alastairp: We cannot put this PR on beta. Because the production code would not know how to handle the new entities. It would result in crashing similar to what happened last time.
      • alastairp
        right, good point
      • maybe we should set up a full test environment which has a separate database
      • ansh
        Yes, I was thinking of the same
      • alastairp
        but otherwise, I'm testing this now, it looks really good. I think that we can go straight to the production deployment
      • ansh
        Thats nice
      • alastairp
      • these colours look a bit difficult
      • ansh
        Yeah, we need to change this.
      • alastairp
        we can probably wait for the new design, I was talking to monkey about that
      • ansh
        yes that sounds good
      • alastairp
        ansh: I've tried entity view/write review/view review for all 3 new entity types, and it looks fine to me. no obvious bugs
      • let's fix these few small issues (series work ordering, remove # from artist work list, remove ordering type from series search) and then we can release
      • ansh
        Sure! I'll make these changes
      • lucifer
        alastairp: with an index, i think it shouldnt matter much but yes makes sense to measure.
      • ansh
        alastairp: For series work ordering, I think it requires some time. BB does it differently which I am not able to figure out at the moment. eg: https://bookbrainz.org/series/968ef651-6a70-410... I'll open a separate PR for it
      • alastairp
        ansh: right, it looks like there is an internal ordering, but there is also the label which says where it comes. let me have a quick look
      • elomatreb[m]
        aerozol: looks great
      • ansh
        alastairp: Were you able to find how it is working?
      • alastairp
        ansh: no, I'm looking through the code and I can't find the bit that makes the query
      • ansh
        alastairp: Let's leave it for now.
      • I'll make a separate PR later.
      • yvanzo
        al-bondigas: It should be "== 1" according to https://github.com/metabrainz/musicbrainz-docke...
      • BrainzGit
        [critiquebrainz] 14alastair merged pull request #452 (03master…add_bb_work): Allow CB to review Literary Works https://github.com/metabrainz/critiquebrainz/pu...
      • [critiquebrainz] 14alastair merged pull request #455 (03master…add_bb_series): Allow CB to review Series https://github.com/metabrainz/critiquebrainz/pu...
      • [critiquebrainz] 14alastair merged pull request #453 (03master…add_bb_author): Allow CB to review Authors https://github.com/metabrainz/critiquebrainz/pu...