Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PAY-3653] Re-land feed performance improvements #10741

Merged
merged 2 commits into from
Dec 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,8 @@ def test_get_feed_es(app):
assert feed_results[0]["playlist_id"] == 1
assert feed_results[0]["save_count"] == 1
assert len(feed_results[0]["followee_reposts"]) == 1
assert len(feed_results[0]["tracks"]) == 1
assert feed_results[0]["tracks"][0]["track_id"] == 1

assert feed_results[1]["track_id"] == 1
assert feed_results[1]["save_count"] == 1
Expand Down

This file was deleted.

2 changes: 2 additions & 0 deletions packages/discovery-provider/src/api/v1/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -606,6 +606,8 @@ def extend_feed_item(item):
}
if item.get("playlist_id"):
extended_playlist = extend_playlist(item)
tracks = list(map(extend_track, item.get("tracks", [])))
extended_playlist["tracks"] = tracks
return {
"type": "playlist",
"item": extended_playlist,
Expand Down
4 changes: 2 additions & 2 deletions packages/discovery-provider/src/api/v1/models/feed.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

from .common import ns
from .extensions.models import OneOfModel
from .playlists import full_playlist_without_tracks_model
from .playlists import full_playlist_model
from .tracks import track_full

track_feed_item = ns.model(
Expand All @@ -17,7 +17,7 @@
"playlist_feed_item",
{
"type": fields.String(required=True),
"item": fields.Nested(full_playlist_without_tracks_model, required=True),
"item": fields.Nested(full_playlist_model, required=True),
},
)

Expand Down
77 changes: 39 additions & 38 deletions packages/discovery-provider/src/queries/get_feed_es.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
from src.models.tracks.track import Track
from src.queries.query_helpers import (
_populate_gated_content_metadata,
filter_hidden_tracks,
Expand Down Expand Up @@ -123,6 +122,7 @@ def get_feed_es(args, limit=10, offset=0):
# track timestamps and duplicates
seen = set()
unsorted_feed = []
playlist_track_ids = set()

for playlist in playlists:
# Q: should es-indexer set item_key on track / playlist too?
Expand All @@ -143,6 +143,7 @@ def get_feed_es(args, limit=10, offset=0):
# supress individual tracks from said album appearing in feed
for track in playlist["tracks"]:
seen.add(item_key(track))
playlist_track_ids.add(track["track_id"])

for track in tracks:
track["item_key"] = item_key(track)
Expand Down Expand Up @@ -186,8 +187,6 @@ def get_feed_es(args, limit=10, offset=0):
else:
mget_reposts.append({"_index": ES_PLAYLISTS, "_id": id})

repost_playlist_track_ids = set()

if mget_reposts:
reposted_docs = esclient.mget(docs=mget_reposts)
for doc in reposted_docs["docs"]:
Expand All @@ -208,56 +207,55 @@ def get_feed_es(args, limit=10, offset=0):
# MISSING: skip reposts for delete, private, unlisted, stem_of, is_stream_gated
# this is why we took soft limit above
continue
keyed_reposts[s["item_key"]] = s

if "playlist_id" in s:
track_ids = set(
map(
lambda t: t["track"],
s.get("playlist_contents", {}).get("track_ids", []),
)
)
repost_playlist_track_ids = repost_playlist_track_ids.union(track_ids)
for track in s["playlist_contents"]["track_ids"]:
playlist_track_ids.add(track["track"])
keyed_reposts[s["item_key"]] = s

# get hidden track ids in reposted playlists
hidden_playlist_track_ids = []
db = get_db_read_replica()
if repost_playlist_track_ids:
with db.scoped_session() as session:
hidden_playlist_track_ids = (
session.query(Track.track_id)
.filter(Track.track_id.in_(list(repost_playlist_track_ids)))
.filter(Track.is_unlisted == True)
.all()
)
hidden_playlist_track_ids = [t[0] for t in hidden_playlist_track_ids]
# attach playlist tracks
playlist_tracks = (
esclient.mget(index=ES_TRACKS, ids=list(playlist_track_ids))
if len(playlist_track_ids) > 0
else []
)
playlist_tracks_by_id = (
{d["_id"]: d["_source"] for d in playlist_tracks["docs"] if d["found"]}
if len(playlist_track_ids) > 0
else {}
)

# replace repost with underlying items
sorted_feed = []
for x in sorted_with_reposts:
if "min_created_at" not in x:
x["activity_timestamp"] = x["created_at"]
sorted_feed.append(x)
item = x
item["activity_timestamp"] = x["created_at"]
else:
k = x["key"]
if k not in keyed_reposts:
# MISSING: see above
continue
item = keyed_reposts[k]

# exclude reposted playlists with only hidden tracks and empty reposted playlists
if "playlist_id" in item:
track_ids = set(
map(
lambda t: t["track"],
item.get("playlist_contents", {}).get("track_ids", []),
)
item["activity_timestamp"] = x["min_created_at"]["value_as_string"]

if "playlist_id" in item:
track_ids = list(
map(
lambda t: t["track"],
item.get("playlist_contents", {}).get("track_ids", []),
)
if all([t in hidden_playlist_track_ids for t in track_ids]):
continue
)
tracks = []
for track_id in track_ids:
if str(track_id) in playlist_tracks_by_id:
tracks.append(playlist_tracks_by_id[str(track_id)])
# exclude reposted playlists with only hidden tracks
if len(tracks) > 0 and all([t["is_unlisted"] for t in tracks]):
continue
item["tracks"] = tracks

item["activity_timestamp"] = x["min_created_at"]["value_as_string"]
sorted_feed.append(item)
sorted_feed.append(item)

# attach users
user_id_set = set([str(id) for id in get_users_ids(sorted_feed)])
Expand All @@ -274,6 +272,9 @@ def get_feed_es(args, limit=10, offset=0):
# GOTCHA: es ids must be strings, but our ids are ints...
uid = str(item.get("playlist_owner_id", item.get("owner_id")))
item["user"] = user_by_id[uid]
if "playlist_id" in item:
for track in item["tracks"]:
track["user"] = user_by_id[str(track["owner_id"])]

(follow_saves, follow_reposts) = fetch_followed_saves_and_reposts(
current_user, sorted_feed
Expand Down Expand Up @@ -301,8 +302,8 @@ def get_feed_es(args, limit=10, offset=0):
sorted_feed,
)
)

# batch populate gated track and collection metadata
db = get_db_read_replica()
with db.scoped_session() as session:
_populate_gated_content_metadata(session, sorted_feed, current_user["user_id"])

Expand Down
4 changes: 4 additions & 0 deletions packages/discovery-provider/src/queries/query_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -791,6 +791,8 @@ def getContentId(metadata):
] = has_download_access

for entity in entities:
if "playlist_id" in entity and "tracks" in entity:
_populate_gated_content_metadata(session, entity["tracks"], current_user_id)
content_id = getContentId(entity)
if content_id not in gated_content_ids:
entity[response_name_constants.access] = {
Expand Down Expand Up @@ -1410,6 +1412,8 @@ def get_users_ids(results):
for result in results:
if "playlist_owner_id" in result:
user_ids.append(int(result["playlist_owner_id"]))
for track in result.get("tracks", []):
user_ids.append(int(track["owner_id"]))
elif "owner_id" in result:
user_ids.append(int(result["owner_id"]))
# Remove duplicate user ids
Expand Down
10 changes: 9 additions & 1 deletion packages/discovery-provider/src/utils/elasticdsl.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,15 @@ def populate_track_or_playlist_metadata_es(item, current_user):
else:
item["has_current_user_reposted"] = False
item["has_current_user_saved"] = False

item["followee_reposts"] = item.get("followee_reposts", [])
item["followee_saves"] = item.get("followee_saves", [])

if "playlist_id" in item and "tracks" in item:
item["tracks"] = [
populate_track_or_playlist_metadata_es(track, current_user)
for track in item["tracks"]
]
return omit_indexed_fields(item)


Expand All @@ -87,7 +96,6 @@ def populate_track_or_playlist_metadata_es(item, current_user):
"following_ids",
"follower_ids",
"subscribed_ids",
"tracks",
# track index
"reposted_by",
"saved_by",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@
*/

import { exists, mapValues } from '../runtime';
import type { PlaylistFullWithoutTracks } from './PlaylistFullWithoutTracks';
import type { PlaylistFull } from './PlaylistFull';
import {
PlaylistFullWithoutTracksFromJSON,
PlaylistFullWithoutTracksFromJSONTyped,
PlaylistFullWithoutTracksToJSON,
} from './PlaylistFullWithoutTracks';
PlaylistFullFromJSON,
PlaylistFullFromJSONTyped,
PlaylistFullToJSON,
} from './PlaylistFull';

/**
*
Expand All @@ -35,10 +35,10 @@ export interface PlaylistFeedItem {
type: string;
/**
*
* @type {PlaylistFullWithoutTracks}
* @type {PlaylistFull}
* @memberof PlaylistFeedItem
*/
item: PlaylistFullWithoutTracks;
item: PlaylistFull;
}

/**
Expand All @@ -63,7 +63,7 @@ export function PlaylistFeedItemFromJSONTyped(json: any, ignoreDiscriminator: bo
return {

'type': json['type'],
'item': PlaylistFullWithoutTracksFromJSON(json['item']),
'item': PlaylistFullFromJSON(json['item']),
};
}

Expand All @@ -77,7 +77,7 @@ export function PlaylistFeedItemToJSON(value?: PlaylistFeedItem | null): any {
return {

'type': value.type,
'item': PlaylistFullWithoutTracksToJSON(value.item),
'item': PlaylistFullToJSON(value.item),
};
}

9 changes: 0 additions & 9 deletions packages/web/src/common/store/lineup/sagas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import { Uid, makeUids, makeUid, removeNullable } from '@audius/common/utils'
import {
all,
call,
delay,
put,
fork,
select,
Expand All @@ -42,7 +41,6 @@ import {
} from 'typed-redux-saga'

import { getToQueue } from 'common/store/queue/sagas'
import { isMobileWeb } from 'common/utils/isMobileWeb'
import { isPreview } from 'common/utils/isPreview'
import { AppState } from 'store/types'

Expand Down Expand Up @@ -193,13 +191,6 @@ function* fetchLineupMetadatasAsync<T extends Track | Collection>(
)
)

// Let page animations on mobile have time to breathe
// TODO: Get rid of this once we figure out how to make loading better
const isNativeMobile = yield* getContext('isNativeMobile')
if (!isNativeMobile && isMobileWeb()) {
yield* delay(100)
}

const lineupMetadatasResponse: LineupEntry<T>[] = yield* call(
lineupMetadatasCall,
action
Expand Down
4 changes: 1 addition & 3 deletions packages/web/src/common/store/pages/feed/lineup/sagas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,16 +74,14 @@ function* getTracks({
const feed = transformAndCleanList(data, userFeedItemFromSDK).map(
({ item }) => item
)

if (feed === null) return null
const filteredFeed = feed.filter((record) => !record.user.is_deactivated)
const [tracks, collections] = getTracksAndCollections(filteredFeed)
const trackIds = tracks.map((t) => t.track_id)

// Process (e.g. cache and remove entries)
const [processedTracks, processedCollections] = (yield* all([
processAndCacheTracks(tracks),
processAndCacheCollections(collections, true, trackIds)
processAndCacheCollections(collections, false)
])) as [LineupTrack[], Collection[]]
const processedTracksMap = processedTracks.reduce<Record<ID, LineupTrack>>(
(acc, cur) => ({ ...acc, [cur.track_id]: cur }),
Expand Down