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

Cache get_avatar_url on /followers page to avoid fetching every user full object #9282

Open
Tracked by #9281
mekarpeles opened this issue May 16, 2024 · 5 comments
Open
Tracked by #9281
Assignees
Labels
Lead: @mekarpeles Issues overseen by Mek (Staff: Program Lead) [managed] Needs: Response Issues which require feedback from lead Priority: 2 Important, as time permits. [managed] Theme: My Books Theme: Performance Issues related to UI or Server performance. [managed]

Comments

@mekarpeles
Copy link
Member

mekarpeles commented May 16, 2024

Currently, the My Followers & Following pages are making individual requests for every patron account on the page in order to fetch the internetarchive identifier required to craft their avatar url. Ideally, we'd be able to fetch these in bulk. At minimum, we should cache these values so they are more easily accessible.

https://openlibrary.org/people/mekBot/followers
https://openlibrary.org/people/mekBot/following

Screenshot 2024-05-16 at 6 25 34 AM

See: https://github.com/internetarchive/openlibrary/pull/8607/files#r1496013460

@mekarpeles mekarpeles changed the title Fix caching of get_avatar_url Cache get_avatar_url on /followers page to avoid network request per avatar fetch May 16, 2024
@mekarpeles mekarpeles changed the title Cache get_avatar_url on /followers page to avoid network request per avatar fetch Cache get_avatar_url on /followers page to avoid fetching every user full object May 16, 2024
@mekarpeles mekarpeles added Theme: Performance Issues related to UI or Server performance. [managed] Priority: 2 Important, as time permits. [managed] Theme: My Books labels May 16, 2024
@RayBB
Copy link
Collaborator

RayBB commented May 16, 2024

@mekarpeles I don't want to scope creep this.
But I've also talked with @cdrini about a bulk api for the works merge page as well. We need bulk ratings/bookshelves/lists/editions (so we can make fewer requests when doing a big merge). Making so many requests now currently seems to cause ratelimiting problems for librarians and puts them in the "slow lane".

Perhaps a more general solution should be considered when approaching this.

@github-actions github-actions bot added the Needs: Response Issues which require feedback from lead label May 17, 2024
@mekarpeles
Copy link
Member Author

@RayBB We already have ways of e.g. using ctx fetch_many, this is a specific case where we just need to change the code powering this API to make a single call for a batch of objects rather than a handful of individual calls.

@mekarpeles mekarpeles removed the Needs: Response Issues which require feedback from lead label May 18, 2024
@Vidyap23
Copy link

@RayBB could you please assign this to me?

@RayBB
Copy link
Collaborator

RayBB commented May 22, 2024

@Vidyap23 you're assigned. If you have questions be sure to tag Mek as he has the context on this one

@github-actions github-actions bot added the Needs: Response Issues which require feedback from lead label May 23, 2024
@mekarpeles mekarpeles added the Lead: @mekarpeles Issues overseen by Mek (Staff: Program Lead) [managed] label May 28, 2024
@Vidyap23
Copy link

Vidyap23 commented May 31, 2024

@mekarpeles I am currently working on this and after going through a few files where values are being cached, I am using memcache_memoize to cache the response. Test code below

def GET(self, username):
        def fetch_cached_avatar(username):
            print(username, "inside function avatar", flush= True)
            url = 'https://picsum.photos/id/237/200/300'
            # url = User.get_avatar_url(username)
            return url
        url = cache.memcache_memoize(
        fetch_cached_avatar,
        'user.avatar',
        timeout= 5 * dateutil.HOUR_SECS,
        )(username)
        raise web.seeother(url)

Is this the right approach since I am pretty new to the codebase and I am not sure how caching works with respect to singular requests in this backend. I was able to test this by adding followers and it looks like it was working when I disabled the browser cache too. Is there any other way to test this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Lead: @mekarpeles Issues overseen by Mek (Staff: Program Lead) [managed] Needs: Response Issues which require feedback from lead Priority: 2 Important, as time permits. [managed] Theme: My Books Theme: Performance Issues related to UI or Server performance. [managed]
Projects
None yet
Development

No branches or pull requests

3 participants