Make the non-BISAC nonfiction reset re-runnable (PP-5129) - #3736
dbernstein wants to merge 4 commits into
Conversation
79b2cb7 to
af7c1b7
Compare
|
Claude finished @dbernstein's task in 6m 56s —— View job Code review
SummaryThe design holds up under checking. DetailsMinor:
|
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| class ResetNonBisacNonfictionSubjectsScript(Script): | ||
| """Manually dispatch the ``reset_non_bisac_nonfiction_subjects`` Celery task. | ||
|
|
||
| The work itself happens in the Celery task; this script just queues it. It | ||
| exists so the repair in migration 52d1bbdd4671 can be applied again, in | ||
| case its reset was consumed by old code before the new classifier was live. | ||
|
|
||
| TODO: Remove this script when the ``reset_non_bisac_nonfiction_subjects`` | ||
| Celery task is removed. | ||
| """ | ||
|
|
||
| def do_run(self, *args: Any, **kwargs: Any) -> None: |
There was a problem hiding this comment.
This adds ResetNonBisacNonfictionSubjectsScript under src/palace/manager/scripts, but the repository explicitly marks this legacy CLI package as deprecated and prohibits new code there. Move this operational entry point to the supported command framework. This repository requirement must be satisfied before merging.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| reset_non_bisac_nonfiction_subjects.delay() | ||
| self.log.info( | ||
| 'The "reset_non_bisac_nonfiction_subjects" task has been queued for ' | ||
| "execution. See the celery logs for details about task execution." | ||
| ) |
There was a problem hiding this comment.
The command discards the AsyncResult returned by .delay() and logs only the static task name. An operator therefore cannot correlate this repair invocation with worker logs or distinguish it from another run. Retain and log the task ID to make execution of this repair verifiable.
| reset_non_bisac_nonfiction_subjects.delay() | |
| self.log.info( | |
| 'The "reset_non_bisac_nonfiction_subjects" task has been queued for ' | |
| "execution. See the celery logs for details about task execution." | |
| ) | |
| result = reset_non_bisac_nonfiction_subjects.delay() | |
| self.log.info( | |
| 'The "reset_non_bisac_nonfiction_subjects" task has been queued for ' | |
| f"execution with task ID {result.id}. See the celery logs for details " | |
| "about task execution." | |
| ) |
Knowledge Base Used:
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## bugfix/unrecognized-bisac-codes-vote-nonfiction #3736 +/- ##
===================================================================================
- Coverage 93.71% 93.71% -0.01%
===================================================================================
Files 510 510
Lines 46493 46452 -41
Branches 6311 6300 -11
===================================================================================
- Hits 43573 43533 -40
Misses 1886 1886
+ Partials 1034 1033 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
46d57e9 to
ef9e2dc
Compare
af7c1b7 to
153af5c
Compare
| @classmethod | ||
| def contradicts_stored_fiction( | ||
| cls, | ||
| identifier: str | None, | ||
| name: str | None, | ||
| stored_fiction: bool | None, | ||
| ) -> bool: | ||
| """Does this classifier disagree with a subject's stored fiction status? | ||
|
|
||
| Subjects are only re-examined when `checked` is false, so a value | ||
| scored under superseded rules persists indefinitely. Repairs that | ||
| reset `checked` need to identify those rows, and they need to agree | ||
| with each other about which rows they are. Expressing the question | ||
| here keeps that definition in one place: a subject is stale when the | ||
| classifier, run now, does not return what is stored. | ||
|
|
||
| :param identifier: The subject's identifier, as stored. | ||
| :param name: The subject's name, as stored. | ||
| :param stored_fiction: The subject's current `fiction` value. | ||
| :return: True when the classifier no longer agrees with `stored_fiction`. | ||
| """ | ||
| if not identifier and not name: | ||
| # Nothing to classify. Subject.lookup will not create such a row, | ||
| # but both columns are nullable, so do not assume. | ||
| return False | ||
| scrubbed_identifier, scrubbed_name = cls.scrub_identifier_and_name( | ||
| identifier, name | ||
| ) | ||
| return cls.is_fiction(scrubbed_identifier, scrubbed_name) is not stored_fiction | ||
|
|
There was a problem hiding this comment.
This adds the public BISACClassifier.contradicts_stored_fiction helper under src/palace/manager/core, but the repository explicitly marks core as deprecated and prohibits new code there. Move the shared stale-classification logic to a supported package and have the classifier, migration, and task call it there. This repository requirement must be satisfied before merging.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
ef9e2dc to
f3535a5
Compare
|
|
||
|
|
||
| def run(services: Services, session: Session, log: logging.Logger) -> Signature | None: | ||
| return classify_unchecked_subjects.s() |
There was a problem hiding this comment.
The startup task queues only classify_unchecked_subjects, even though old web containers can still consume the migration's reset afterward. The docstring says a later startup task will reapply the reset after those containers are recycled, but no such task exists. If old code restores a stale subject to checked=True, classification will no longer select it, leaving the incorrect nonfiction value in place.
Knowledge Base Used:
f3535a5 to
5c1cc0b
Compare
f99d214 to
76f13e8
Compare
Hold in draft. Merge only after the release containing #3726 and #3736 has gone out. The release N repair -- migration 52d1bbdd4671 plus the startup task that dispatches the re-score -- is exposed for as long as any old code is running. Anything reaching Subject.assign_to_genre before the re-score lands consumes the reset, and code on the superseded rules re-stamps checked=True with the same wrong value. helpers/migrate.yml stops the scripts container, where every Celery worker runs, before migrating, so the worker path is safe. The web containers are not: they are recycled after the migration and can reach assign_to_genre through a presentation recalculation. Fargate deployments are not governed by that playbook at all. Running the reset again a release later closes both, because by then no old code is live anywhere. The task is idempotent, so if the release N repair took, this is a no-op. Re-scoring is left to the nightly classify_unchecked_subjects: with no old code to lose the reset to, dispatching it here would buy nothing. Same shape as the null-audience repair, where startup task 2026_06_17 re-ran what 2026_05_12 had dispatched a release earlier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| def run(services: Services, session: Session, log: logging.Logger) -> Signature | None: | ||
| return chain( |
There was a problem hiding this comment.
The new public run function has no function-level reStructuredText docstring. This violates the repository requirement that all public functions be documented, so the requirement must be satisfied before merging.
| def run(services: Services, session: Session, log: logging.Logger) -> Signature | None: | |
| return chain( | |
| def run(services: Services, session: Session, log: logging.Logger) -> Signature | None: | |
| """Queue the subject reset and reclassification tasks. | |
| :param services: Application services supplied by the startup task runner. | |
| :param session: Database session supplied by the startup task runner. | |
| :param log: Logger supplied by the startup task runner. | |
| :return: A Celery chain that resets and reclassifies affected subjects. | |
| """ | |
| return chain( |
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
5c1cc0b to
a19c2c7
Compare
Hold in draft. Merge only after the release containing #3726 and #3736 has gone out. The release N repair -- migration 52d1bbdd4671 plus the startup task that dispatches the re-score -- is exposed for as long as any old code is running. Anything reaching Subject.assign_to_genre before the re-score lands consumes the reset, and code on the superseded rules re-stamps checked=True with the same wrong value. helpers/migrate.yml stops the scripts container, where every Celery worker runs, before migrating, so the worker path is safe. The web containers are not: they are recycled after the migration and can reach assign_to_genre through a presentation recalculation. Fargate deployments are not governed by that playbook at all. Running the reset again a release later closes both, because by then no old code is live anywhere. The task is idempotent, so if the release N repair took, this is a no-op. Re-scoring is left to the nightly classify_unchecked_subjects: with no old code to lose the reset to, dispatching it here would buy nothing. Same shape as the null-audience repair, where startup task 2026_06_17 re-ran what 2026_05_12 had dispatched a release earlier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
a19c2c7 to
3325a89
Compare
Migration 52d1bbdd4671 repairs subjects that were stored as nonfiction because an unresolvable BISAC code fell through the ruleset catch-all. It only sets checked=false; the re-scoring happens later, in classify_unchecked_subjects. That leaves a window. If old code reaches those subjects first -- a still-running scripts server, or a host that redeploys itself via watchtower or ECS/Fargate auto_restart -- it re-scores them under the superseded rules and re-stamps checked=true. The reset is consumed rather than lost, so nothing errors, nothing retries, and no later run revisits them. The repair quietly did nothing, having paid for a full reindex. Raised in review on #3726; it is not preventable from inside the migration, so make it recoverable instead. Adds reset_non_bisac_nonfiction_subjects, a Celery task that re-applies the reset, with ResetNonBisacNonfictionSubjectsScript and bin/work_reset_non_bisac_nonfiction_subjects to queue it -- the same three layers as reclassify_null_audience_works, which repairs the sibling audience defect. The task resets only. Re-scoring stays with classify_unchecked_subjects, which picks these subjects up on its next nightly run and can be triggered immediately through bin/work_classify_unchecked_subjects. One task, one job, and no large reindex fired the moment someone runs the repair. The selection lives on BISACClassifier as contradicts_stored_fiction, so the migration and the task cannot disagree about which rows are affected. Both already import the classifier, so this adds no coupling that was not there, and it puts the definition with the code that owns the answer. The migration is updated to use it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Migration 52d1bbdd4671 only resets checked=False. classify_unchecked_subjects is what re-scores those subjects and recalculates their works, and left to itself that does not happen until the nightly run. The gap between the two is where the repair is exposed: anything reaching Subject.assign_to_genre in the meantime consumes the reset, and if it is running the superseded rules it re-stamps checked=True with the same wrong value. Nothing errors and nothing revisits the subject afterwards. Adds a startup task that dispatches the re-score immediately after the migration, shrinking that gap from about a day to seconds. This is the pattern the null-audience repair already used -- migration d856ff4dbefb makes the data change, startup task 2026_05_12 dispatches the follow-up. The timing works out: helpers/migrate.yml stops the scripts container -- which is where every Celery worker and beat run -- before running the migration, and starts it again afterwards from the new image. So no worker is alive when this dispatches, and the one that picks the task up is necessarily new code. Web containers are the remaining exposure. The deploy recycles them after the migration, and they can reach assign_to_genre through a presentation recalculation. A second startup task in the next release re-applies the reset once no old code is running anywhere. This moves roughly 4,877 works' worth of recalculation and reindexing from overnight to deploy time. That is about 2% of the nightly volume behind PP-4472, so it should be unremarkable, but it is a deliberate choice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follow-on from dropping the migration. The reset and the re-score are now chained from one place: reset_non_bisac_nonfiction_subjects marks the stale subjects unchecked, classify_unchecked_subjects re-scores them. The second signature is immutable so the chain does not pass the first task's return value into a task that takes no arguments. There is now one implementation of the selection instead of two. The task is the only thing that knows how to find these subjects, and all three callers -- this startup task, the next-release re-apply, and the bin wrapper -- go through it. Chaining rather than dispatching the reset alone is what keeps the repair from being exposed. The reset on its own can be consumed by anything reaching Subject.assign_to_genre first, and code on the superseded rules re-stamps checked=True with the same wrong value, silently. Running the re-score straight after closes that to seconds instead of waiting for the nightly. Docstrings that referred to migration 52d1bbdd4671 now describe the condition they repair rather than pointing at a file that no longer exists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The migration this named was removed when the repair moved to a startup task. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1a85d67 to
5769c9a
Compare
Hold in draft. Merge only after the release containing #3726 and #3736 has gone out. The release N repair -- migration 52d1bbdd4671 plus the startup task that dispatches the re-score -- is exposed for as long as any old code is running. Anything reaching Subject.assign_to_genre before the re-score lands consumes the reset, and code on the superseded rules re-stamps checked=True with the same wrong value. helpers/migrate.yml stops the scripts container, where every Celery worker runs, before migrating, so the worker path is safe. The web containers are not: they are recycled after the migration and can reach assign_to_genre through a presentation recalculation. Fargate deployments are not governed by that playbook at all. Running the reset again a release later closes both, because by then no old code is live anywhere. The task is idempotent, so if the release N repair took, this is a no-op. Re-scoring is left to the nightly classify_unchecked_subjects: with no old code to lose the reset to, dispatching it here would buy nothing. Same shape as the null-audience repair, where startup task 2026_06_17 re-ran what 2026_05_12 had dispatched a release earlier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Hold in draft. Merge only after the release containing #3726 and #3736 has gone out. The release N repair -- migration 52d1bbdd4671 plus the startup task that dispatches the re-score -- is exposed for as long as any old code is running. Anything reaching Subject.assign_to_genre before the re-score lands consumes the reset, and code on the superseded rules re-stamps checked=True with the same wrong value. helpers/migrate.yml stops the scripts container, where every Celery worker runs, before migrating, so the worker path is safe. The web containers are not: they are recycled after the migration and can reach assign_to_genre through a presentation recalculation. Fargate deployments are not governed by that playbook at all. Running the reset again a release later closes both, because by then no old code is live anywhere. The task is idempotent, so if the release N repair took, this is a no-op. Re-scoring is left to the nightly classify_unchecked_subjects: with no old code to lose the reset to, dispatching it here would buy nothing. Same shape as the null-audience repair, where startup task 2026_06_17 re-ran what 2026_05_12 had dispatched a release earlier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Closing: these commits have been rolled into #3726. The classifier fix and the data repair really have to ship together — run on its own, the reset would just be re-scored by the old rules and re-stamped Nothing was dropped. All four commits ( |
Hold in draft. Merge only after the release containing #3726 and #3736 has gone out. The release N repair -- migration 52d1bbdd4671 plus the startup task that dispatches the re-score -- is exposed for as long as any old code is running. Anything reaching Subject.assign_to_genre before the re-score lands consumes the reset, and code on the superseded rules re-stamps checked=True with the same wrong value. helpers/migrate.yml stops the scripts container, where every Celery worker runs, before migrating, so the worker path is safe. The web containers are not: they are recycled after the migration and can reach assign_to_genre through a presentation recalculation. Fargate deployments are not governed by that playbook at all. Running the reset again a release later closes both, because by then no old code is live anywhere. The task is idempotent, so if the release N repair took, this is a no-op. Re-scoring is left to the nightly classify_unchecked_subjects: with no old code to lose the reset to, dispatching it here would buy nothing. Same shape as the null-audience repair, where startup task 2026_06_17 re-ran what 2026_05_12 had dispatched a release earlier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Hold in draft. Merge only after the release containing #3726 and #3736 has gone out. The release N repair -- migration 52d1bbdd4671 plus the startup task that dispatches the re-score -- is exposed for as long as any old code is running. Anything reaching Subject.assign_to_genre before the re-score lands consumes the reset, and code on the superseded rules re-stamps checked=True with the same wrong value. helpers/migrate.yml stops the scripts container, where every Celery worker runs, before migrating, so the worker path is safe. The web containers are not: they are recycled after the migration and can reach assign_to_genre through a presentation recalculation. Fargate deployments are not governed by that playbook at all. Running the reset again a release later closes both, because by then no old code is live anywhere. The task is idempotent, so if the release N repair took, this is a no-op. Re-scoring is left to the nightly classify_unchecked_subjects: with no old code to lose the reset to, dispatching it here would buy nothing. Same shape as the null-audience repair, where startup task 2026_06_17 re-ran what 2026_05_12 had dispatched a release earlier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Hold in draft. Merge only after the release containing #3726 and #3736 has gone out. The release N repair -- migration 52d1bbdd4671 plus the startup task that dispatches the re-score -- is exposed for as long as any old code is running. Anything reaching Subject.assign_to_genre before the re-score lands consumes the reset, and code on the superseded rules re-stamps checked=True with the same wrong value. helpers/migrate.yml stops the scripts container, where every Celery worker runs, before migrating, so the worker path is safe. The web containers are not: they are recycled after the migration and can reach assign_to_genre through a presentation recalculation. Fargate deployments are not governed by that playbook at all. Running the reset again a release later closes both, because by then no old code is live anywhere. The task is idempotent, so if the release N repair took, this is a no-op. Re-scoring is left to the nightly classify_unchecked_subjects: with no old code to lose the reset to, dispatching it here would buy nothing. Same shape as the null-audience repair, where startup task 2026_06_17 re-ran what 2026_05_12 had dispatched a release earlier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Description
Carries the whole data repair for PP-4849, as a startup task rather than a migration. The migration in #3726 was removed since it would unnecessarily duplicate the work done here in the startup task.
Three layers, mirroring
reclassify_null_audience_works, which repairs the sibling audience defect:reset_non_bisac_nonfiction_subjects— a Celery task that re-applies the reset.ResetNonBisacNonfictionSubjectsScript— queues the task.bin/work_reset_non_bisac_nonfiction_subjects— the wrapper an operator runs.The selection lives on
BISACClassifierascontradicts_stored_fiction, with exactly one implementation — the Celery task — that all callers go through.Note
Stacked on #3726 and based on its branch, since the migration this repairs does not exist on
mainyet. GitHub will retarget this tomainwhen #3726 merges. Only the single commit here is new.Motivation and Context
Raised by @tdilauro in review on #3726.
The migration only sets
checked=false; the re-scoring happens later, inclassify_unchecked_subjects. That leaves a window. If old code reaches those subjects first — a still-running scripts server, or a host that redeploys itself via watchtower or ECS/Fargateauto_restart— it re-scores them under the superseded rules and re-stampschecked=true.The failure is quiet, which is what makes it worth addressing. The reset is consumed rather than lost, so nothing errors, nothing retries, and no later run revisits those subjects. The repair did nothing, having paid for a full reindex to do it. It will not happen on a normal
migratedeployment.This cannot be prevented from inside the migration, so the aim is to make it recoverable instead: run the reset again.
The task resets only
Re-scoring stays with
classify_unchecked_subjects, which picks these subjects up on its next nightly run and can be triggered immediately throughbin/work_classify_unchecked_subjects. One task, one job — and no large reindex fired the moment someone runs the repair.How Has This Been Tested?
New tests:
TestBISACClassifier::test_contradicts_stored_fiction— seven parametrized cases in both directions: a vendor code, a shape-valid but non-existent code, and a realFIC*code all read as stale when stored nonfiction; a real nonfiction code and agreeing values read as current;(None, None)is not stale.test_reset_non_bisac_nonfiction_subjects— four subjects establishing the selection boundary. The stale one is reset; a real nonfiction code, a subject already scored fiction, and atag-typed subject carrying the same identifier are all left checked.test_reset_non_bisac_nonfiction_subjects_is_idempotent— a second run leaves the reset in place and finds nothing to do.TestResetNonBisacNonfictionSubjectsScript::test_run— the script queues the task, mirroring the null-audience script's test.Full suite locally against Postgres + Valkey: 6,276 passed, 0 failed. 104 errors, all the OpenSearch container not running locally (
test_search.py); CI covers those.tests/migrationpasses with the migration routed through the shared classmethod.mypyclean across 1,166 files; all pre-commit hooks pass.Checklist
🤖 Generated with Claude Code
Update: the migration is gone
An earlier revision of this stack did the reset in a migration (
52d1bbdd4671) and kept the Celery task only for re-runs. That meant two implementations of one algorithm — select BISAC subjects stored as nonfiction, filter throughcontradicts_stored_fiction, resetchecked, log the counts — written once in raw SQL and once through the ORM, sharing only the per-row predicate.The migration has been dropped (see the commit on #3726). The repair now runs from a startup task, which:
reset_non_bisac_nonfiction_subjectsintoclassify_unchecked_subjects(the second signature immutable, so the chain does not pass a return value into a task that takes none), closing the reset-to-re-score gap to seconds;bin/wrapper and with Re-apply the non-BISAC nonfiction reset a release later (PP-5129) #3737's next-release re-apply.It also puts the repair where the README says post-deployment work depending on new code belongs, and removes a frozen migration whose selection depended on the live classifier.
The trade, for the record: a migration runs regardless of Celery's health, where a dispatched chain is lost if the broker is down at deploy. #3737 is what makes that acceptable.