TIMX 496 - add migrations folder and run_timestamp migration#147
Conversation
Why these changes are being introduced:
We have a need to perform a onetime bulk edit of the parquet dataset,
which is very similar to a SQL database migration: addition of a column
and backfilling of data.
While we may not require the strictness of SQL schema migrations, which
often can be "replayed" in order to upgrade or reverse order to downgrade,
we nonetheless would benefit from some structure and code-as-documentation
for bulk operations we perform on the actual parquet dataset.
How this addresses that need:
* Creates a new root level migrations/ folder
* Includes a README with a proposed simple structure for migrations (mostly
naming conventions)
* Includes our first migration to backfill all pre-existing parquet files
with a new 'run_timestamp' column
* The run_timestamp will be pulled from the creation date of the parquet
file in S3, which is close enough for our purposes
Side effects of this change:
* TDA becomes the location for storing code related to "migrations" for
the parquet dataset
* All pre-existing parquet files will now have a run_timestamp column
Relevant ticket(s):
* https://mitlibraries.atlassian.net/browse/TIMX-496
| # Read all rows from the parquet file into a pyarrow Table | ||
| # NOTE: memory intensive for very large parquet files, though suitable for onetime | ||
| # migration work. | ||
| table = parquet_file.read() | ||
|
|
||
| # Get S3 object creation date | ||
| creation_date = get_s3_object_creation_date(parquet_filepath, dataset.filesystem) # type: ignore[attr-defined] | ||
|
|
||
| # Create run_timestamp column using the exact schema definition | ||
| num_rows = len(table) | ||
| run_timestamp_field = TIMDEX_DATASET_SCHEMA.field("run_timestamp") | ||
| run_timestamp_array = pa.array( | ||
| [creation_date] * num_rows, type=run_timestamp_field.type | ||
| ) | ||
|
|
||
| # Add the run_timestamp column to the table | ||
| table_with_timestamp = table.append_column("run_timestamp", run_timestamp_array) | ||
|
|
||
| # Write the updated table back to the same file | ||
| if not dry_run: | ||
| pq.write_table( | ||
| table_with_timestamp, # type: ignore[attr-defined] | ||
| parquet_filepath, | ||
| filesystem=dataset.filesystem, # type: ignore[attr-defined] | ||
| ) | ||
| logger.info(f"Successfully updated file: {parquet_filepath}") | ||
| else: | ||
| logger.info(f"DRY RUN: Would update file: {parquet_filepath}") |
There was a problem hiding this comment.
I think this is arguably the most complex part of this migration (and potentially future migrations). Everything else is loop structure and familiar operations (like getting S3 data).
What we do here is:
- read the full parquet file into memory as a pyarrow table
- create a new pyarrow field
run_timestamp, using our schemaTIMDEX_DATASET_SCHEMAto ensure we get the type right - create an array of values for this field, all the same value, that matches the lenght of the parquet file we read into memory
- add this to the in-memory pyarrow table as a new column
- write the pyarrow table back to S3, overwriting the original parquet file
We have some gaurantees that make this safe:
- we use our own
TIMDEX_DATASET_SCHEMAto ensure the newrun_timestampcolumn has the right type - when we perform the overwrite, the schema of the pyarrow table is the same as when we read it from S3
jonavellecuerdo
left a comment
jonavellecuerdo
left a comment
There was a problem hiding this comment.
Just a couple of clarification questions and minor change requests. I also have some follow-up questions on some of the information included in the PR description:
- The PR description reads:
we could explore how this migrations/ structure could impose some mandatory guardrails like making it transactional (e.g. a full dataset backup --> perform migration --> remove backup pattern).
Can you clarify what it would mean to make migrations "transactional"? I don't think I understood the example noted in parentheses. 🤔 - The PR description reads:
The data backup used here, unfortunately, updated the S3 object create time on copy so the run_timestamp for each is 2025-05-30. But some clicking around in the real dataset should confirm that it will pickup accurate dates for the real migration work.
- How are data backups performed in production?
- Was this backup manually created?
- When you mention "clicking around in the real dataset", are you referring to "clicking around the actual dataset in S3 and observing the "Last modified" dates shown in the console?"
|
|
||
| ## Structure | ||
|
|
||
| Each migration is either a single python file, or a dedicated directory, with that follows the naming convention: |
There was a problem hiding this comment.
Can you clarify "with ..."? 🤔
There was a problem hiding this comment.
Haha, sorry if poorly worded.
The "with" is referring to "Each migration is either a single python file, or a dedicated directory,..."
I intended to convey:
"...each migration is a single python file, or a dedicated directory, and therefore the following naming conventions should be used..."
There was a problem hiding this comment.
Got it! If adding this blurb, as suggested in #147 (comment), can you make this small update, too? 🤓
| if creation_date.tzinfo is None: | ||
| creation_date = creation_date.replace(tzinfo=UTC) |
There was a problem hiding this comment.
To clarify, if creation_date.tzinfo is None, the creation_date value remains the same, the tzinfo is just set to UTC?
Is the following scenario possible:
run-0hascreation_date=datetime.datetime(2025, 6, 3, 15, 10, 50, 996767, tzinfo=datetime.timezone.utc))run-1hascreation_date=datetime.datetime(2025, 6, 3, 11, 05, 50, 996767, tzinfo=None)- Because
tzinfo=None, the end value isdatetime.datetime(2025, 6, 3, 15, 5, 50, 996767, tzinfo=datetime.timezone.utc).
?
- Because
When ordering these runs, run-1 appears to have occurred before run-0. 🤔
There was a problem hiding this comment.
To clarify, if creation_date.tzinfo is None, the creation_date value remains the same, the tzinfo is just set to UTC?
Yes, that's correct. The timestamps in S3 are UTC when pulled down, so we're just ensuring the value is timezone aware.
Is the following scenario possible:
I think we can safely ignore this edge case, as all dates are coming from S3 and either will or will not (but they will!) have a timezone attached. This code is kind of just generically being safe to add timezones if not present. But it's assuming that all datetimes coming back are coming from the same source... so they are all with UTC timezone... or all without it.
There was a problem hiding this comment.
Can we add this lil' blurb to the method definition?:
This assumes that all datetimes coming back are coming from the same source and will be formatted similarly, which means either all values are timezone aware or not.
|
@jonavellecuerdo - some answers to your review comment:
This has been performed, informally, by making a copy of the We rely on S3 having backups if we were to accidentally lose the
The dataset backup outlined in the PR was created manually, but I have since removed it. There is a new on here, But it's tricky: these backups aren't that meaningful / helpful unless we created them automatically at the time a migraiton is run, such that they are up-to-date. This PR does not actually handle the creation or management of backups. That was mostly just conversation and observing that we could do that.
Yep! |
|
@jonavellecuerdo - updates per discussion and comments here. Good questions all around! I should have been clearer in the |
| This directory stores data and/or schema modifications that were made to the TIMDEX parquet dataset. Consider them like ["migrations"](https://en.wikipedia.org/wiki/Schema_migration) for a SQL database, but -- at least at the time of this writing -- considerably more informal and ad-hoc. | ||
|
|
||
| Unless otherwise noted, it assumed that these migrations were: | ||
|
|
||
| * manually run by a developer, either on a local machine or some cloud operations | ||
| * have been performed already, should not be performed again | ||
| * the migration script does not contain a way to rollback the changes |
There was a problem hiding this comment.
Clear as a cucumber 🥒 !
Pull Request Test Coverage Report for Build 15424246153Details
💛 - Coveralls |
3 participants
Purpose and background context
This PR introduces a new root level
migrations/folder where we can locate scripts used for making bulk updates to the parquet dataset. As noted in the commit, while not as formal as migrations for SQL databases -- which often support "replaying" the migrations in an upward or downward fashion -- it provides some structure and code-as-documentation for bulk operations we may need to perform on the TIMDEX parquet dataset.The following is a rendered form of the
migrations/folder README: https://github.com/MITLibraries/timdex-dataset-api/tree/TIMX-496-establish-migrations-and-backfill-migration/migrations.If we find ourselves needing to do this fairly often, or make invasive changes, we could explore how this
migrations/structure could impose some mandatory guardrails like making it transactional (e.g. a full dataset backup --> perform migration --> remove backup pattern).The first migration added is related to TIMX-496, backfilling all pre-existing parquet files with a
run_timestampcolumn. The value for that column, for each file, is pulled from the parquet file's creation date in S3. For our purposes, this is accurate enough.As noted in #146, the order of actually running this migration will be as follows:
run_timestampcolumn for all future runsrun_timestampcolumnApproval and merge of this PR is for code review only; the migration will be manually run from a developer's machine according to this plan.
How can a reviewer manually see the effects of these changes?
The migration can be run with a
--dry-runflag, showing what will happen before we actually rewrite any parquet files.The following dry-run will not perform any write operations back to S3, but it will download full parquet files locally for a moment in memory. You can
ctrl + canytime once satisfied with early debugging log statements.The data backup used here, unfortunately, updated the S3 object creaete time on copy so the
run_timestampfor each is2025-05-30. But some clicking around in the real dataset should confirm that it will pickup accurate dates for the real migration work.1- Set AWS production credentials in terminal
2- Perform a dry run of a backup of the production dataset:
As noted above, you can
ctrl + cto kill the process anytime after you are satisfied with how it's working sans actually writing a new parquet file to S3.Includes new or updated dependencies?
NO
Changes expectations for external applications?
YES(ish)
What are the relevant tickets?
Developer
Code Reviewer(s)