Skip to content

Fix fastrun bugs, resolve TODOs and update the documentation - #1001

Closed
LeMyst wants to merge 3 commits into
masterfrom
fix-wbi-fastrun
Closed

Fix fastrun bugs, resolve TODOs and update the documentation#1001
LeMyst wants to merge 3 commits into
masterfrom
fix-wbi-fastrun

Conversation

@LeMyst

@LeMyst LeMyst commented Jul 5, 2026

Copy link
Copy Markdown
Owner

Summary

This PR fixes several long-standing bugs in wbi_fastrun.py, resolves the three TODOs of the module, and rewrites the outdated fast run section of the README. Every fix comes with a regression test; the offline test suite grows from 203 to 215 tests.

Bug fixes in write_required() / get_items()

  • Append mode could silently skip a required write: a new value missing from the item was masked by another value matching two duplicate statements (len(comp) counted matching pairs, not matched statements). Each new statement must now individually exist on the item.
  • FORCE_APPEND now always reports a write: previously, when all the submitted statements already existed, the forced append was silently skipped.
  • case_insensitive mode crashed with an AttributeError (datavalue is a dict, not a string) and could never match values differing by case. The comparison is now handled by _statements_equal(), which compares string values casefolded together with qualifiers (and references when use_refs is enabled).
  • Deletion objects (value-less claims) crashed get_items() with a KeyError: the skip condition used and where the intent (and the equivalent code in write_required()) was or.
  • Quantity properties never matched: get_items() looked up the full SPARQL literal ("+42"^^xsd:decimal) while rev_lookup stores plain amounts (+42), so a write was always wrongly reported.
  • Unitless quantities never matched: the RDF export represents a unitless quantity with the Wikidata Q199 entity whatever the instance (verified against WDQS), while the JSON representation uses '1'. The unit URIs are now normalized by a dedicated _normalize_unit() method, replacing the previous "dirty fix" that only covered non-Wikidata instances.

TODO resolutions

  • Qualifier/reference reconstruction now supports every datatype: qualifiers and references are rebuilt with parse_sparql_value() instead of passing a generic value to constructors that do not all accept it (quantity qualifiers crashed with a TypeError).
  • Time.parse_sparql_value() added: time values crashed the generic parsing path (Time.set_value() has no value parameter). The precision is inferred from the timestamp.
  • Monolingual text qualifiers and references keep their language in format_query_results(), so they can be reconstructed and compared.
  • The remaining limitation (attributes absent from the SPARQL simple values: time precision, globe coordinate precision, quantity bounds) is documented in the code and the README instead of a TODO.

Cleanups

  • Remove the dead del_props handling and the unreachable FORCE_APPEND condition
  • Raise a clear ValueError instead of an IndexError when no datatype class matches
  • clear() also resets the language data cache
  • Debug-only loops only run when DEBUG logging is enabled

Documentation

The "Examples (in fast run mode)" section of the README still documented the WikidataIntegrator API (fast_run=True, fast_run_base_filter as a dictionary, init_fastrun()). It is rewritten around the current API: entity.write_required() and its options, the three base filter forms (including property paths), the fact that only the claims whose property appears in the base filter are compared, label/description/aliases checking through the fastrun container, and the limitations.

Test plan

  • pytest test/ — 215 passed (12 new regression tests)
  • mypy — no issues on the modified files
  • Unitless unit representation verified against the live Wikidata Query Service

🤖 Generated with Claude Code

LeMyst and others added 3 commits July 5, 2026 22:32
- get_items: skip claims missing a value OR a datatype; deletion
  objects (value-less claims) crashed with a KeyError before
- get_items: look up quantity values in the same format as stored in
  rev_lookup ('+42'); fastrun always reported a write for quantities
- write_required: in append mode, a new value missing from the item
  could be masked by another value matching duplicate statements,
  silently skipping a required write
- write_required: FORCE_APPEND now always reports a write, even when
  the submitted statements already exist on the item
- write_required: fix the case_insensitive comparison, which crashed
  with an AttributeError (datavalue is a dict) and could never match
- Remove the dead del_props handling and unreachable FORCE_APPEND check
- Raise a clear ValueError instead of an IndexError when a datatype has
  no implementing class
- clear() now also resets the language data cache
- Only run the write_required debug loop when DEBUG logging is enabled

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The section still documented the WikidataIntegrator API (fast_run=True,
fast_run_base_filter as a dictionary, init_fastrun(), add_claims()),
which no longer exists. Rewrite it around the current API:

- Document entity.write_required() and its options (use_refs,
  case_insensitive, action_if_exists)
- Document the three base filter forms, including property paths
- Document that only the claims whose property appears in the base
  filter are compared
- Document label/description/aliases checking through the fastrun
  container (check_language_data), since write_required() does not
  check them
- Document the limitations (SPARQL lag, memory, partially supported
  datatypes)
- Remove the outdated sentence saying WikibaseIntegrator lacks the
  fastrun functionality

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reconstruction of qualifiers and references (TODO: support Time,
MonolingualText, GlobeCoordinate):

- Rebuild qualifiers and references with parse_sparql_value() instead
  of passing a generic value to the datatype constructors, which do not
  all accept it: quantity qualifiers crashed with a TypeError
- Add Time.parse_sparql_value(); time values crashed the generic
  parsing path with a TypeError because Time.set_value() has no value
  parameter (the precision is inferred from the timestamp)
- Keep the language of monolingual text qualifiers and references in
  format_query_results(), it is needed to reconstruct them
- Unify the main statement reconstruction on parse_sparql_value() and
  document that attributes missing from the SPARQL simple values (time
  precision, quantity bounds...) are rebuilt with default values

Unitless unit normalization (TODO: dirty fix Q199):

- The RDF export of any Wikibase instance represents a unitless
  quantity with the Wikidata Q199 entity (verified on WDQS). Normalize
  it to '1' (the JSON representation) in a dedicated _normalize_unit()
  method. On Wikidata itself, Q199 was previously stored as a local
  entity ID, so unitless quantities never matched the local claims and
  a write was always wrongly reported.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
LeMyst added a commit that referenced this pull request Jul 27, 2026
…1012)

The README still claimed WikibaseIntegrator lacks the fastrun
functionality of WikidataIntegrator, while wbi_fastrun.py exists and is
documented later in the same file. Replace the sentence with the same
wording as PR #1001 so both branches merge cleanly.

AGENTS.md said the version string is mirrored in __init__.py, but it is
now read from the installed package metadata via importlib.metadata.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
LeMyst added a commit that referenced this pull request Jul 28, 2026
Port the idea behind the generic parse_sparql_value() of the old fastrun
implementation (PR #1001) to the from_sparql_value() mechanism: give
BaseDataType a working generic implementation for the data types whose
RDF representation is the literal value itself, instead of a stub
returning None.

A generic implementation is not enough for the data types backed by a
URI: storing the raw URI would never match the value held by a local
claim. Implement from_sparql_value() for each of them, extracting the
value the local claim holds:

- Property, Lexeme, Form and Sense: the entity ID
- GeoShape and TabularData: the Commons page title, percent-decoded
- CommonsMedia: the file name, percent-decoded

Before this change, Property, Lexeme, Form, Sense, GeoShape and
TabularData were skipped with a warning and their statements were always
reported as requiring a write, a limitation the README documented.
CommonsMedia was worse: it inherited the URL implementation, silently
stored the whole Commons URL, and could never match a local claim
without even a warning.

Every reachable datatype is now compared. Only EntitySchema is left out,
since it has no PTYPE mapping to the Wikibase ontology and is therefore
never resolved from the SPARQL property type.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@LeMyst

LeMyst commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

Closing in favour of #333, which rewrites the fastrun implementation instead of patching the current one.

The decisive difference is architectural. This PR keeps _query_data(), which loads the whole corpus with a single paginated SPARQL query. That query is what repeatedly timed out (five consecutive WDQS 504s) on a real benchmark of only 95 entities, and it is the part that cannot scale to a larger corpus. #333 scopes its queries per statement instead, so each one stays small.

This PR also still carries the crash at wbi_fastrun.py:291: the list comprehension in that log.debug() call builds [z.mainsnak.datavalue for z in x.qualifiers], but iterating qualifiers yields Snaks, which expose .datavalue directly (lines 274-278 get this right). Since the list is built before the call, it raises an AttributeError even with debug logging disabled. #333 no longer has that code.

Everything worth keeping from this PR was compared against #333 and was already covered there: the clear ValueError on unknown datatypes, the Q199 unitless normalization, clear() resetting the language cache, FORCE_APPEND always requiring a write, the case-insensitive comparison fix, and lazy debug logging.

The one thing this PR still had over #333 was datatype coverage, through the generic parse_sparql_value() on BaseDataType. That has now been ported to #333 (commit Support every datatype in fastrun comparisons) as a generic from_sparql_value() for literal values, plus URI-specific implementations for Property, Lexeme, Form, Sense, GeoShape, TabularData and CommonsMedia. It also fixed a silent bug found on the way: CommonsMedia inherited the URL implementation, stored the whole Commons URL and could never match a local claim, without even emitting a warning.

@LeMyst LeMyst closed this Jul 28, 2026
LeMyst added a commit that referenced this pull request Jul 28, 2026
* Implement new version of FastRun

* Complete the new FastRun implementation

Features:
- Restrict write_required() to the entity being edited: baseentity now
  passes entity_filter=self.id, so data existing on another entity no
  longer inhibits a required write
- Support action_if_exists in write_required(); FORCE_APPEND always
  reports a write as required
- Port the language data checking (labels, descriptions, aliases) from
  the old implementation: init_language_data(), get_language_data() and
  check_language_data(), backed by the shared base filter
- Implement the case insensitive mode: string values are compared
  casefolded, the SPARQL data is keyed casefolded at load time
- Add a clear() method and a module docstring

Fixes:
- write_required() crashed with an IndexError when no claim matched the
  property filter; it now reports a write as required
- The deep comparison returned early on the first mismatching statement:
  an entity holding duplicate statements (one matching, one not) was
  wrongly reported as requiring a write. The comparison now looks for
  at least one entity holding a matching statement for every claim
- A load restricted to a value or to qualifiers (cache disabled) was
  reused as a complete cache, poisoning later comparisons; partial
  loads are no longer marked as complete
- _load_qualifiers()/_load_references() crashed when building datatypes:
  the full property URI was passed as prop_nr; it is now reduced to the
  bare property ID
- _load_references() duplicated references spanning two result pages
- A base filter mixing a valueless property and a property path crashed
  in the base filter string generation
- Datatypes without from_sparql_value() support and unparseable values
  no longer crash the load; they are skipped with a warning and the
  statements are reported as requiring a write
- Lazily loaded qualifiers, references and ranks are memoized per
  statement, honoring the cache flag

Tests: rebuild test/test_wbi_fastrun.py on the offline MockWikibase
infrastructure with a per-query-type SPARQL dispatcher (45 tests).

Docs: rewrite the fast run README section around the actual API (base
filter forms, write_required() options, language data checking,
limitations) and remove the outdated WikidataIntegrator-era example.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Compare the unit of quantity values in fastrun

The simple value of a quantity statement does not carry the unit: two
amounts only differing by their unit were considered equal and a
required write could be skipped.

load_statements() now loads the unit from the value node (OPTIONAL, so
only quantities bind it) and the normalized unit becomes part of the
comparison key. The unit is normalized to the format of the JSON
representation: the RDF export of any Wikibase instance represents a
unitless quantity with the Wikidata Q199 entity, which maps back to
'1', and the other units are reduced to their bare entity ID.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Fix mypy errors in the fastrun tests

- Guard the re.search() results before reading the match groups
- Import ItemEntity instead of a string annotation

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Fix codespell typo: unparseable -> unparsable

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* Support every datatype in fastrun comparisons

Port the idea behind the generic parse_sparql_value() of the old fastrun
implementation (PR #1001) to the from_sparql_value() mechanism: give
BaseDataType a working generic implementation for the data types whose
RDF representation is the literal value itself, instead of a stub
returning None.

A generic implementation is not enough for the data types backed by a
URI: storing the raw URI would never match the value held by a local
claim. Implement from_sparql_value() for each of them, extracting the
value the local claim holds:

- Property, Lexeme, Form and Sense: the entity ID
- GeoShape and TabularData: the Commons page title, percent-decoded
- CommonsMedia: the file name, percent-decoded

Before this change, Property, Lexeme, Form, Sense, GeoShape and
TabularData were skipped with a warning and their statements were always
reported as requiring a write, a limitation the README documented.
CommonsMedia was worse: it inherited the URL implementation, silently
stored the whole Commons URL, and could never match a local claim
without even a warning.

Every reachable datatype is now compared. Only EntitySchema is left out,
since it has no PTYPE mapping to the Wikibase ontology and is therefore
never resolved from the SPARQL property type.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant