IN-1672-base-metadata-transformer - #268
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new submission path rejects previously valid SCCS metadata, and Wiley’s inherited transformer stub returns None.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Introduces a shared metadata-transformer framework and moves transformation into the base submission workflow.
Changes:
- Adds transformer protocol and reusable field-based transformer.
- Migrates SCCS, OCW, Wiley, and Digitized Theses transformer declarations.
- Updates OCW metadata iteration and tests.
File summaries
| File | Description |
|---|---|
dsc/workflows/base/transformer.py |
Adds shared transformer abstractions. |
dsc/workflows/base/workflow.py |
Runs transformers during submission. |
dsc/workflows/base/__init__.py |
Exports transformer types. |
dsc/workflows/sccs/transformer.py |
Adds SCCS field transformation. |
dsc/workflows/sccs/workflow.py |
Configures the SCCS transformer. |
dsc/workflows/sccs/__init__.py |
Exports the SCCS transformer. |
dsc/workflows/opencourseware/transformer.py |
Implements the transformer protocol. |
dsc/workflows/opencourseware/workflow.py |
Yields untransformed source metadata. |
dsc/workflows/digitized_theses/transformer.py |
Implements the transformer protocol. |
dsc/workflows/wiley/transformer.py |
Adds protocol inheritance. |
dsc/workflows/archivesspace/workflow.py |
Explicitly retains mapping-based metadata. |
tests/test_workflow_opencourseware.py |
Updates OCW transformation expectations. |
Review details
- Files reviewed: 12/13 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| item_submission.prepare_dspace_metadata( | ||
| item_metadata=item_metadata, | ||
| s3_bucket=self.s3_bucket, | ||
| batch_path=self.batch_path, | ||
| ) |
There was a problem hiding this comment.
This is fine, it looks like the metadata example is being misinterpreted as a requirement
There was a problem hiding this comment.
Side note: I also think that those two fields are the minimum required metadata fields in the DSpace REST API contract, so I think it would raise an issue if dc.date.issued was missing from the record. 🤔
| from dsc.workflows.base import BaseTransformer | ||
|
|
||
|
|
||
| class SCCSTransformer(BaseTransformer): |
Why these changes are being introduced: * Create a MetadataTransformer and BaseTransformer class to align the metadata transformation of the various workflows How this addresses that need: * Add Protocol-based MetadataTransformer class as transformer parent class * Add BaseTransformer class to transform source metadata where the field names correspond to DSpace field names * Add metadata_transformer attribute to Workflow class, update submit_items method to account for metadata transformer path, and add _run_metadata_transformer method * Update type hinting for DigitizedThesesTransformer and OpenCourseWareTransformer fields attribute * Update OpenCourseWare.item_metadata_iter method to align with other workflows * Add SCCSTransformer class * Add metadata_transformer attribute to SCCS class * Update OCW unit test to use metadata transformer * Update dependencies Side effects of this change: * NA Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-1672
* Add stub transform method to WileyTransformer * Add unit tests for SCCSTransformer Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
2749717 to
223295b
Compare
| raise NotImplementedError( | ||
| f"'{self.workflow_name}' does not have a metadata_transformer." | ||
| ) |
There was a problem hiding this comment.
This will be removed once we deprecate the metadata mapping path
| else: | ||
| item_metadata = batch_metadata[item_identifier] | ||
| item_submission.prepare_dspace_metadata( | ||
| metadata_mapping=self.metadata_mapping, | ||
| item_metadata=item_metadata, | ||
| s3_bucket=self.s3_bucket, | ||
| batch_path=self.batch_path, |
There was a problem hiding this comment.
This will be removed once we deprecate the metadata mapping path
ghukill
left a comment
ghukill
left a comment
There was a problem hiding this comment.
I think the nature of comments are pretty circular, they all kind of reference one another. I'm opting to pause here to lodge these comments/questions, and it's quite possible an easy explanation addresses them all at once!
Though I suppose there is a legit request for a docstring in the base Workflow class.
All that said, really liking the shape of this! I like the class level metadata_transformer property, where workflows kind of "register" their transformer at definition time.
Looking good, just looking for some clarity re: my questions before I continue.
| workflow_name: str = "base" | ||
| submission_system: str = "IR-8" | ||
| required_env_vars: ClassVar[list] = [] | ||
| metadata_transformer: ClassVar[type[MetadataTransformer] | None] = None |
There was a problem hiding this comment.
This.... I like. But it took me awhile to get there. Let me explain my confusion, and perhaps it would suggest where a comment or docstring could help.
- I see it's
Nonehere at the class level, cool. - I look in
__init__()and don't see it set anywhere, so a bit confused - I look at archivespace's workflow and see it's explicitly setting None, a bit more confused because I thought that was default
- Do finally look at digitized_theses's workflow setting a transformer, as I just happen to know it uses one
I'm not immediately sure where a docstring could have helped me, but I think it'd be in the base workflow class. My proposal would be defining and explaining the class level properties in the base workflows's docstsring, e.g.:
class Workflow(ABC):
"""A base workflow class from which other workflow classes are derived.
Class properties:
- workflow_name: ...
- submission_system: ...
- ...
- ...
- metadata_transformer: <as lengthy an explanation as needed that it defaults to None, but workflows can define or reuse a pre-existing transformer and set it here>
"""|
|
||
| workflow_name: str = "archivesspace" | ||
| submission_system: str = "Dome" | ||
| metadata_transformer = None # uses metadata_mapping.json |
There was a problem hiding this comment.
I might propose omitting this metadata_transformer = None # uses metadata_mapping.json and just let it fall-through to the base workflow's default of None. Unless we have compelling reasons not to.
There was a problem hiding this comment.
I'm returning to this comment as I'm either more confused... or would like to rescind this suggestion 😅.
Why does this workflow, and only this workflow, not need to define a transformer class and thus doesn't register one on the workflow?
I'm a bit unclear how this archivesspace workflow differs from one like sccs which extends the BaseTransformer (but also see my note about that class naming) and registers its custom transformer.
It seems like archivesspace is not using a transformer at all?
| class BaseTransformer(MetadataTransformer): | ||
| """Base transformer for workflows where field names correspond to DSpace field names. | ||
|
|
||
| Subclasses declare which fields to include and which are pipe-delimited. | ||
| """ |
There was a problem hiding this comment.
After a bit of circling around, I think I understand my confusion here.
On my first read through, I thought BaseTransformer was what all child transformers would inherit from. I understand that's incorrect, and it's actually MetadataTransformer.
Before I understood that, I thought this class was somehow handling custom vs delimited mapped fields, and I couldn't see that anywhere in the logic.
What if this class were renamed to something like DelimitedMappingTransformer?
* Add stub ArchivesSpaceTransformer class and update the workflow to call it * Update metadata_mapping_path attributes to raise NotImplementedError for all workflows * Rename BaseTransformer > SimpleCSVTransformer and shift to simple_csv workflow folder
|
@ghukill Pushed a new commit based on our discussion! |
ghukill
left a comment
ghukill
left a comment
There was a problem hiding this comment.
Enthusiastic approve! Nice work, and thanks for the huddle last week. All my questions virtually vanish now.
I had one parting though that is not a request, and maybe not even something we want to do today, but came to mind.
It feels like simple_csv is sort of a "generic" workflow that needs extension + opinionation. SCCS comes to mind obviously, maybe there are others.
I wonder if there is value in docstring additions to both SimpleCSV(Workflow) and SimpleCSVTransformer(MetadataTransformer) that make pretty clear they are not designed to be used without extension?
I wouldn't be surprised if we have a test that treats it like a real workflow, but we know that we'd never call SimpleCSV -- workflow or transformer -- directly in a real ingest. I don't think we really need programattic business logic to enforce this, but something in the directory structure or docstrings that could hint to future us that these are meant to be extended could be helpful. The fact that it's a sibling directory to sccs, opencourseware, etc., makes it a bit confusing.
But again, optional and not need right now. This PR is approved as-is.
Let's discuss when @jonavellecuerdo gets back, good suggestions! |
|
Re: @ghukill 's last comment (#268 (review)) --
@ehanson8 If you update the docstring, I think everything else is looking good and happy to approve then! |
* Update SimpleCSVTransformer docstring to reflect that it needs to be extended
@jonavellecuerdo Updated! |
Purpose and background context
This PR creates a
Protocol-basedMetadataTransformerclass as a parent class for all transformers. This is needed given the differing inputs to thetransformmethods on each transformer class (e.g. XML string, JSON). Eventually all workflows will use a transformer class and the metadata mapping path will be removed in DSO-64.It also creates a
BaseTransformerclass that corresponds to thesccs/simple_csvworkflow.How can a reviewer manually see the effects of these changes?
Due to continued issues with our MIT Open Scholarship test instance, a Docker container was used for local DSpace testing. Tests were performed for several workflows to confirm they still produced metadata that could be ingested by DSpace. Some were not possible to test with reasons noted below:
SCCS
Create
Submit
Finalize
OpenCourseWare
Create
Submit
Finalize
Wiley
Only
createhas been implemented which doesn't use the transformer class but this test shows it still functions.Create
Digitized Theses
Can't be tested given that the workflow reaches out to the DSpace instance during the
createstepDSS logs for ingesting
SCCSandOpenCourseWarein the local DSpace 8 instanceIncludes new or updated dependencies?
YES
Changes expectations for external applications?
NO
What are the relevant tickets?
Code review