#metabrainz

/

      • BrainzGit
        [musicbrainz-android] 14akshaaatt opened pull request #119 (03master…dashboard-revamp): Implement a fresh Homepage and Fix ListensActivity.kt https://github.com/metabrainz/musicbrainz-andro...
      • [musicbrainz-android] 14akshaaatt merged pull request #119 (03master…dashboard-revamp): Implement a fresh Homepage and Fix ListensActivity.kt https://github.com/metabrainz/musicbrainz-andro...
      • MRiddickW joined the channel
      • MRiddickW has quit
      • hackerman8 joined the channel
      • hackerman has quit
      • hackerman8 is now known as hackerman
      • KevlarNoir joined the channel
      • -- BotBot disconnected, possible missing messages --
      • -- BotBot disconnected, possible missing messages --
      • BrainzBot joined the channel
      • SothoTalKer has quit
      • SothoTalKer joined the channel
      • [listenbrainz-server] 14amCap1712 opened pull request #1984 (03crossvalidation…switch-playcounts): Add option to switch between playcounts and transformed listencounts https://github.com/metabrainz/listenbrainz-serv...
      • [listenbrainz-server] 14amCap1712 opened pull request #1985 (03switch-playcounts…custom-confidence-func): Apply a transformation function on listen counts before calculating confidence in CF https://github.com/metabrainz/listenbrainz-serv...
      • texke has quit
      • ZaphodBeeblebrox is now known as CatQuest
      • CatQuest is now known as dusjekatt
      • odnes joined the channel
      • alastairp
        morning
      • lucifer
        morning!
      • mayhem
        mooin!
      • Ansh
        morning!
      • yvanzo
        O’Moin
      • atj
        🥱
      • Ansh
      • As you mentioned in CB-434, we don’t need all these tabs. Also since we don’t have anything called “other” in our database, we can just have a parameter `include_events_with_no_type`.
      • BrainzBot
        CB-434: Show events for a place https://tickets.metabrainz.org/browse/CB-434
      • Ansh
        Then in CB, when `other` tab is called, It would request all other event types like Convention/Expo, Launch event, Award ceremony, etc. with parameter include_events_with_no_type as True from BU. So in this case, I don’t feel that having a default case, and custom True and false case would be of much use.
      • Also for CB, as mentioned, that we’ll have a tab for Stage Performance. I don’t think it is a good idea because when I checked in the MB database, there were only 130 stage performance events.
      • alastairp
        hi
      • yeah, regarding Stage Performance, reosarevok suggested it as a "perhaps we could add it", but if there are so few, let's omit it
      • so what do you think that the behaviour of the "events with no type" should be?
      • BrainzGit
        [listenbrainz-server] 14amCap1712 merged pull request #1975 (03master…experiment-tvs): Use TrainValidationSplit for tuning ALS Model https://github.com/metabrainz/listenbrainz-serv...
      • Ansh
        So when the parameter is true, it would include events with null type and the event for list passed.
      • alastairp
        and what would you set the default value of the parameter to?
      • Ansh
        False
      • So basically in CB, for Concert and festivals, the value would be false. For other events, a list of event_types would be passed, and the parameter would be true.
      • lucifer
        mayhem: need to do this PR first, all others are based on it. https://github.com/metabrainz/listenbrainz-serv...
      • mayhem
        on it
      • lucifer
        thanks! 👍
      • alastairp
        Ansh: right, let's try that then. it'll be simpler than what I suggested
      • Ansh: could you add a reply to that comment saying that we discussed it and decided on a different behaviour (so that in the future if we want to remember why we did it like that we have a record of the discussion)
      • Ansh
        Okay i'll reply to it
      • mayhem
        lucifer: on #1976 what is the parallelism parameter?
      • lucifer
        mayhem: how many trainings to do concurrently. total trainings is nFolds * nRanks * nAlpha * nLambda * nItrerations.
      • mayhem
        that would change if we change the number of nodes in our cluster?
      • KevlarNoir has quit
      • lucifer
        upto us how much we want to set it to. default is 1. i had tried 3 once and it went fine. there's a probably a limit to how much we can increase it before it OOMs.
      • mayhem
        ok, should be fine for now then.
      • lucifer
        👍
      • BrainzGit
        [listenbrainz-server] 14amCap1712 merged pull request #1976 (03master…crossvalidation): Replace TrainValidationSplit with CrossValidation https://github.com/metabrainz/listenbrainz-serv...
      • riksucks
        hi monkey, are you up?
      • you know how there is a trash icon besides events in the feed page, and now there would be an eye icon too. I was thinking of creating a function that would conditionally render, instead of filling the main code with lots of ternary operators
      • I was wondering, what should I name the function, something like `eventSideButton` should work right?
      • odnes has quit
      • dusjekatt is now known as CatQuest
      • BrainzGit
        [listenbrainz-server] 14amCap1712 merged pull request #1984 (03master…switch-playcounts): Add option to switch between playcounts and transformed listencounts https://github.com/metabrainz/listenbrainz-serv...
      • Etua joined the channel
      • lucifer
        mayhem: monkey: alastairp: any PRs to merge? will do a release soon
      • alastairp
        no, don't think so. although this week I want to finish the py3.10 pr and the cb reviews one
      • lucifer
        sounds good 👍
      • BrainzGit
        [listenbrainz-server] 14amCap1712 merged pull request #1985 (03master…custom-confidence-func): Apply a transformation function on listen counts before calculating confidence in CF https://github.com/metabrainz/listenbrainz-serv...
      • lucifer
        alastairp: oh actually https://github.com/metabrainz/listenbrainz-serv... looks ready to me
      • alastairp
        oh yeah, I saw your approval. I tested it locally with my own consul server. do you want to go ahead and try it?
      • lucifer
        but that'll require to brind timescale down no?
      • alastairp
        sorry, I don't mean "try that the bring-down functionality works", just "let's try and release it"
      • lucifer
        (i do not have a local setup for consul currently so need to do on test)
      • ah ok 👍
      • alastairp
        but you're right, we could deploy on test and change the config
      • lucifer
        yup can change the config there but the scenario that broke it last time won't be triggered if timescale is up
      • BrainzGit
        [listenbrainz-server] 14amCap1712 merged pull request #1980 (03master…consul-config-listenstore): Use consul-template if-statement to check if the listenstore is up https://github.com/metabrainz/listenbrainz-serv...
      • alastairp
        right. that shouldn't happen any more. if consul setting is "true" then we get real config, if it's "false" then we get empty config. we'll never get SERVICEDOESNOTEXIST if the config is "false"
      • so we don't need to test shutting down the timescale server
      • lucifer
        makes sense
      • alastairp
        let me prep a PR for docker-server-configs to change the setting values to json bool
      • lucifer
        i'll release first on beta anyway to make sure config loads fine.
      • thanks!
      • alastairp: another question, is there any use of getattr here if the second arg is always fixed. getattr(payload, 'data'), this should be equal to payload.data afaiu but there are many instances of such code in LB so wanted to confirm.
      • alastairp
        if payload.data doesn't exist on the payload object, then that's an exception. getattr will return None instead
      • in which case is this used? if it's on dynamic data then maybe getattr makes sense? but if it's a fixed class that we control then maybe not
      • lucifer
        but there is no fallback defined so getattr will raise exception too for this. like:
      • many instances in this file.
      • its being used on instance of pydantic model
      • alastairp
        wait, what?
      • I thought the default of getattr was None, always?
      • like dictionary.get()
      • lucifer
        i think the 3rd arg needs to be specified for it manually but i haven't used it much so not sure
      • alastairp
        yes, I just read the docs. whoops, I think I've written lots of buggy code :)
      • lucifer
        hehe lol
      • alastairp
        ok, looking through the code and the commits that added it, I see that it was changed from a dictionary to an object
      • expected_top_artist_recommendations = self.user_recommendations['recording_mbid']['top_artist'][:25]
      • expected_top_artist_recommendations = getattr(self.user_recommendations, 'recording_mbid').dict()['top_artist'][:25]
      • in which case, my feeling is that there wasn't a clear reason to do it like this, maybe we just saw that there was a 'string' and wanted to keep it like that
      • lucifer
        i see, makes sense. i'll remove these redundant getattr's then. they have always irked me ;)
      • Etua has quit
      • alastairp
        I guess it makes sense if you need a name which changes. e.g. `arg = 'artist_mbid' if do_artist else 'recording_mbid'; getattr(foo, arg)`
      • but if it's always known, then let's just get the objects directly
      • lucifer
        yup right.
      • alastairp
        lucifer: docker-server-confs PR made, affecting beta and test
      • if you merge that then release, then you should be able to test it
      • change true -> false to take down listenstore
      • once you're ready to deploy on prod, make the same change to the prod config file and deploy it at the same time
      • I'm going to go and look for some lunch, back soon
      • odnes joined the channel
      • mayhem
        lucifer: no PRs from me.
      • Etua joined the channel
      • zas
        yvanzo: I just restarted sir-prod on floyd
      • it created zombies processes and was outputting errors like: https://www.irccloud.com/pastebin/TGnblNH6/
      • yvanzo
        Ok, thanks
      • lucifer
        mayhem: 👍
      • BrainzGit
        [listenbrainz-server] release 03v-2022-05-09.0 has been published by 14github-actions[bot]: https://github.com/metabrainz/listenbrainz-serv...
      • lucifer
        alastairp: thanks!
      • mayhem: with the latest listened at stuff in recommendations, we can add a troi config or patch to filter out recs that have been already listened in past X days.
      • then create a playlist of only not listened ones.
      • mayhem
        I was wondering if we should remove the listened tracks from the candidate sets on the spark side of things.
      • but this can work for now...
      • lucifer
        would be interesting to try that as well.
      • yvanzo
      • BrainzBot
        SEARCH-671: Live indexing errors on track update and fails to recover
      • reosarevok
        yvanzo: back after the weekend
      • Where did we leave it? I saw you did some reviews during the weekend, thanks!
      • alastairp: my worry with stage performance would be that stuff like opera houses might show as empty otherwise
      • But I guess as long as you default to showing other if there's no concerts nor festivals, it should work fine
      • yvanzo
        reosarevok: It seems that Selenium tests are not passing anymore.
      • alastairp
        reosarevok: hmm, interesting point about showing Other if there are no concerts, that makes some level of sense
      • Ansh: what do you think? ^
      • yvanzo
        reosarevok: I updated https://github.com/metabrainz/musicbrainz-serve... to make CI tests pass, but it has some unresolved conversations.
      • alastairp
        the idea would be to see if there is no query parameter set to show the type of event - first try and show Concerts, but if there are 0 left, then try and load festivals, but if there are 0 then show the "Other" tab
      • reosarevok
        I think that makes sense (for all entities, so if you have no albums I'd show singles, too)
      • yvanzo: wtf "08006 DBI connect('dbname=musicbrainz_test;host=localhost;port=5432','musicbrainz',...) failed: FATAL: role "musicbrainz" does not exist"
      • Do we need to build some new selenium something-something?
      • yvanzo
        Not sure where does it come from but it is actually not a fatal error.
      • reosarevok
        Oh
      • So I guess it's expected, just weird
      • yvanzo
        👍
      • (to expected, and to weird)
      • reosarevok: My guess is that some PR that has been merged was missing to update some Selenium test as well.
      • reosarevok
        Does it always fail on this starman thing?
      • I will check
      • Seems so
      • Ok, checking locally
      • yvanzo
        Thanks, I cannot run Selenium tests locally atm.
      • monkey
        riksucks: Hi! Yes, I think extracting that code in a separate method would make everything clearer and more readable. Maybe `renderEventActionButton` or something?
      • lucifer
        LB prod updated.
      • monkey
        Thanks lucifer, sorry I forgot to answer. Will have PRs ready soon but none today :)
      • lucifer
        monkey: no worries, sounds good 👍