Skip to content

Timx 738 marc lxml - #283

Open
ghukill wants to merge 5 commits into
timx-738-alma-speedfrom
timx-738-marc-lxml
Open

ghukill wants to merge 5 commits into
timx-738-alma-speedfrom
timx-738-marc-lxml

Conversation

@ghukill

@ghukill ghukill commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Note

This PR is stacked on #282.

Purpose and background context

This is the 3rd and final PR that seeks to improve Transmogrifier transform performance, specifically for alma.

This converts the alma transformer to use lxml instead of BeautifulSoup4 (bs4) for transformations.

This work was POC'ed quite some time ago and was confirmed to be a massive speedup. When combined with the parallel workers from [the previous PR], this pushes transformation speeds into ~4-5k records/second. Before any speed improvements a full transform of alma at ~4.5m records was roughly 14-15 hours. If we assume 4.5k records/second after parallel workers + lxml based transformation, we're looking at ~16.6 minutes.

This fundamentally changes how we can develop in Transmogrifier. Now, we are unblocked to make meaningful and substantive changes and perform an entire run to see how they play out in alma. Timmy is helpful for the analyzing the new transformed records, but without these Transmogrifier speed improvements, we would wait too long between those analysis.

This shift from bs4 --> lxml takes the following approach:

  • create some helper methods like Marc.get_element_string() and Marc.find_all_elements() that mimic the ergonomics of bs4, but using lxml under the hood
  • updates to all fields methods are still required, but very minimal by leaning on these helper methods

The effect is a shift in the transformation libraries used, but very little change ergonomically in code and zero change to the records themselves. This is confirmed by some analysis in Timmy: see the following Timmy report that analyzes all alma records before any of the work in these stacked PRs and after, timx-738-alma-full-before-and-after.pdf. The finding is that two fields have changes, identifiers and holdings, but these are related to TIMX-692 and not these PRs.

Supporting the case for all this work, the code changes from TIMX-692 have not been applied to the records in production because it's so slow! With the work complete in TIMX-700 it's arguablly much easier, as we could kickoff a "transform all current alma records" style ETL run, but this would still take ~14-15 hours.

Lastly, note that this bs4 --> lxml conversion is only for alma at this time. All other XML sources would benefit from this treatment as well, but were not included in this PR.

How can a reviewer manually see the effects of these changes?

Tempory Timmy Instance

To facilitate seeing the before and after, a temporary Timmy instance has been created on our linux box for review.

The following commands were used to generate this temporary Timmy instance and ingest all alma records (recalling that they predate the TIMX-692 changes):

# create temp timmy
timmy \
instance \
create-temp ~/.timmy/instances/temp/timx-738 \
--name timx-738 \
--aws-profile timmy-prod

# ingest
timmy \
--config ~/.timmy/instances/temp/timx-738/timmy.toml \
timmylake import --sync s3://timdex-extract-prod-300442551476/timmy/exports/alma-current-full-2026-11-02/

Next, Transmogrifier was run on that machine transforming the full alma corpus with the new parallel workers (note that we're using 7 workers because this machine has 8 CPU cores) + lxml code, writing the output to S3 as well:

uv run transform -s alma \
--input-parquet s3://timdex-extract-prod-300442551476/timmy/exports/alma-current-full-2026-11-02/ \
--output-parquet s3://timdex-extract-prod-300442551476/timmy/exports/alma-current-full-timx-738-transformed/ \
--run-id alma-current-full-timx-738-transformed \
--workers 7

The entire process took 22 minutes to complete:

2026-11-03 11:38:50,486 INFO transmogrifier.cli.main(): Running transform for source alma
...
2026-11-03 12:01:05,136 INFO transmogrifier.sources.transformer.write_to_parquet_files(): Parquet files write complete - files: 43, rows: 4229248
2026-11-03 12:01:05,144 INFO transmogrifier.cli.main(): Completed transform, total records processed: 4229248, transformed records: 4229248, skipped records: 0, deleted records: 0
2026-11-03 12:01:05,144 INFO transmogrifier.cli.main(): Total time to complete transform: 0:22:14.658843

Finally, the transformed records are ingested into Timmy for analysis:

# ingest
timmy \
--config ~/.timmy/instances/temp/timx-738/timmy.toml \
timmylake import --sync s3://timdex-extract-prod-300442551476/timmy/exports/alma-current-full-timx-738-transformed/

# serve webapp
uv run timmy --config ~/.timmy/instances/temp/timx-738/timmy.toml serve -d

With VPN connected, access Timmy at: http://10.51.1.241:5100/.

1- Navigate to runs: http://10.51.1.241:5100/runs/.

2- Locate the run alma-current-full-timx-738-transformed: http://10.51.1.241:5100/runs/alma-current-full-timx-738-transformed

3- Click into "Analyze field changes": http://10.51.1.241:5100/runs/alma-current-full-timx-738-transformed/changes

The data here shows what the report summarizes above. Most fields are unchanged when getting transformed via lxml and new transform code. The fields that do change, are unrelated from these PRs.

Includes new or updated dependencies?

NO

Changes expectations for external applications?

NO

What are the relevant tickets?

Code review

  • Code review best practices are documented here and you are encouraged to have a constructive dialogue with your reviewers about their preferences and expectations.

Why these changes are being introduced:
* Contributor kinds for a name are collected in a set and sorted
case-insensitively. Kinds differing only by case (e.g. "Editor" and "editor")
tie on that key and keep the set's iteration order, which depends on
per-process string hash randomization, so the same record could transform
differently between runs or worker processes.

How this addresses that need:
* Sort contributor kinds by lowercased value, then by value.

Side effects of this change:
* Records with kinds differing only by case always list the uppercase kind
first. In the TIMX-696 alma test set (1,092,437 records) this affects one
record, alma:9935169041606761.

Relevant ticket(s):
* https://mitlibraries.atlassian.net/browse/TIMX-738
Why these changes are being introduced:
* Profiling the alma transform shows ~90% of per-record time in Marc field
extraction, most of it inside bs4's find/find_all, and ~10% in parsing. Moving
Marc to lxml requires lookups that behave exactly as the bs4 calls they replace,
so transformed records do not change.

How this addresses that need:
* Add Marc.get_element_string(), mirroring bs4's Tag.string: a single child
text, comment, or processing instruction node is returned, a single child
element is followed recursively, anything else is None. Whitespace-only strings
are collapsed to a newline or space, as bs4 stores them.
* Add Marc.find_all_elements() and Marc.find_element_string(), mirroring
find_all(name, **attrs) and find(name, string=True, **attrs).string, matching
local names in any or no namespace.
* Tests compare get_element_string() with bs4's Tag.string for text, whitespace,
entities, CDATA, nested elements, mixed content, comments and processing
instructions.
* Transformer.serialize_source_record() supports lxml elements, for transformers
given already parsed records.

Side effects of this change:
* None: lxml parsing is stubbed, but the transformer is still using BS4 mechanics

Relevant ticket(s):
* https://mitlibraries.atlassian.net/browse/TIMX-738
Why these changes are being introduced:
* Marc field extraction with bs4 is ~90% of alma transform time. lxml lookups
are much faster, and alma is by far the largest source.

How this addresses that need:
* Marc.parse_source_record() parses each record into an lxml element, with the
parser options bs4 uses (recover=True, strip_cdata=False). Other XML sources
still use bs4.
* All Marc field methods use the bs4-equivalent lxml helpers. find_all_elements()
matches single attribute values with lxml path predicates.
* The LOC country and language crosswalks are still bs4, as their lookups are
cached.
* get_main_titles() checks for a missing 245 field instead of catching an
AttributeError, and get_source_record_id() raises a ValueError when the 001
control field is missing, which still results in an error action.

Side effects of this change:
* Transformed records are unchanged: 100,000 alma records re-transformed from a
Timmy export, and the fixtures for every source, are identical before and after.
* alma is ~8x faster: ~790 records/sec in one process (was ~100), and ~4,100
records/sec with --workers 8 (was ~675).
* A missing 245 field is logged with logger.error() instead of
logger.exception(), so without a traceback.

Relevant ticket(s):
* https://mitlibraries.atlassian.net/browse/TIMX-738
@ghukill
ghukill force-pushed the timx-738-marc-lxml branch from 757d5d5 to 009a0bc Compare November 3, 2026 15:55
@ghukill
ghukill marked this pull request as ready for review November 3, 2026 18:30
@ghukill
ghukill requested a review from a team as a code owner November 3, 2026 18:30
@jonavellecuerdo jonavellecuerdo self-assigned this Nov 3, 2026
@ghukill
ghukill requested a balanced review from Copilot November 3, 2026 19:15

Copilot AI 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.

🟡 Changes recommended

Element serialization can include tail text and produce invalid standalone XML, while changed whitespace normalization alters transformed records.

2 open issues
What changed in this PR

This PR moves Alma’s MARC transformations from BeautifulSoup to lxml, complementing the parallel-worker changes in #282 to accelerate full-corpus processing while aiming to preserve transformed output.

Changes:

  • Adds lxml-based MARC parsing and lookup helpers, and migrates field extraction to use them.
  • Supports serialization of parsed lxml records and makes contributor-kind ordering deterministic.
  • Updates MARC tests and adds coverage for helper behavior, parsing, error handling, and serialization.
File Description
transmogrifier/​sources/​xml/​marc.py Migrates MARC parsing and field extraction to lxml.
transmogrifier/​sources/​transformer.py Adds serialization support for lxml elements.
tests/​sources/​xml/​test_marc.py Adapts MARC tests and covers new helpers and ordering behavior.
tests/​sources/​test_transformer.py Tests serialization of parsed lxml records.

Review effort: Balanced

Comment thread transmogrifier/sources/transformer.py Resolved
Comment thread transmogrifier/sources/xml/marc.py Resolved
@ehanson8 ehanson8 self-assigned this Nov 3, 2026
Why these changes are being introduced:
* bs4 collapses strings of only ASCII whitespace to a single newline or space
when parsing, but Marc.get_element_string() returned them as-is.  Field
methods that do not strip values, e.g. summary from a whitespace-only 520 $a,
could differ from bs4.

How this addresses that need:
* Marc.get_element_string() collapses whitespace-only text and comment strings
as bs4 does.
* Whitespace cases are now compared with bs4's Tag.string, replacing the test
that expected them unchanged.

Side effects of this change:
* None: restores bs4 parity for whitespace-only strings.

Relevant ticket(s):
* https://mitlibraries.atlassian.net/browse/TIMX-738
Why these changes are being introduced:
* etree.tostring() includes an element's tail by default, so serializing an
already parsed record taken from a parent element could store trailing text,
e.g. "<record/>after".

How this addresses that need:
* Transformer.serialize_source_record() serializes lxml elements with
with_tail=False, as XMLTransformer.read_source_file() does.

Side effects of this change:
* None: only used when a transformer is given already parsed records.

Relevant ticket(s):
* https://mitlibraries.atlassian.net/browse/TIMX-738

@jonavellecuerdo jonavellecuerdo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving based on the Timmy report comparing records in Alma before and after any of the changes in the last three (stacked) PRs!

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.

4 participants