Repository navigation
Conversation
| # cache of LOC crosswalk code element lookups, keyed by (code type, code) | ||
| _loc_crosswalk_code_cache: dict[tuple[str, str], Tag | None] = {} |
There was a problem hiding this comment.
This was a surprising finding for me. I had not considered the ramifications of our crosswalk data stored as XML, and using bs4 for looking up values.
By caching values as we parse and find them, we get a 2x speedup for alma transformations.
The approach may seem opaque at first, but it's pretty conceptually simple:
- create this
_loc_crosswalk_code_cachecache object - as we encounter a crosswalk type + value (e.g. "locations" and "Stacks") we check if a cached crosswalk value exists, and return if so
- if not, we use the same
bs4find(...)approach, get the value and cache it for next time
There was a problem hiding this comment.
Interesting, I wasn't expecting that much of an effect either
| if self.workers > 1: | ||
| return self._iter_with_workers() |
There was a problem hiding this comment.
This part is critical.
Formerly, the Transformer.__iter__() method returned self, indicating that if we "iterate overe" a Transformer instance we'd just keep calling __next__(). That is purely sequential.
Now, if workers > 1, iterating over a Transformer instance will invoke self._iter_with_workers() which introduces the parallel processes.
| while True: | ||
| transformed_record = None | ||
| timdex_record_id = None | ||
| def _iter_with_workers(self) -> Iterator[DatasetRecord]: |
There was a problem hiding this comment.
As noted above, if workers > 1, then we use this method skipping the simpler __iter__() --> __next__() approach previously used.
Now, we do work in two distinct pieces:
1- get a chunk of input source records via:
for chunk in itertools.batched(
self.source_records, WORKER_CHUNK_SIZE, strict=False
):2- and for each chunk, transform via module level helper function _transform_in_worker():
pending.append(
(
records, <-------- chunk of records
executor.submit(
_transform_in_worker, <---------------- function to transform
[(record, next(offsets)) for record in records],
),
)
)If we look at _transform_in_worker we see that it uses _process_source_record, just like __next__() for a single worker approach:
def _transform_in_worker(chunk: list[tuple[bytes, int]]) -> list[TransformResult]:
"""Transform a chunk of (source record, run_record_offset) in a worker process."""
if _worker_transformer is None:
raise RuntimeError("Worker transformer was not initialized")
return [
_worker_transformer._process_source_record(record, run_record_offset) # noqa: SLF001
for record, run_record_offset in chunk
]Said more generally: when we do parallel work, we can't use __next__(), because we can't pass around the Transfomer instance between processes! Therefore, both __next__() and this parallel approach both use the same method ._process_source_record() which is safe for a single or parallel context.
There was a problem hiding this comment.
I always appreciate an explanation when parallel processing or multithreading is involved. Thanks for this write-up!
| return self._create_dataset_record(raw_source_record, transform_result) | ||
|
|
||
| @final | ||
| def _process_source_record( |
There was a problem hiding this comment.
All this code was formerly in __next__() but was moved out to be compatible with parallel worker contexts.
Why these changes are being introduced: * Profiling an alma transform showed ~45% of time spent in Marc.loc_crosswalk_code_to_name(), which searches the entire LOC language or country crosswalk with bs4 for every code on every record. How this addresses that need: * Cache the code element found for each code, per crosswalk, so each distinct code is only searched once. * The cache stores the exact result of the original find(), preserving first-match behavior for codes that appear more than once in a crosswalk (e.g. 'ai' is an obsolete Anguilla code before it is Armenia's current code). * DEBUG logging for invalid and obsolete codes is unchanged, per record. Side effects of this change: * Output is unchanged: 2,000 alma records re-transformed from a parquet input produced identical transformed records. ~1.7x faster for alma end to end (~55 -> ~92 records/sec, single process). Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-738 * https://mitlibraries.atlassian.net/browse/TIMX-343
Why these changes are being introduced: * To transform records in parallel worker processes, the main process should only read source records, not parse them. Parsing with bs4 is ~1 ms per record, which would cap a parallel run near ~1,000 records/sec, and bs4 objects do not pass between processes well. How this addresses that need: * Add Transformer.read_source_file(), implemented by XMLTransformer and JSONTransformer, which yields each source record as bytes: lxml's serialization of each XML <record> element, or each non-blank JSONLines line. * Rename parse_source_records_from_dataset() and parse_source_records_from_parquet() to read_source_records_from_*(), which yield the stored source_record bytes. * Transformer.load() passes these bytes iterators to the transformer, and Transformer.__next__() parses each record with parse_source_record() before transform(). * Transformer.parse_source_file() remains, as read_source_file() plus parse_source_record(), and transformers can still be given already parsed records. * ResearchDatabases overrides read_source_file() to append its synthetic delete records as bytes, replacing its copy of the XML file parsing. * Remove the jsonlines dependency, no longer used. Side effects of this change: * Stored source_record is unchanged: still serialized from the parsed record. * A source record that cannot be parsed is now an 'error' action for that record, rather than an exception that stops the run. Blank lines in JSONLines files are skipped. * Output is unchanged: fixture files for every source, and 2,000 alma records from a Timmy export, produced identical source and transformed records. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-738
Why these changes are being introduced: * Source records are now read as bytes before parsing, so the bytes read can be stored as-is, instead of re-serializing the parsed bs4 Tag or dict. This keeps the stored source record closer to what was harvested, and lets worker processes return only transformed records while the bytes stay in the main process. How this addresses that need: * Transformer.__next__() stores the source record bytes read from the input as DatasetRecord.source_record. * serialize_source_record() is only used when a transformer is given already parsed records. Side effects of this change: * Deliberate change to stored source_record for file inputs: * XML: lxml's serialization of each <record> element, without an added <?xml ...?> declaration and with original whitespace. This is not a byte slice of the file: a namespace declared on a parent element is copied onto the record, and recover=True can repair malformed XML. * JSON: the original line, without json.dumps() re-escaping. * Dataset and parquet inputs pass the stored bytes through untouched. * One-time churn: the first run after deploy writes new source_record bytes for every XML and JSON file record, so each will show a source change once. * Transformed records are unchanged: fixture files for every source produced identical transformed records, and 2,000 alma records produced identical source and transformed records. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-738
Why these changes are being introduced: * To transform records in worker processes, the work done for one record must be separate from updating the transformer's run state, which has to happen in the main process in run order. How this addresses that need: * Add Transformer._process_source_record(), which parses and transforms one source record and returns a small, picklable TransformResult of action, timdex_record_id, and transformed_record. It catches DeletedRecordEvent, SkippedRecordEvent, and unhandled exceptions as actions, lets CriticalError propagate, and changes no transformer state. The run_record_offset for provenance is passed in. * Add Transformer._update_record_counts(), which updates record counts and deleted records from a TransformResult. It is a separate method, rather than part of __next__(), so that the upcoming parallel workers path, which does not go through __next__(), can reuse the same bookkeeping. * Add Transformer._create_dataset_record(), which builds the DatasetRecord from the source record, TransformResult, and run data. * Transformer.__next__() now calls these three in order, and its docstring describes what must stay in the main process. Side effects of this change: * None: fixture files for every source, and 2,000 alma records from a Timmy export, produced identical source and transformed records. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-738
Why these changes are being introduced: * Alma transforms are CPU bound at ~90 records/sec per process, mostly bs4 querying, which makes full re-transforms of ~1M records slow. How this addresses that need: * Add CLI option --workers (default 1). With 1, records are transformed in the main process exactly as before. * With more than 1, iterating the transformer uses a pool of worker processes. The main process reads source records as bytes, and sends them in chunks of 100, each with its run_record_offset, to workers. Workers return only TransformResults; the source bytes stay in the main process. Results are handled in input order, with the same main process bookkeeping as __next__() (_update_record_counts() and _create_dataset_record()), so output is identical to a single process. * At most 4 chunks per worker are in flight, which bounds memory. * Workers are started with "spawn" explicitly, so behavior is the same on macOS and Linux (e.g. ECS) and does not depend on the platform or Python version default. Each builds its own instance of the transformer class (and so its own crosswalk cache) with the main process's run data, logging, and Sentry configuration. * A CriticalError in any worker stops the run, and pending chunks are cancelled. Side effects of this change: * 100,000 alma records from a Timmy export: 2m28s with --workers 8 (~675 records/sec, vs ~92 in one process), and identical source and transformed records. * Production runs are unchanged unless --workers is passed. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-738 * https://mitlibraries.atlassian.net/browse/TIMX-343
Why these changes are being introduced: * The README did not describe the --workers option, or that source records are now stored as read. How this addresses that need: * Describe how source records are stored for each input. * Add a workers and performance section, and refresh the transform --help output. Side effects of this change: * None. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-738
34e5469 to
a630427
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Worker-enabled incremental consumption can lose prefetched records or bypass pending results because iteration and next() do not share a persistent worker iterator.
3 open issues
What changed in this PR
This PR adds optional process-based parallel transformation to Transmogrifier, building on the preceding Parquet support to make large transformation runs faster.
Changes:
- Adds
--workers, ordered worker processing, and centralized record bookkeeping. - Defers parsing until transformation and preserves original source-record bytes.
- Caches MARC crosswalk lookups, updates Parquet reading, and expands tests and documentation.
| File | Description |
|---|---|
uv.lock |
Removes the jsonlines dependency. |
transmogrifier/sources/xmltransformer.py |
Reads XML records as bytes for deferred parsing. |
transmogrifier/sources/xml/researchdatabases.py |
Adapts indexing and deletion records to byte-based input. |
transmogrifier/sources/xml/marc.py |
Caches crosswalk lookups to reduce repeated searches. |
transmogrifier/sources/transformer.py |
Adds worker orchestration, separates bookkeeping, and updates input readers. |
transmogrifier/sources/jsontransformer.py |
Reads JSONLines records as bytes without jsonlines. |
transmogrifier/cli.py |
Exposes the worker-count option. |
tests/test_cli.py |
Tests worker configuration and validation. |
tests/sources/xml/test_researchdatabases.py |
Updates expectations for byte-based reading. |
tests/sources/xml/test_marc.py |
Tests cached crosswalk behavior. |
tests/sources/test_transformer.py |
Tests byte preservation, bookkeeping, and worker behavior. |
tests/conftest.py |
Updates the transformer fixture’s reader interface. |
README.md |
Documents workers, performance, and source-byte preservation. |
pyproject.toml |
Removes the direct jsonlines dependency. |
Review effort: Balanced
ehanson8
left a comment
There was a problem hiding this comment.
Impressive improvement in speed, great work!
Why these changes are being introduced: * With workers, each iter() call created a new worker iterator, and next() bypassed it. Partially consuming one iterator, then iterating again or calling next(), dropped records already sent to workers. Unlikely, as the write methods consume the transformer once, but a good edge case to guard against, e.g. if used as a library. How this addresses that need: * Transformer.__iter__() creates the worker iterator once and reuses it. * Transformer.__next__() takes the next record from it when using workers. Side effects of this change: * None. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-738
Why these changes are being introduced: * The worker parity test used 4 records with a chunk size of 100, so only one chunk was submitted, and draining pending chunks at the bound was never exercised. How this addresses that need: * Patch WORKER_CHUNK_SIZE and WORKER_MAX_PENDING_CHUNKS_PER_WORKER to 1 in the parity test, so records span 4 chunks, 2 are in flight, and pending chunks are drained. Order, provenance offsets, and counts are compared against a single process as before. Side effects of this change: * None. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-738
jonavellecuerdo
left a comment
There was a problem hiding this comment.
This is really exciting! I just have one small request to include more detail in a docstring.
| _worker_transformer = transformer_class( | ||
| source, iter(()), exclusion_list_path=exclusion_list_path | ||
| ) |
There was a problem hiding this comment.
Hmm, because this _init_worker method is specifically called as part of Transformer._iter_with_workers() method, it did give me pause to see iter(()) being passed in to the source_records parameter. From my understanding, this empty iter is not relevant / used by the _iter_with_workers() workflow but was only needed to instantiate a Transformer class. 🤔
Can you update the docstring to clarify this information?


Note
This PR is stacked on #281.
Purpose and background context
This 2nd PR in the stacked PRs for TIMX-738 introduces the ability to perform transformations via parallel worker processes. While this is the most impactful change, there are also some more minor (but still impactful) performance changes to the
almasource specifically.Until now, Transmogrifier was a single process, sequentially processing application. The architecture was a
Transformerclass that defines a__next__()method, effectively making it an iterator. If one were to callnext(my_transformer)you would get aDatasetRecordinstance which is the containing object of a transformed record.For most sources, where records to transform are low, this is inconsequential. Total runtime maybe seconds to low minutes. But for sources like
dspace, more recentlydigitalcollections, and certainlyalmathe single process, sequential nature of Transmogrifier becomes a major barrier to speed. Speed is not everything... but Transmogrifier is slow enough that it prohibits making substantial changes to transformation logic because full runs are expensive and cumbersome to complete. This is a problem.Thankfully, much of the architecture lends itself well to parallelization!
This PR reworks the Transformer
__next__(),__iter__(), andtransform()methods slightly to lend themselves to transforming via parallel processes. To be precise, we are talking processes and not threads, where most of the time spent is CPU bound and threads would not be a meaningful speed increase.This matters, because cross-process parallelization in python requires objects that are pickable/serializable to pass between processes. This required shifting when and where we parse raw source record bytes into BeautifulSoup4 objects (
Tag) or dictionaries. By shifting that into code that would be isolated per process, the__next__()method becomes more of an orchestrator than transformation and business logic. This is arguably also a good thing! We as developers shouldn't have to muck around in this method once; it's preferable that farm out to methods we do touch.The net effect is a dramatic speedup. Where a transformation may have taken 60 minutes previously, if we run Transmogrifier with
--workers 8and parallelize across 8 workers, we'd see a runtime of about 7.5 minutes (60 / 8). As we'll see in the final PR there is still more room for significant performance improvements, but parallelization is a major one.How can a reviewer manually see the effects of these changes?
1- Set Dev1 TIMDEX AWS credentials
2- Sync data from S3 locally to isolate network speed variables:
3- Run Transmogrifier with a single worker (default also if
--workersis not provided):On my machine, this process takes around 165 seconds, which for a subset containing 13,941 records works out to roughly 87 records/second:
4- Now, we'll run with 8 parallel workers:
This completes in roughly 27 seconds, which works out to 516 records/second, overall a roughly 6x speedup:
Extrapolating that out, where something may have taken 3 hours it would now take 22.5 minutes. Even before the next pass of performance improvements in the next PR, we can now perform runs over substantial amounts of records to preview changes we are making.
Includes new or updated dependencies?
YES
Changes expectations for external applications?
NO: at this time, if
--workersis excluded, we default to a single process. However, we may choose to allow the pipeline lambdas to set--workersfor the Transmogrifier jobs in the future. And/or, update Transmogrifier to identify how many CPU cores there are and attempt an automatic, conservative setting.What are the relevant tickets?
Code review