heh, alastairp is soo much more picky than me, I'd never find more stuff. ;)
2021-05-11 13150, 2021
_lucifer
lol :)
2021-05-11 13149, 2021
ruaok
potential supporters who are coming in from competing services ask the oddest questions. I suspect I would understand them better if I knew what sorts of evil things are in the contracts that they make their customers sign.
2021-05-11 13152, 2021
alastairp
_lucifer: I just mean that you use the constant `1` for the user id in 3 lines, just abstract it out into `user_id = 1`
2021-05-11 13156, 2021
_lucifer
oh ok. i have replied to most other comments. addressing most and opening tickets where it made sense.
2021-05-11 13110, 2021
alastairp
I see, thanks!
2021-05-11 13115, 2021
alastairp
going back through it now
2021-05-11 13100, 2021
alastairp
_lucifer: given that there's pending work to be done to upgrade to spotipy 2.18 with the youtube/spotify changes, maybe we just merge 2.17 now to close up the PR and get 2.18 done later in the week after the other changes are done?
2021-05-11 13108, 2021
_lucifer
alastairp, sure. that works.
2021-05-11 13133, 2021
alastairp
great
2021-05-11 13152, 2021
alastairp
_lucifer: what did we decide on the Dockerfile one? keep mkdir, but put it all in one place?
2021-05-11 13136, 2021
_lucifer
alastairp, yeah, i think that makes sense.
2021-05-11 13159, 2021
alastairp
I just noticed some failing tests on the BU 2.0 one, will take a look at those again
2021-05-11 13111, 2021
_lucifer
alastairp, turns out we do not update the last_updated field if there is no new listen, that looks wrong.
2021-05-11 13156, 2021
alastairp
interesting. I guess the function of this field is "the last time we tried to do something with it"
2021-05-11 13104, 2021
alastairp
not necessarily "the last time we successfully imported"
2021-05-11 13124, 2021
_lucifer
right, i too think its the former. in that case, last_updated should be updated irrespective of whether we got new listens?
2021-05-11 13136, 2021
alastairp
yes, I think so
2021-05-11 13158, 2021
alastairp
because we use it to order users to import by, right?
2021-05-11 13117, 2021
alastairp
with the current setup we'll always have non-imported people at the beginning?
2021-05-11 13129, 2021
_lucifer
my understanding is that with the current setup, we would have non imported people at end.
2021-05-11 13110, 2021
alastairp
ah, we order by latest_listened_at
2021-05-11 13155, 2021
_lucifer
ah yes, i think we only used last_updated field to display it to the user the last time we tried
2021-05-11 13155, 2021
alastairp
but in that case, we do want to show the last time we tried, even if we didn't get anything
2021-05-11 13158, 2021
alastairp
so let's change it, then?
2021-05-11 13153, 2021
_lucifer
yes, right. changing it.
2021-05-11 13123, 2021
alastairp
👍
2021-05-11 13144, 2021
_lucifer
the importer code could use some more integration testing (uhhh mocks) but that's for another day
2021-05-11 13105, 2021
_lucifer
alastairp, i think i have addressed all the comments. can you take a look again at the comments I have not marked as resolved. i think those can be looked again in the future in a different PR. let me know if you want any of those addressed in the current PR.
2021-05-11 13121, 2021
alastairp
will do
2021-05-11 13131, 2021
alastairp
_lucifer: 2 comments on the dockerfile pr
2021-05-11 13150, 2021
alastairp
I can make the changes depending on what we choose
2021-05-11 13104, 2021
_lucifer
yup, saw that. right, its a development image. do we even need labels in it?
2021-05-11 13126, 2021
alastairp
I think in general it's a good idea to have them. I'm going to change `name`
2021-05-11 13155, 2021
alastairp
but I don't think we need vcs-ref
2021-05-11 13119, 2021
_lucifer
yeah, let's remove the ones that can vary and keep the ones that are fixed.
2021-05-11 13153, 2021
alastairp
pushed, if you OK it I can merge
2021-05-11 13113, 2021
alastairp
merging master in to catch the node 16 upgrades
2021-05-11 13138, 2021
_lucifer
looks good to merge!
2021-05-11 13119, 2021
BrainzGit joined the channel
2021-05-11 13112, 2021
alastairp
hello BrainzGit, you're back!
2021-05-11 13150, 2021
alastairp
_lucifer: can you approve 1444 if you're happy with our decision there?
_lucifer: what's the motivation for docker-compose.spark.override? are there any cases where we want to use docker-compose.spark.yml without the override?
2021-05-11 13102, 2021
_lucifer
alastairp: we use the override in development and without override in testing.
2021-05-11 13123, 2021
_lucifer
we do not need volumes during testing, and the network is also different in the two cases.
2021-05-11 13136, 2021
alastairp
ah right, neat
2021-05-11 13103, 2021
_lucifer
one thing I was wondering is that we don't actually need 3 containers to run tests or develop locally everything can run inside the same one.
2021-05-11 13104, 2021
alastairp
at the same time? running multiple processes in a container controlled with docker-compose isn't easy
2021-05-11 13115, 2021
_lucifer
but i was unable to coordinate how to run all processes in the same one.
2021-05-11 13134, 2021
_lucifer
yeah, right. it turned out to be a mess. so i kept the 3 containers.
2021-05-11 13147, 2021
alastairp
yeah, better to keep the pattern of "if you want to run 3 things then start up 3 containers".
or maybe not. at first i thought, the error came from sentry but it now seems its an unhandled error on our end
2021-05-11 13124, 2021
alastairp
but now we've started using _prep_key in a few places in LB code because we want the global namespace. This comes into our discussion about if we should keep adding additional items to cache.py, or simplify it to just pass any function call down to the underlying redis
2021-05-11 13120, 2021
_lucifer
we should just pass it down to redis i think.
2021-05-11 13150, 2021
alastairp
I'll put out 2.0.1 with optional namespace for now, then
2021-05-11 13152, 2021
_lucifer
can we wrap the redis instance, so that it calls prep key first on the key and then just calls the redis function
2021-05-11 13126, 2021
alastairp
we can do that if we had a class, just override __getattr__ (I think). Not sure how to do it in a module
2021-05-11 13122, 2021
alastairp
we could also do introspection
2021-05-11 13135, 2021
alastairp
read all methods in _r and for each of them create a new named lambda in the current module which calls prep_key on the key beforehand
2021-05-11 13151, 2021
_lucifer
i think a class would make sense
2021-05-11 13111, 2021
_lucifer
let's open a ticket for now?
2021-05-11 13102, 2021
alastairp
oh yeah - this is not something I want to deal with immediately :)
2021-05-11 13141, 2021
alastairp
I think our idea was that we have 2 main use-cases: 1) we just want to throw _something_ into a cache and pick it up at a later point. In this case, keep a cache module with .set and .get. do encoding so that we can put in complex objects; 2) we want to actually use redis to store stuff. expose the Redis class instance, but with some additional nice features that allow us to add namespaces to the keys
2021-05-11 13105, 2021
_lucifer
right.
2021-05-11 13131, 2021
alastairp
cool - if you could put that in a ticket that would be great
ruaok, they tried to delete the listens before importing but were unsure whether it got deleted actually.
2021-05-11 13100, 2021
ruaok
"FWIW, I also tried to delete all of my existing listens prior to the import and they never seemed to go away, either. So, I'm wondering if a combination of things I did borked something."
alastairp: https://github.com/metabrainz/listenbrainz-server… , i think we should actually remove this commands. it does not make sense to have them in multiple places. the request consumer is the right place to have these commands.
2021-05-11 13130, 2021
_lucifer
ruaok: thoughts?
2021-05-11 13134, 2021
_lucifer
we have some commands in spark_manage.py to request stats and recommendations. these commands are in request consumer as well.
2021-05-11 13150, 2021
_lucifer
lets remove the duplicate ones from spark_manage.py
2021-05-11 13146, 2021
ruaok
yes, because we never really issue requests from the cluster. our setup is to drive the cluster from lemmy and only lemmy.
2021-05-11 13109, 2021
ruaok
if you dig into that, would it make sense to compress all the stats commands into one manage.py command?
2021-05-11 13121, 2021
_lucifer
sure.
2021-05-11 13134, 2021
ruaok
that will make our cron tab management a LOT easier.
2021-05-11 13143, 2021
ruaok
its quite messy right now.
2021-05-11 13159, 2021
alastairp
_lucifer: ah, right. I don't mind where we put them. if we remove from spark manage but they're already somewhere else then that's fine
2021-05-11 13108, 2021
_lucifer
also, looked into the spark docs for that command. those are outdated.
2021-05-11 13150, 2021
_lucifer
i have an idea to simplify this. put spark reader in the override file so that it comes up with spark containers in dev mode.
akshaaatt[m] uploaded a video: MusicBrainz (13917KiB) < https://matrix.org/_matrix/media/r0/download/matrix.org/eFJkTuxCQgIYeTusIMHdYkzL >
2021-05-11 13109, 2021
akshaaatt[m]
Hi _lucifer I had a great time updating the UI and cleaned up the code at a few places. I have found months worth of work in the meantime while going through the code as well. I will continue exploring more code and UI since I need to start with the documentation soon. I'll try to start with the Tagger next. I think this UI update clubbed with the one I did a few days back is great enough for the next month or so. I
2021-05-11 13109, 2021
akshaaatt[m]
will start with some codebase documentation and the tagger soon 😇
