Skip to content

Signal-ranked Notable Authors for subject pages#13142

Open
Armansiddiqui9 wants to merge 8 commits into
internetarchive:masterfrom
Armansiddiqui9:Subject-notable-authors
Open

Signal-ranked Notable Authors for subject pages#13142
Armansiddiqui9 wants to merge 8 commits into
internetarchive:masterfrom
Armansiddiqui9:Subject-notable-authors

Conversation

@Armansiddiqui9

@Armansiddiqui9 Armansiddiqui9 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Closes #13135
Phase 1

Replaces "Prolific Authors" (raw author_key facet count) with a signal-ranked "Notable Authors" list, per Phase 1 of the subject page redesign epic.

Technical

  • One extra, bounded Solr query (100 rows, sorted by readinglog_count) scans works once; each author's first occurrence in that signal-sorted list becomes their representative work.
  • Exact per-author book counts are merged in from the existing author_key facet (already computed by the main query free).
  • Author photos are batch-fetched with a single site.get_many call, mirroring the existing decorate_with_tags pattern.

Files

  • openlibrary/plugins/worksearch/subjects.py - new get_notable_authors_async (SubjectEngine) and decorate_with_author_photos (page handler)
  • openlibrary/macros/SubjectAuthors.html - new macro (replaces ProlificAuthors.html, which is now unused and removed)
  • openlibrary/templates/subjects.html - swapped macro call
  • static/css/components/subject-authors.css - new card layout styles, built on existing design tokens
  • openlibrary/plugins/worksearch/tests/test_subjects.py new test coverage for both the ranking logic and the photo-decoration fallback

Known tradeoff

The 100-row sample bound means a very prolific author could theoretically be undercounted if none of their works land in the sample, unlikely in
practice since the sort already surfaces their best work early, but flagging it as a documented tradeoff rather than a silent gap.

Testing

  • Confirmed /subjects/science_fiction renders the new "Notable Authors" box correctly, author card with photo fallback (placeholder avatar, since local Solr data has no cover), representative work link
    ("Flatland"), and author name link, all functioning.
  • python -m ruff check
  • pytest openlibrary/plugins/worksearch/tests/test_subjects.py -v - all passed
    • Covers decorate_with_author_photos: photo present, photo missing,
      author thing missing entirely, empty/missing notable_authors list
    • Covers get_notable_authors_async: representative-work selection from a signal-sorted sample, MAX_NOTABLE_AUTHORS cap, and the no-matching-works (sparse subject) case

Not yet tested / left for reviewer awareness:

  • Real production-scale Solr data (local dev Solr is sparse per the epic's own note) representative work quality should be revisited once tested against fuller data.
  • Cross-browser/mobile responsiveness of the new card layout.

Screenshot

Screenshot 2026-07-12 214310

Stakeholders

@lokesh

@accesslint accesslint Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 2 issues across 2 rules (1 WCAG, 1 Best Practice).

Comment thread openlibrary/macros/SubjectAuthors.html
Comment thread openlibrary/macros/SubjectAuthors.html Outdated
@openlibrary-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

The linked issue hasn't been triaged yet — triage happens on Mondays and Fridays. There are currently 29 open non-draft PRs ahead of yours.

Possible improvements for this PR

  • CI is failingpre-commit.ci reports failures on the latest commit. See the pre-commit guide for running hooks locally before pushing.

Note: this PR uses Closes #13135, which is the tracking epic for the full 7-phase subject-page redesign, not a phase-specific sub-issue. Merging with a closing keyword will auto-close the epic even though only Phase 1 lands here — maintainers may want to change this to a non-closing reference (e.g. Part of #13135) before merge.

PR triage checklist (maintainers / Richy)
  • PR description — not empty; explains what the change does and how to verify it
  • References an issue — PR body contains a #NNN reference
    • Linked issue is triaged — has a Priority: * label (not just Needs: Feedback / Needs: Staff Decision)
    • Linked issue is assigned — has at least one assignee
  • Commit history clean — one logical commit + a pre-commit.ci auto-fix commit
  • CI passingpre-commit.ci check is failing
  • Test cases present — new coverage in test_subjects.py
  • Proof of testing — screenshot and manual verification steps included in the description

Note

This comment was automatically generated by PAM, Open Library's Project AI Manager. PAM provides status visibility, performs basic project management functions, and gives actionable feedback so contributors aren't left waiting.

— Richy, via PAM (Open Library's Project AI Manager)

@openlibrary-bot openlibrary-bot added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Jul 9, 2026
@lokesh

lokesh commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator
Screenshot 2026-07-09 at 10 58 54 PM

Took a pass at the Notable Authors display to bring it closer to the mockup — compact author cards instead of the single large avatar. (Changes pushed to lokesh:Subject-notable-authors.)

What changed:

  • Each author now renders as a small bordered card: portrait author thumbnail + name + representative work ("Title and N more").
  • The author thumbnail is a rounded rectangle matching the 131×180 placeholder aspect ratio, so the silhouette placeholder isn't cropped into a circle.
  • The whole card links to the author; a small cover of the representative work shows on the right when one is available.
  • Added cover_i to the notable-authors Solr query so that work cover can render.
  • Graceful fallbacks throughout: no photo → placeholder, no representative work → "N books" count, no cover → thumbnail omitted.
  • Dropped the "influential voices in this subject" tagline.
  • Added guards + tests for works missing key/title and multi-author works overshooting the author cap.

@lokesh

lokesh commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Just calling out that the new Author list does incur some backend cost. The breakdown is below and if the decision is made to minimize the cost, some solutions are posted below as well.


Notable Authors — Solr/DB cost & caching

What the new work costs

Extra Solr queryq=subject_facet:"x", sort=readinglog_count desc, rows=100, facet=false, 4 stored fields. This is cheaper than the main subject query it rides alongside: no facet computation (the main query faceting author_key/publisher/language + publishing-history is the expensive one), sorting is on a numeric docValues field with a size-100 heap, and the payload is 100 docs × 4 small fields. Order-of-magnitude: low tens of ms of Solr CPU on a warm index. The real cost isn't CPU — it's that it's a second round-trip added unconditionally to every subject render.

get_many author fetch — up to 8 author Things. One batched PK fetch, and individual Things are themselves memcached (@cache.memoize on the Thing model in core/models.py), so typically sub-10ms and often mostly memcache hits. Minor.

So per uncached subject page you're adding roughly one extra Solr round-trip (less than the existing one) plus one cheap batched fetch — call it a 30–50% bump in backend work for that page. Not alarming in isolation. The reason to care: subject pages are an SEO-crawled long tail — crawlers hit thousands of rarely-viewed subjects, so front-of-house edge caching has low hit rates exactly where this cost lands.

Mitigation

Since these lists change infrequently, this is a textbook fit for memcache_memoize, which is stale-while-revalidate: after the TTL it serves the old value instantly and recomputes on a background thread, so no user ever waits for the refresh (core/cache.py). Ranked:

A. Memoize the notable-authors computation, keyed by normalized subject path, long TTL (6–24h). (Do this one.) Collapses both the extra Solr query and the get_many into a single memcache GET on the hot path. Steady-state added cost drops to ~one memcache lookup — negligible — and it matches "changes infrequently" exactly.

Two implementation gotchas:

  • get_notable_authors_async is async; memcache_memoize is sync (value = self.f(...)). Memoize at a sync seam — a small sync helper that runs the async bridge and also folds in photo decoration, so photos get cached too (mirror how get_subject = async_bridge.wrap(get_subject_async) already bridges).
  • memcache round-trips through JSON, so web.storage comes back as a plain dict. The Templetor macro does $author.key / $author.name, which fails on a bare dict — so re-hydrate to web.storage on the read side before handing to the template. Easy to miss; would only surface in prod (dev uses a mock memcache).

B. Trim the sample. rows=100 is arbitrary; with MAX_NOTABLE_AUTHORS=8 over readinglog-sorted docs you almost always fill 8 unique authors within the first ~20–30 rows. rows=40 cuts scan/payload with no realistic quality loss. Cheap, stacks with A.

C. (Future, if needed) precompute offline. A periodic job writing notable_authors per subject into a table/Solr field. Overkill for Phase 1 — a fallback if caching ever proves insufficient.

Don't derive rep-works from the main query instead of a second query — the main query is paged to 12 works and sorted for the carousel, so you'd lose author coverage. The separate bounded query is the right structure; just cache it.

Recommendation: A (+ B as a freebie). That turns the steady-state added load into one memcache GET, proportional to how rarely this data changes.

@github-actions github-actions Bot removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Jul 12, 2026
@Armansiddiqui9 Armansiddiqui9 force-pushed the Subject-notable-authors branch from e69a846 to a8f2d68 Compare July 12, 2026 15:12
@Armansiddiqui9

Copy link
Copy Markdown
Contributor Author

Replaces ProlificAuthors.html (raw author_key facet count, no photo, no context) with a new SubjectAuthors macro showing signal-ranked authors with a portrait, a representative work, and graceful fallbacks when photo/work data is missing.

Backend (openlibrary/plugins/worksearch/subjects.py)

  • SubjectEngine.get_notable_authors_async - one bounded extra Solr query (rows=40, sorted by reading log) picks each author's first-seen work as their representative work, capped at MAX_NOTABLE_AUTHORS=8 (cap checked inside the inner author loop so a co-authored work can't overshoot it).
  • _compute_notable_authors_with_photos + get_cached_notable_authors - memcache_memoize-wrapped, 12h TTL, with a delegate.fakeload() guard for the background stale-while-revalidate refresh thread.
  • decorate_with_notable_authors - rehydrates cached plain dicts back into web.storage (memcache round-trips through JSON) and merges in exact per-author book counts from the facet computed fresh every request, so counts stay accurate even while the ranking data is intentionally cached.

Frontend (SubjectAuthors.html + subject-authors.css)

  • Compact bordered cards, portrait thumbnail, representative work + count blurb, optional trailing cover thumbnail. All fields degrade independently, no photo, no representative work, and no cover all fall back cleanly.
  • CSS uses existing design tokens throughout.

Tests: 21 tests covering decoration, caching seams, and the async ranking logic (malformed-doc guards, cap-overshoot protection, cover passthrough).

Adopted a few structural refinements from @lokesh's review branch

@github-actions github-actions Bot added the Needs: Response Issues which require feedback from lead label Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs: Response Issues which require feedback from lead

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Epic: Subject page redesign — a guided "genre dashboard"

3 participants