Prerequisites for Digitized Theses workflow

[jonavellecuerdo]

Purpose and background context

This PR comprises of a series of updates to the DSC app in preparation for the Digitized Theses workflow. At high level, these updates are:

• Updating Workflow.prepare_batch to return ItemSubmission objects
  • This change was proposed in initial DSC refactoring discussions
  • Prior to this change, this method was returning attributes for an ItemSubmission object
• Create a util function for invoking sync AWS CLI command
  • Called by sync DSC CLI command
  • To be called by digitized-theses workflow during batch creation when syncing batch folder from digitized-theses-workspace to a minted batch folder in dsc-upload in S3.
• Rework archive_file_with_new_key as general method for moving files in S3
  • This method is not currently used by any implemented workflows (pulled from Wiley)
  • This method will be used by digitized-theses workflow to move files into replacement-theses/ and new_theses/ subfolders (prefixes)
• Make collection handle an optional argument for submit CLI command

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

Full DSC OpenCourseWare workflow run:

1. create (step function execution, CloudWatch log stream)
2. submit without collection-handle (step function execution, CloudWatch log stream)
3. submit with collection-handle (step function execution, CloudWatch log stream)
4. ingest with DSS (step function execution, CloudWatch log stream)
5. finalize (step function execution, CloudWatch log stream)

See ingested item at: https://mit-dspace.eks.prod.4science.cloud/handle/1721.1/164758

Includes new or updated dependencies?

YES

Changes expectations for external applications?

NO

What are the relevant tickets?

• https://mitlibraries.atlassian.net/browse/IN-1626
• https://mitlibraries.atlassian.net/browse/IN-1688

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.

[ehanson8]

Excellent work! A few questions

[ghukill]

These are some thoughtful and targeted changes, nice work!

Please see my comments about considering a new private _submit_item() (singular), how this exposes collection_handle as kind of awkward, and a possible doubling down on the getting of collection handles as a first class action that might deserve a method that is always called, not just when collection_handle = None.

[ghukill]

Commenting here, but not a formal request for this PR.

I think this method might benefit from leaning on another like _submit_item(...). Then, this could be simplified as a for loop that orchestrates, but the private _submit_item(...) method has most of the business logic.

You'd end up with something more like this, with the try only attemping one statement:

for item_submission in ItemSubmission.get_batch(self.batch_id):
    self.submission_summary["total"] += 1

    if not item_submission.ready_to_submit():
        self.submission_summary["skipped"] += 1
        continue

    try:
        items.append(
            self._submit_item(item_submission, collection_handle, batch_metadata)
        )

    except NotImplementedError:
        raise

    except Exception as exception:  # noqa: BLE001
        self.submission_summary["errors"] += 1
        item_submission.status = ItemSubmissionStatus.SUBMIT_FAILED
        item_submission.status_details = str(exception)
        item_submission.submit_attempts += 1
        item_submission.upsert_db()

What I feel like that refactor work does is expose how collection_handle and batch_metadata continue to feel kind of awkward.

It would appear that for any given invocation of submit_items() they are shared across all items, so why couldn't they be saved to the workflow instance? or, attached to the ItemSubmission instances that are getting passed around?

Feel free to ignore this for now, but I may reference it for the collection handle resolution if not provided in another comment.

[ghukill]

I'm circling back to this now given my continued reading and comment down below.

I get how this works, but am finding there is some cognitive dissonance here.

Is it correct that for most workflows, collection_handle is static / fixed / same for all items, and that's how it gets passed to submit_items()? But for some -- e.g. digi theses -- it is not, and can theoretically vary per item?

If so, I think I'd double down and always use a dedicated method for getting the collection handle. And, make all workflows implement it. Maybe 90% of them just return a single, hardcoded, static string, but then there is parity on how a single item gets its collection handle: it calls get_collection_handle(<ItemSubmission>) or something.

This still leaves the awkward batch_metadata in my comment above about refactoring to a private _submit_item(), but it knocks out the collection_handle passing weirdness!

[jonavellecuerdo]

To recap, the collection_handle is passed into DSC ad follows:

• Users can provide a collection_handle in the input payload for submit command when invoking the Step Function.
• The collection_handle is set for the DSC submit CLI command.
• The collection_handle is passed to Workflow.submit_items() call.
• The collection_handle is set on the ItemSubmission object.

---

In my initial passes at defining this method, I was planning for the following signature: _get_item_collection_handle(item_metadata: dict), rather than accepting an ItemSubmission instance (for context, item metadata is not saved to the ItemSubmission object and is thus not present when the object is loaded from DynamoDB). Thus, item metadata has to come from somewhere, and currently it is from this line (which is less than ideal).

Continuing the assumption that item metadata is not saved to the ItemSubmission object, if we were to define the method for all workflows, a workflow like opencourseware or sccs would look like this:

class Workflow:
   def submit_items(self, collection_handle: str):
      get_item_collection_handle(collection_handle)

   # where
   def get_item_collection_handle(collection_handle: str): 
      return collection_handle

The sample code above feels redundant/somewhat of an anti-pattern. 🤔 For this reason, I proposed:

class Workflow:
   def submit_items(self, collection_handle: str | None = None): 
      item_submission.collection_handle = (
                    collection_handle or self._get_item_collection_handle(<item metadata accessed from somewhere>)
                )

---

I hope this provides some context into the updates proposed here! Lastly, I selected the name _get_item_collection_handle in hopes it would imply how, if the method is called, this collection handle is at the item-level and not intended to be used across all the item submissions for a given batch.

Let me know what you think about keeping the derivation as is. Fully acknowledging that we still should tackle how we access item metadata in a follow-up PR!

Tagging @ehanson8 to request additional feedback and thoughts, too.

[ghukill]

@jonavellecuerdo - you're definitely much closer to the codebase, so I say go for it if it's feeling right still. I can always dig back in post PR and, if something still feels off, propose a small new PR at that time. Thanks for hearing me out!

[jonavellecuerdo]

I will merge as-is for now but continue to ponder in next PR!

[coveralls]

Coverage Report for CI Build 24908887711

Warning

No base build found for commit cf71ca5 on main.
Coverage changes can't be calculated without a base build.
If a base build is processing, this comment will update automatically when it completes.

Coverage: 96.282%

Details

• Patch coverage: 5 uncovered changes across 1 file (41 of 46 lines covered, 89.13%).

Uncovered Changes

File	Changed	Covered	%
dsc/utils/aws/s3.py	32	27	84.38%

Coverage Regressions

Requires a base build to compare against. How to fix this →

---

Coverage Stats

Relevant Lines:	1049
Covered Lines:	1010
Line Coverage:	96.28%
Coverage Strength:	0.96 hits per line

---

💛 - Coveralls