Add batch creation method to Workflow abstract base class #200

jonavellecuerdo · 2025-09-16T17:28:22Z

Purpose and background context

Support DSC workflows that require programmatic batch creation.

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

For now, passing unit tests will suffice.

Includes new or updated dependencies?

NO

Changes expectations for external applications?

YES - Enables the addition of a "batch creation" step to the DSC workflow.

What are the relevant tickets?

https://mitlibraries.atlassian.net/browse/IN-1455

Why these changes are being introduced: * Adding an explicit 'batch creation' step to the DSC workflow means a new method that can handle all init-level operations, such as persisting records to DynamoDB for the first time. This allows for simplifying some of the methods in the ItemSubmission and ItemSubmissionDB models. How this addresses that need: * Refactor ItemSubmissionDB.create as a conditional save method; instances are created by calling the ItemSubmissionDB constructor * Add save method to domain model, which uses ItemSubmissionDB.create to perform conditional save * Deprecate unused ItemSubmissionDB.get_or_create method Side effects of this change: * This encourages the following approach: if writing new records, use ItemSubmission.create + ItemSubmission.save if updating existing records, set attributes on ItemSubmission() + ItemSubmission.upsert_db TLDR: Any upserts will update and overwrite an existing record in DynamoDB. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-1455

coveralls · 2025-09-16T17:32:28Z

Pull Request Test Coverage Report for Build 18017142515

Details

43 of 51 (84.31%) changed or added relevant lines in 5 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.5%) to 96.154%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
dsc/exceptions.py	7	8	87.5%
dsc/workflows/base/init.py	14	21	66.67%

Files with Coverage Reduction	New Missed Lines	%
dsc/item_submission.py	1	99.29%

Totals
Change from base Build 17497497241:	-0.5%
Covered Lines:	1075
Relevant Lines:	1118

💛 - Coveralls

jonavellecuerdo · 2025-09-16T17:45:58Z

dsc/workflows/base/__init__.py

+            item_submission = ItemSubmission.get(
                batch_id=self.batch_id,
                item_identifier=item_metadata["item_identifier"],
-                workflow_name=self.workflow_name,
-                source_system_identifier=item_metadata.get("source_system_identifier"),
            )

+            # if no corresponding record in DynamoDB, skip
+            if not item_submission:
+                continue


For now, the Workflow.reconcile_items method is simply updated to use ItemSubmission.get, removing the option to create the record during the reconcile step. This is because a record is now added during the create-batch step, which assumes the responsibility of creating an item in the DynamoDB table for every item in a batch.

Note: If ItemSubmission.get returns None--i.e., a corresponding record is not found in DynamoDB--the item submission is skipped from the reconcile. This raises the question of how to report these cases, as reconcile_items currently only tracks metadata without bitstreams and vice versa. I propose resolving this as outside of the scope of this ticket and will likely be revisited as we work on the Wiley workflow and can take a closer look.

Thanks for raising this @jonavellecuerdo. I'm appreciating the tension here given that reconcile was still using metadata to drive its iteration. And I think this may prompt another comment for me on that specific line.

Regardless of the outcome on the discussion, I support moving forward and maybe creating a ticket if needed.

I think if we switch to iteration from DynamoDB rather than the metadata, this check is unnecessary

ghukill

Looking really good to me, nice work on scaffolding this update.

Requesting comment for some inline comments, and this question:

Do we need a new CLI command for create-batch?

dsc/workflows/base/__init__.py

ghukill · 2025-09-17T19:08:54Z

dsc/workflows/base/__init__.py

+            item_submission = ItemSubmission.get(
                batch_id=self.batch_id,
                item_identifier=item_metadata["item_identifier"],
-                workflow_name=self.workflow_name,
-                source_system_identifier=item_metadata.get("source_system_identifier"),
            )

+            # if no corresponding record in DynamoDB, skip
+            if not item_submission:
+                continue


Thanks for raising this @jonavellecuerdo. I'm appreciating the tension here given that reconcile was still using metadata to drive its iteration. And I think this may prompt another comment for me on that specific line.

Regardless of the outcome on the discussion, I support moving forward and maybe creating a ticket if needed.

dsc/workflows/base/__init__.py

ehanson8

Great work! A few comments, mostly piggybacking off of @ghukill

dsc/item_submission.py

dsc/workflows/base/__init__.py

ehanson8 · 2025-09-18T13:20:12Z

dsc/workflows/base/__init__.py

+            item_submission = ItemSubmission.get(
                batch_id=self.batch_id,
                item_identifier=item_metadata["item_identifier"],
-                workflow_name=self.workflow_name,
-                source_system_identifier=item_metadata.get("source_system_identifier"),
            )

+            # if no corresponding record in DynamoDB, skip
+            if not item_submission:
+                continue


I think if we switch to iteration from DynamoDB rather than the metadata, this check is unnecessary

jonavellecuerdo · 2025-09-19T18:01:58Z

@ghukill @ehanson8

The latest commits were motivated by the discussion in #200 (comment). In order to support moving the item_metadata_iter into the batch creation step--specifically, during the writing of records to DynamoDB, the following updates were implemented:

Storing the source metadata as a serialized JSON in the DynamoDB table
- Adding a source_metadata property to ItemSubmission that parses the serialized JSON as needed
Setting the bitstream_s3_uris attribute during the batch creation step
- Removing assignment of bitstream_s3_uris from the submit step
Updating the methods for creating DSpace metadata in ItemSubmission to access source metadata from self

NOTE: While I put in effort to update existing tests, I've opted not to add to the existing test suite. I am hoping to do some test cleanup as part of the later tickets as adding new tests to the existing suite has proven to be quite difficult. I do think the existing tests do offer some value but could be refactored to be clearer and to better support new tests for added functions.

ghukill · 2025-09-19T18:33:18Z

Thanks for the summary @jonavellecuerdo. Before I jump into a review, I have a concern:

Storing the source metadata as a serialized JSON in the DynamoDB table

If I'm recalling correctly, DynamoDB has fairly small limits on row size, I think like 400kb or something. While I'm betting most metadata would fit, it feels risky to store it there.

Admiteddly, I have not fully embarked on a review to understand how and why this change was needed, but I do see this in a commit message:

Side effects of this change: * In storing the source metadata as serialized JSON in DynamoDB, any subsequent methods can simply read the metadata from DynamoDB whereas previously, it would have to reach into the metadata file in S3 each time.

If the goal is just to avoid S3 reads, I'm not sure Dynamo is a good fit. If part of the goal is to normalize how different workflows loop through metadata, that feels a bit different. Happy to chat it out if helpful, sync or async, but I do worry that Dynamo isn't up for the task of storing item metadata.

ehanson8 · 2025-09-19T19:00:54Z

If the goal is just to avoid S3 reads, I'm not sure Dynamo is a good fit. If part of the goal is to normalize how different workflows loop through metadata, that feels a bit different. Happy to chat it out if helpful, sync or async, but I do worry that Dynamo isn't up for the task of storing item metadata.

I was surprised by this as well. I know I was saying to "minimize calls to item_metadata_iter()" but I was more thinking about driving reconcile with DynamoDB post-create_batch rather than moving source metadata and bitstreams functionality away from submit. I don't know that create_batch needs to be handling that, so I think this can be a simpler set of changes just related to reconcile

ghukill · 2025-09-19T19:31:13Z

If the goal is just to avoid S3 reads, I'm not sure Dynamo is a good fit. If part of the goal is to normalize how different workflows loop through metadata, that feels a bit different. Happy to chat it out if helpful, sync or async, but I do worry that Dynamo isn't up for the task of storing item metadata.

I was surprised by this as well. I know I was saying to "minimize calls to item_metadata_iter()" but I was more thinking about driving reconcile with DynamoDB post-create_batch rather than moving source metadata and bitstreams functionality away from submit. I don't know that create_batch needs to be handling that, so I think this can be a simpler set of changes just related to reconcile

This was my understanding as well.

If we assume it feels correct that the result of create_batch would be two things:

a folder in S3 with all metadata and bitstreams
all intellectual items from that batch are in Dynamo

Then it feels like reconcile, submit, and finalize can all use Dynamo to drive their loop.

What this complicates is when you have a "loose" file in the batch of which there is no metadata. How can we know, if we are driving reconciliation by intellectual items in Dynamo, that came from metadata in the first place?

My answer would be ignoring loose files. Perhaps when things settle down we could go back with a microscope and try to figure out how to bubble up that reconciliation error, but it continues to feel odd. I think we should let metadata records drive the whole thing, obviously noting when we can't find any bitstreams for them, but choose to ignore when there are "loose" files in the batch. Or heck, perhaps reconciliation could identify them by process of elimination? If we unite metadata records with bitstreams (passing reconciliation), but we have a file left over at the end, could we:

Mark every row in Dynamo as reconciled=passed because all items with metadata have bitstreams
Have the reconciliation job as a whole still fail, becaus of this error

ehanson8 · 2025-09-19T19:54:07Z

My answer would be ignoring loose files.

I agree, this is a minimal risk in the first place but we could have a ticket to address the issue later

jonavellecuerdo · 2025-09-24T19:07:11Z

@ghukill @ehanson8 I am requesting for another pass at review! As mentioned, I dropped the two commits that involved changes to the DynamoDB model. I've refactored the batch creation methods of Workflow.prepare_batch and Workflow._create_batch_in_db. While we discussed holding off on touching subclasses, I did find it helpful to go ahead and derive SimpleCSV.prepare_batch (it really wasn't a lot of code)!

Looking forward to getting your thoughts. :)

ghukill

Hearty approval! This is looking great. I know there is still wiring to be done for the CLI, and I'm betting implementing in all the workflows may put some pressure on it and potentially require little tweaks, but it feels like a good foundation for this batch creation.

And, potentially the removal of reconcile, broadly speaking.

dsc/workflows/base/__init__.py

dsc/workflows/base/simple_csv.py

ehanson8

Looking good, a few comments!

dsc/workflows/base/__init__.py

dsc/workflows/base/simple_csv.py

tests/conftest.py

ehanson8

Good changes, let's just remove the _insufficient_item_metadata method and then I'll approve!

dsc/workflows/base/simple_csv.py

ehanson8

Excellent work!

Why these changes are being introduced: * Support workflows that require programmatic batch creation. It was decided that the batch creation step would assume the responsibilities that were previously assigned to the reconcile step, which is now up for deprecation. How this addresses that need: * Define shared private method for writing records to DynamoDB * Add abstract method custom batch preparation processes * Define SimpleCSV.prepare_batch * Add unit tests for batch creation methods * Remove record creation from Workflow.reconcile_items Side effects of this change: * Sets the stage for deprecation of 'reconcile_items' Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-1455

jonavellecuerdo force-pushed the IN-1455-create-batch-for-base-workflow branch from adf4ae1 to f2d9e07 Compare September 16, 2025 17:29

jonavellecuerdo commented Sep 16, 2025

View reviewed changes

jonavellecuerdo requested review from ehanson8 and ghukill September 16, 2025 17:47

jonavellecuerdo marked this pull request as ready for review September 16, 2025 17:47

jonavellecuerdo requested a review from a team as a code owner September 16, 2025 17:47

ghukill reviewed Sep 17, 2025

View reviewed changes

ehanson8 reviewed Sep 18, 2025

View reviewed changes

jonavellecuerdo added a commit that referenced this pull request Sep 19, 2025

Address comments in PR #200

8764d56

jonavellecuerdo force-pushed the IN-1455-create-batch-for-base-workflow branch from 9adb187 to 7f5a824 Compare September 19, 2025 17:59

jonavellecuerdo requested review from ehanson8 and ghukill September 19, 2025 18:02

jonavellecuerdo force-pushed the IN-1455-create-batch-for-base-workflow branch from 7f5a824 to eb1c57c Compare September 24, 2025 19:04

ghukill approved these changes Sep 24, 2025

View reviewed changes

dsc/workflows/base/__init__.py Show resolved Hide resolved

dsc/workflows/base/__init__.py Show resolved Hide resolved

dsc/workflows/base/simple_csv.py Outdated Show resolved Hide resolved

ehanson8 reviewed Sep 24, 2025

View reviewed changes

dsc/workflows/base/__init__.py Show resolved Hide resolved

dsc/workflows/base/simple_csv.py Outdated Show resolved Hide resolved

dsc/workflows/base/simple_csv.py Outdated Show resolved Hide resolved

tests/conftest.py Show resolved Hide resolved

jonavellecuerdo added a commit that referenced this pull request Sep 25, 2025

Address comments in PR #200

a933189

jonavellecuerdo requested review from ehanson8 and ghukill September 25, 2025 16:13

ehanson8 reviewed Sep 25, 2025

View reviewed changes

dsc/workflows/base/simple_csv.py Outdated Show resolved Hide resolved

jonavellecuerdo added a commit that referenced this pull request Sep 25, 2025

Address comments in PR #200

8a05e5c

jonavellecuerdo requested a review from ehanson8 September 25, 2025 18:16

ehanson8 approved these changes Sep 25, 2025

View reviewed changes

jonavellecuerdo force-pushed the IN-1455-create-batch-for-base-workflow branch from 8a05e5c to eafb01e Compare September 25, 2025 18:31

jonavellecuerdo merged commit 7ba3fce into main Sep 25, 2025
3 checks passed

jonavellecuerdo deleted the IN-1455-create-batch-for-base-workflow branch September 25, 2025 18:34

Add batch creation method to Workflow abstract base class #200

Add batch creation method to Workflow abstract base class #200

Uh oh!

Conversation

jonavellecuerdo commented Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose and background context

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

Includes new or updated dependencies?

Changes expectations for external applications?

What are the relevant tickets?

Uh oh!

coveralls commented Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 18017142515

Details

💛 - Coveralls

Uh oh!

jonavellecuerdo Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghukill Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

ehanson8 Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

ghukill left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ghukill Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ehanson8 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ehanson8 Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

jonavellecuerdo commented Sep 19, 2025

Uh oh!

ghukill commented Sep 19, 2025

Uh oh!

ehanson8 commented Sep 19, 2025

Uh oh!

ghukill commented Sep 19, 2025

Uh oh!

ehanson8 commented Sep 19, 2025

Uh oh!

jonavellecuerdo commented Sep 24, 2025

Uh oh!

ghukill left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ehanson8 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ehanson8 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ehanson8 left a comment

Choose a reason for hiding this comment

Uh oh!

jonavellecuerdo commented Sep 16, 2025 •

edited

Loading

coveralls commented Sep 16, 2025 •

edited

Loading

jonavellecuerdo Sep 16, 2025 •

edited

Loading