Location Tagger default agent + geocoding in add_annotations_from_exact_strings (#1822)#1854
Conversation
) - Register a global "Location Tagger" default agent (agents migration 0015 + DEFAULT_LOCATION_TAGGER_INSTRUCTIONS) wired to the add_annotations_from_exact_strings tool. - Extend add_annotations_from_exact_strings with an optional per-item hints field. OC_COUNTRY/OC_STATE/OC_CITY spans are geocoded offline (#1819) and {canonical_name, lat, lng, admin_codes, geocoded} is stored on Annotation.data. Non-geographic labels are untouched (backward compatible). - Add shared build_geocoded_annotation_data helper in geographic_service and reuse it in the GraphQL geographic mutations (removes a duplicated inline dict; single source of truth for the Annotation.data shape). - Update the tool registry definition, docs (agent guide + manual test script), tests, and CHANGELOG. Closes #1822
CorpusAction uses agent_config= (not agent=) and requires a non-empty task_instructions for agent-based actions; update the manual walkthrough snippet to match the model so copy-paste works.
Code Review — Location Tagger default agent + geocoding (#1822)OverviewThis PR adds a built-in Location Tagger agent that auto-creates geocoded
Overall the approach is sound and the design is clean. There are a few issues to address before merging. 🔴 Settings reference inside migration (will break fresh installs)File: system_instructions=settings.DEFAULT_LOCATION_TAGGER_INSTRUCTIONS,Referencing Fix: copy the prompt literal into the migration itself, same way the other default-agent migrations store their instructions. 🟠 Geocoding called inside
|
| Severity | Issue |
|---|---|
| 🔴 | Settings referenced in migration — will break fresh-install migrations if setting is renamed/removed |
| 🟠 | Geocoding runs inside transaction.atomic() — should be lifted out to avoid holding the lock during disk I/O |
| 🟡 | hints type annotation is dict instead of dict[str, str] |
| 🟡 | Return type of build_geocoded_annotation_data should be dict[str, Any] |
| 🟡 | geocode_label_type is not validated at runtime (only type: ignore'd) |
| ⚪ | PR body credits Claude Code — violates CLAUDE.md policy |
The red item is the most important — please inline the instructions into the migration before merging.
Linter CI flagged two files black would reformat (collapse wrapped single-statement lines). Run black/isort to satisfy pre-commit.
Code Review: PR #1854 — Location Tagger default agent + geocodingOverviewThis PR implements a built-in Location Tagger agent that geocodes geographic mentions in documents to power map views. The approach is sound: extract a shared Blocking Issues1. PR description credits Claude Code — violates CLAUDE.md The PR body ends with a "Generated by Claude Code" credit. CLAUDE.md is explicit: "Never credit Claude or Claude Code in commit messages, PR messages, comments, or any other artifacts." Please remove this line before merge. Important Issues2. Migration idempotency uses if AgentConfiguration.objects.filter(
scope="GLOBAL", name=LOCATION_TAGGER_NAME
).exists():
returnIf 3. Migration silently skips on a fresh database with no superuser if not system_user: # pragma: no cover
returnOn a fresh production deploy (migrations run before the first superuser is seeded) the Location Tagger is never created and nothing is logged. Operators won't know to recreate it manually. A 4. hints: NotRequired[dict]The geocoder only reads string keys Minor / Style Notes5. Lazy imports inside the function body (annotations.py) The 6. The failure path includes 7. No runtime guard behind the resolved = resolve_place(
text,
geocode_label_type, # type: ignore[arg-type] # validated by caller / resolver
...
)"Validated by caller" but there is no assertion. Adding 8. Re-run deduplication (documented TODO) Running the tagger twice over the same document will duplicate all geographic annotations. This is called out in the docs under Limitations — just make sure a follow-up issue is filed so it does not get lost. What's Done Well
SummaryOne blocking change (remove the Claude Code PR credit), two important fixes (migration idempotency key + log a warning instead of silently skipping without a superuser), and a few minor nits. The core implementation is well-designed and the test suite is solid. |
- Lift offline geocoding (disk I/O via resolve_place) out of the transaction.atomic() block in add_annotations_from_exact_strings; resolve per item up front and reuse the payload inside the loop. - Migration 0015: key idempotency off the unique slug (not name), log a warning instead of silently skipping when no superuser exists, and read the prompt via getattr(settings, ..., fallback) so a fresh migrate never hard-fails if the setting is later renamed. - Tighten hints typing to dict[str, str]; build_geocoded_annotation_data now returns dict[str, Any] and asserts the label type instead of only suppressing the checker.
Code Review — Location Tagger default agent + geocoding in
|
…save, assert raw_text - geographic_service.resolve_place: raise ValueError instead of assert for an invalid geocode_label_type — assert is stripped under python -O (common in prod containers), which would let a bad type reach resolve_place silently. - annotations.py: drop the unreachable isinstance(item, dict) guard (item is an AnnotationItem TypedDict, always a dict at runtime); keep the inner hints guard. - test fixture: drop redundant doc.save() (FieldFile.save commits with save=True). - test_ungeocodable_geo_span_marks_not_geocoded: assert the miss sentinel preserves raw_text so the map aggregation's unresolved-trace contract is pinned.
Code Review — Location Tagger default agent + geocoding (#1822)Overall this is a clean, well-structured implementation. The DRY improvements, transaction-safety discipline, and backward-compatibility story are all solid. A few things worth addressing before merge: Issues1.
parsed_items: list[tuple[str, str, dict | None]] = []The parsed_items: list[tuple[str, str, dict[str, str] | None]] = []2. No test for multi-occurrence geocoding (the resolve-once optimisation is untested) The code correctly pre-resolves geocoding once per item before entering the transaction and then reuses that payload for every occurrence of the same span: prepared: list[tuple[str, str, dict | None]] = []
for label_text, exact_str, hints in parsed_items:
# resolve once …
prepared.append((label_text, exact_str, annotation_data))
with transaction.atomic():
for label_text, exact_str, annotation_data in prepared:
…
while pos != -1:
annot_obj = _create_annotation(pos, end_idx, label_obj)
if annotation_data is not None:
annot_obj.data = annotation_data # same object reused per occurrence
annot_obj.save()There is no test that verifies: (a) when a place name appears twice in a document, both resulting annotations carry the geocoded data; and (b) 3. Migration reads live settings — limitation is documented but the fallback does change behaviour silently
system_instructions=getattr(
settings,
"DEFAULT_LOCATION_TAGGER_INSTRUCTIONS",
_FALLBACK_INSTRUCTIONS,
),This is intentional and the PR explains the tradeoff, so this is a note rather than a blocker. The concern is: any deployment that runs Nits
Strengths
|
…add tests - Narrow parsed_items' hint element to dict[str, str] | None to match AnnotationItem.hints (review #1). - 0015 migration logs a warning when DEFAULT_LOCATION_TAGGER_INSTRUCTIONS is absent and it creates the agent with the concise fallback prompt, so the degraded state is visible; clarified the misleading header comment (review #3 + nit). - Add a test pinning the migration→settings system_instructions wiring, and a multi-occurrence test proving geocoding resolves once per item and is reused for every occurrence (review #2).
Code Review — Location Tagger default agent + geocoding (#1822)Overall this is a solid, well-scoped PR. The architecture decisions (geocoding before the transaction, DRY builder, idempotent migration) are all the right calls, and the test suite covers the meaningful deterministic surface. A few items below worth addressing before merge. Correctness / Bugs1. annot_obj = _create_annotation(pos, end_idx, label_obj) # unsaved
if annotation_data is not None:
annot_obj.data = annotation_data # set before the single save
annot_obj.save() # ← one DB writeThis is correct and efficient. Worth a short inline comment since the two-step pattern is non-obvious to a future reader. 2. Empty / whitespace Type / Style3. # annotations.py
prepared: list[tuple[str, str, dict | None]] = []The third element is typed as bare Design / Architecture4. Migration instructions are a point-in-time snapshot of the settings value 5. Tests6. # test assumes population tie-break always picks Paris, FR over Paris, TX
self.assertEqual(ann.data["admin_codes"]["iso_alpha2"], "FR")This is reasonable given the current geocoder, but the assertion is fragile if the dataset or tie-breaking logic changes. At minimum, add a comment explaining the assumption ("unhinted 'Paris' resolves to the highest-population match, which is Paris, FR"). Alternatively, the test could simply assert 7. Spy test: Minor / Documentation8. Docs link may be stale 9. Cleanup step in Annotation.objects.filter(
annotation_label__text__in=[OC_COUNTRY_LABEL, OC_STATE_LABEL, OC_CITY_LABEL]
).delete()This deletes all OC_* annotations across the entire database — dangerous if the test is run against a populated staging/production instance. Scoping the delete to the specific corpus ( Summary
Items 2 and 9 are the ones worth fixing before merge; the rest are either low-risk nits or documentation improvements. |
…cope, comments
- Skip blank/whitespace exact_string in add_annotations_from_exact_strings
(prevents wasted geocoder call and an infinite occurrence loop on find(""))
- Tighten prepared tuple type to dict[str, str] | None
- Fix stale docs link (walkthrough/step-9-corpus-actions.md)
- Note migration system_instructions is a point-in-time snapshot
- Scope end-to-end cleanup delete to the test corpus (was DB-wide OC_* wipe)
- Comment the unsaved-then-save pattern, the migration identity check, and the
population tie-break assumption in test_city_without_hints_resolves_to_france
Code Review — Location Tagger default agent + geocoding (#1822)OverviewThis PR adds a built-in Location Tagger global agent that geocodes Strengths
Issues1. Blank-span guard is untested
if not exact_str.strip():
continueThe PR description calls it out as a bug fix (would have caused an infinite loop) but there is no test exercising it. A simple case — call 2. Stray loop variable reference in test
for ann in anns:
self.assertIsNotNone(ann.data)
self.assertTrue(ann.data["geocoded"])
self.assertEqual(ann.data["admin_codes"]["iso_alpha2"], "FR")
# uses the last-iteration value of `ann`, not both annotations
self.assertEqual(ann.annotation_type, SPAN_LABEL)Move the 3.
|
Code Review — Location Tagger default agent + geocoding in
|
…ci-nTwmN # Conflicts: # CHANGELOG.md
|
Babysit pass:
No code changes were needed beyond the merge. |
Code Review — Location Tagger default agent + geocoding (#1822)OverviewThis PR implements a well-structured feature: a built-in Location Tagger agent that auto-creates geocoded OC_COUNTRY/OC_STATE/OC_CITY annotations via the existing corpus-actions framework. The core design — extracting Strengths
IssuesMedium — test assertion only checks the last loop iterationFile: The for ann in anns:
self.assertIsNotNone(ann.data)
self.assertTrue(ann.data["geocoded"])
self.assertEqual(ann.data["admin_codes"]["iso_alpha2"], "FR")
# Resolver invoked exactly once for the single item, not per occurrence.
self.assertEqual(spy.call_count, 1)
self.assertEqual(ann.annotation_type, SPAN_LABEL) # <-- only the last `ann` from the loopMinor —
|
2 participants
Summary
Implements issue #1822 — a built-in Location Tagger default agent that auto-creates geocoded
OC_COUNTRY/OC_STATE/OC_CITYannotations so documents populate the Discover (#1820) and Corpus Home (#1821) maps at scale. It runs through the existing corpus-actions framework (same path as the Document/Corpus assistants).Assessment first: I verified the issue's hard dependency #1819 is already merged on this branch before building anything —
resolve_place(opencontractserver/utils/geocoding/service.py),ResolvedPlace, theOC_*label constants (opencontractserver/constants/annotations.py), theAnnotation.datafield (migration0075), and the geographic mutations/service all exist.What changed
Default agent registration
opencontractserver/agents/migrations/0015_create_location_tagger_agent.py— idempotent (keyed on the uniqueslug),scope=GLOBAL,is_active/is_public, globe badge,available_tools=["add_annotations_from_exact_strings"]. Reads the prompt viagetattr(settings, ..., fallback)so a freshmigratenever hard-fails if the setting is later renamed; logs a warning instead of silently skipping when no superuser exists yet.DEFAULT_LOCATION_TAGGER_INSTRUCTIONSinconfig/settings/base.py— prompt covering the three label types, exact-string requirement, and the country/state hint rules for disambiguating "Paris", "Springfield", etc.Geocoding integrated into the existing tool (
opencontractserver/llms/tools/core_tools/annotations.py)AnnotationItemgains an optionalhintsfield (NotRequired[dict[str, str]]).label_text ∈ {OC_COUNTRY, OC_STATE, OC_CITY}the span is resolved throughresolve_placeand{canonical_name, lat, lng, admin_codes, geocoded}is written toAnnotation.data. Geocoding is resolved once per item before the DB transaction (it does disk I/O), then reused for every occurrence.hintsis ignored for them — backward compatible; the async wrapper inherits the change.DRY: shared data-builder (
opencontractserver/annotations/services/geographic_service.py)build_geocoded_annotation_data(...) -> dict[str, Any]+LABEL_TEXT_TO_GEOCODE_LABEL_TYPEare the single source of truth for theAnnotation.datapayload.config/graphql/annotation_mutations.pynow reuses it (removing the duplicated inline dict + a now-deadresolve_placeimport), so the GraphQL mutations and the agent tool emit an identical shape that the map aggregation service consumes.Tool registry — updated the
add_annotations_from_exact_stringsdefinition/description to documenthints.Docs & tests
docs/agents/location_tagger.md(configuration walkthrough, worked Paris-FR/Paris-TX/Austin example, limitations).docs/test_scripts/location_tagger_end_to_end.md(manual upload → corpus action → map pins).opencontractserver/tests/test_location_tagger_agent.py— agent registration + idempotency, country/state geocoding, hint disambiguation (Paris→TX vs Paris→FR), ungeocodable-missgeocoded:Falsesentinel, and non-geographic backward-compatibility.[Unreleased] > Added.Notes / out of scope (per the issue)
Closes #1822