[jonavellecuerdo]

Purpose and background context

Transmogrifier can read and transform source records from the TIMDEX dataset for a given (data) source. As described in TIMX-700, the motivation for this feature was to enable transformation for a subset of TIMDEX records. This does not imply subsetting the harvested records during the "extract" (or harvest) step of the TIMDEX ETL pipeline. Given that source records are already stored in the TIMDEX dataset, rerunning the "extract" step isn't required either.

At this time, this feature only supports reading and transforming source records associated with all "current" records for a given TIMDEX source, as this is expected to be the most common scenario we will encounter. That is:

Changes are made to a metadata transformation (a Transformer in Transmogrifier) -> For each TIMDEX source that relies on the Transformer, re-transform all "current" source records

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

These tests were run using a local TIMDEX dataset on my machine (I used Rclone to sync a local dataset with the TIMDEX dataset in Dev at some point previously).

1. The following env vars were set on my machine:

   TDA_LOG_LEVEL=DEBUG
   WARNING_ONLY_LOGGERS=asyncio,botocore,urllib3,s3transfer,boto3,MARKDOWN
   TIMDEX_DATASET_LOCATION=/Users/jcuerdo/timdex/dataset
   

2. Launch IPython shell and ran the following commands:

import os

import pandas as pd

from timdex_dataset_api import TIMDEXDataset
from timdex_dataset_api.config import configure_dev_logger
 
configure_dev_logger()
td = TIMDEXDataset(os.environ["TIMDEX_DATASET_LOCATION"])
td.metadata.rebuild_dataset_metadata()

# create dataframe with transformed records for 'gismit' source
gismit_transformed_records = list(td.records.read_transformed_records_iter(table="current_records", source="gismit", action="index"))
gismit_transformed_records_df = pd.DataFrame(gismit_transformed_records)

# select column to review
print(gismit_transformed_records_df["content_type"].describe())

Which prints the output:

count               2061
unique                 9
top       [Polygon data]
freq                1090
Name: content_type, dtype: object

3. Temporarily update the MITAardVark transformer so that the content_type value is updated. For testing purposes, I'm appending "Cool" to each content type.

@classmethod
    def get_content_type(cls, source_record: dict) -> list[str] | None:
        if content_types := source_record.get("gbl_resourceType_sm"):
            return [f"Cool {content_type}" for content_type in content_types]
        return None

4. To avoid clobbering my local TIMDEX dataset, I copied the contents of my local TIMDEX dataset to a new location: /Users/jcuerdo/timdex/cooler-dataset/ (Note: "cooler" prefix)

5. Now, let's try to rerun the transformer using the source records for the 2,061 records for the 'gismit' source in the TIMDEX dataset. Open another terminal and run the command:

uv run --env-file=.env transform -s gismit -id /Users/jcuerdo/timdex/dataset -o /Users/jcuerdo/timdex/cooler-dataset/

6. Let's read the COOLER dataset. Return to IPython shell:

cooler_td=TIMDEXDataset("/Users/jcuerdo/timdex/cooler-dataset/")
cooler_td.refresh()

gismit_transformed_records = list(cooler_td.records.read_transformed_records_iter(table="current_records", source="gismit", action="index"))
gismit_transformed_records_df = pd.DataFrame(gismit_transformed_records)

# select column to review
print(gismit_transformed_records_df["content_type"].describe())

Which prints the output:

count                    2061
unique                      9
top       [Cool Polygon data] # <---- Reflects change to append "Cool" to all `content_type` values
freq                     1090
Name: content_type, dtype: object

Includes new or updated dependencies?

YES: Added freezegun as dev dependency (using uv add --dev)

Changes expectations for external applications?

YES:

• timdex-pipeline-lambdas needs to generate a transform command that reads and transforms source records from the TIMDEX dataset
• TIMDEX step function needs to be updated to support input payloads that with required key-value pairs for transforming **all "current" records for a given TIMDEX source.

What are the relevant tickets?

• https://mitlibraries.atlassian.net/browse/TIMX-700

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.

[Copilot]

🟡 Changes recommended

Omitting --output-location for a dataset-only invocation leaves the writer’s location as None instead of defaulting to the input dataset path.

1 open issue

• Default output location to input dataset · New

What changed in this PR

Adds support for re-transforming a source’s current indexed records directly from a TIMDEX dataset, without repeating the harvest step.

Changes:

• Adds dataset input to the CLI, with mutually exclusive file and dataset options.
• Reads stored source records, parses JSON or XML, and generates patch run metadata.
• Adds parsing tests and updates existing tests and fixtures.

File	Description
transmogrifier/​sources/​xmltransformer.py	Parses individual stored XML records.
transmogrifier/​sources/​transformer.py	Loads dataset records and generates dataset-backed run metadata.
transmogrifier/​sources/​jsontransformer.py	Parses stored JSON records and validates dictionary output.
transmogrifier/​cli.py	Adds dataset input and validates input/output options.
tests/​sources/​test_transformer.py	Tests record parsing and updates metadata assertions.
tests/​sources/​json/​test_libguides.py	Simplifies fixture formatting without changing behavior.
tests/​conftest.py	Updates the generic fixture for the new abstract parsing method.

_{Review effort: Balanced}

[ghukill]

Full disclosure, I went into this quite nit-picky given that I'll be using this functionality soon and I think we may want to extend and build upon in later!

Overall, I think the approach is simple and elegant. 90% of my comments are about namings of things and docstrings, much of which is baggage from previous changes that just now tip into feeling like it's worth some attention.

[ghukill]

This comment is a catch-all for the CLI options.

As mentioned in a slack huddle, I think these arguments are getting a little confusing and overloaded. This is not the fault of this PR, as it was heading that direction when we started supporting writing to a TIMDEX dataset versus individual files.

One variable here is that I think these will continue to change in the coming weeks / months (e.g. the ability to read parquet files as input), so I'd propose that any changes now mostly serve to be explicit and obvious, making possible future additions or changes easier. And, gain a little bit of clarity now.

What do you think about the following set of arguments @jonavellecuerdo?

• --input-file
  • unchanged/existing
  • read an input file for parsing + transforming, could be local or S3
  • this would remain the default assumed used argument in the StepFunction
• --input-dataset
  • changed/new
  • indicates that this TIMDEX dataset is used for input and nothing to do with the outputs of the transformed records
• --output-file
  • changed/new
  • explicitly states that output is to a local/S3 file
  • this was the default mode pre TIMDEX dataset, is not any longer, but remains helpful for quick debugging
• --ouput-dataset
  • changed/new
  • explictily states that the output records are written to this TIMDEX dataset

This would require a small change in the pipeline lambdas: update --output-location to be --output-dataset. I'd propose that this is more explicit, easier to understand.

Why do all this?

• input validation is easier: one --input-* and one --output-* arg required, and only one of each
• if we support new input/output options, just add them, e.g. --input-parquet and --output-parquet
• part of the required work for this PR is an update to the pipeline lambdas already, so a decent time to align to the new --output-dataset argument name

[jonavellecuerdo]

Made changes that result in the following CLI options -- see the latest commit e8b700c:

--i / --input-file
--id / --input-dataset
--o / --output-dataset

I realized that we no longer have any write_*() methods on Transformer that write to a file; the only method on Transformer is write_to_parquet_dataset().

Are you requesting restoring a Transformer's ability to write to an output file (not just a TIMDEX dataset)? 🤔

[ghukill]

@jonavellecuerdo - reviewing now and just wanted to confirm: you opted to drop / not include --output-file as an option?

FWIW, I'm okay with that, given some work I have scoped and ready to PR. It will introduce a new --output-parquet which is honestly what I'd use over --output-file any day.

[jonavellecuerdo]

I didn't included --output-file as an option because the Transformer class no longer has a method to write to a non-parquet dataset (as in a JSON Lines file) 🤔

[ghukill]

Righto! I couldn't remember where we landed on that, or if any supporting code existed. Sounds like it didn't. Agreed, not needed.

[ghukill]

I'd be remiss if I didn't add that I may propose adding this in one of my upcoming commits, to give parity with --input-file. If so, it's not a sleight on this PR! I think it was the right call to not add it now. My commit / PR will introduce --input-parquet and --output-parquet so it's building up a 3 option in, 3 option out setup.

[ghukill]

I like this here! I was going to propose that we move to the __init__ method -- and maybe that should also have it? -- but it's needed here I think, because the if/else below would take the first one available and could quietly be confusing. This is a good explicit check here.

[jonavellecuerdo]

Here were some conclusions/points we arrived at during our discussion on 10/29:

Transformer.__init__():

• We should be able to instantiate a Transformer without providing a source_file and source_dataset.
  • The only required params are source and source_record.
• During instantiation, source_file and source_dataset is only needed when calling self.get_run_data().

Transformer.get_run_data():

• Only required parameter is source.
• source_file and source_dataset are still optional because the method falls back to default values. Namely:

   run_data = {
           "source": source,
           "run_date": current_datetime.strftime("%Y-%m-%d"),
           "run_type": "daily",
           "run_id": run_id or str(uuid.uuid4()),
           "run_timestamp": run_timestamp or current_datetime.isoformat(),
       }

• An error is only raised if both source_file and source_dataset are provided
  • Why: Run data is either parsed from source_file OR configured for source_dataset; cannot be both.

[ghukill]

@jonavellecuerdo - not a hard requirement, but may want to consider rebasing against main as this PR for TIMX-692 was merged since you started this branch I think.

[ghukill]

Approved! Nice work, thanks for the good discussions.

[ehanson8]

Looks great, the discussion landed this update in a solid place!