Implement Wiley workflow 'create' step - #263
Conversation
Why these changes are being introduced: * The Wiley workflow requires a batch creation process that must be capable of performing the following functions: 1. Accept a CSV file of DOIs as input 2. Can download bitstreams (PDFs) from Wiley via API 3. Can download metadata from Crossref 4. Can determine whether a DOI was seen from a previous batch run for Wiley How this addresses that need: * Add required env vars for Wiley workflow * Add ItemSubmission method for retrieving all submissions for a given workflow * Add download method to S3 client * Create Wiley workflow module Side effects of this change: * This requires the addition of two new env vars required by the Wiley workflow: - WILEY_METADATA_API_URL - WILEY_BITSTREAM_API_URL Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-1097
There was a problem hiding this comment.
🟡 Changes recommended
Incomplete sync handling, capped historical lookup, and unchecked operational failures can produce missing or duplicate submissions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Implements the Wiley batch-creation workflow for DOI-based submissions.
Changes:
- Downloads Wiley PDFs and Crossref metadata from CSV input.
- Adds workflow-wide submission lookup and S3 download support.
- Registers Wiley configuration, tests, and dependencies.
File summaries
| File | Description |
|---|---|
dsc/workflows/wiley/workflow.py |
Implements Wiley batch creation. |
dsc/workflows/wiley/transformer.py |
Adds transformer placeholder. |
dsc/workflows/wiley/__init__.py |
Exports Wiley components. |
dsc/workflows/__init__.py |
Registers the workflow. |
dsc/item_submission.py |
Adds workflow submission lookup. |
dsc/utils/aws/s3.py |
Adds S3 file downloads. |
dsc/utils/aws/__init__.py |
Exports AWS utilities. |
dsc/config.py |
Adds Wiley settings and changes logging setup. |
pyproject.toml |
Updates dependency constraints. |
tests/workflows/wiley/test_workflow.py |
Tests Wiley helper behavior. |
tests/workflows/wiley/__init__.py |
Initializes the test package. |
tests/test_item_submission.py |
Updates creation expectations. |
Review details
- Files reviewed: 12/13 changed files
- Comments generated: 10
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ghukill
left a comment
There was a problem hiding this comment.
Requesting changes, primarily for a docstring request for the workflow class. There are additional suggestions, but those didn't trigger the requesting of changes.
Nice work! All said, pretty easy to reason about. I'm unsure how much my prior knowledge helped... but the code is easy to follow.
| # get list of DOIs for completed item submissions | ||
| skip_list = self._get_completed_item_submission_ids() | ||
| logger.info(f"There are {len(skip_list)} completed Wiley item submissions") |
There was a problem hiding this comment.
Excellent idea to retrieve all at once, and then reuse. Quite the performance gain it sounds like!
There was a problem hiding this comment.
Indeed...Do you think it will continue to be performant, say we get to retrieving 10K item identifier values from the DynamoDB table? 😅
There was a problem hiding this comment.
I do. Seems much better to download ~10k identifiers (assuming we can limit to this workflow only, or at least scan to it and only retrieve that much data) than make ~10k requests to Dynamo!
| # copy csv of DOIs into temp batch folder | ||
| s3_client = S3Client() | ||
| s3_client.download_file( | ||
| s3_uri=f"s3://{CONFIG.s3_bucket_submission_assets}/{original_batch_path}MIT_Authored_Articles_Wiley.csv", |
There was a problem hiding this comment.
Is MIT_Authored_Articles_Wiley.csv the naming convention "that the DSC app expects"? Is this what you mentioned the other day about hardcoding, but perhaps we'd revisit?
There was a problem hiding this comment.
[nods heavily] Yes! When I look at the files in the v1 Wiley workflow S3 bucket, I didn't see any consistency in the filenames. I'm hoping we can request a format that they can agree to!
| } | ||
|
|
||
|
|
||
| class Wiley(Workflow): |
There was a problem hiding this comment.
Perhaps I'll change my tune, but as I kind of dig into this prepare_batch() PR, I'm finding that it could be really helpful if the workflow docstring provided a high level overview of how this workflow works.
It doesn't need to be exhaustive, but maybe touch on things like:
- this workflow pulls from 3rd party APIs
- how the batch + batch assets are created; my nascent understanding is that it's created by the workflow, pulling a file from S3 that it's expecting to be there... matched on
YYYY-MMin the prefix? - etc.
Forgive me if I'm missing this somewhere, but it feels like in code, not just confluence, we should explain how this workflow works.
There was a problem hiding this comment.
Agreed, I think you're part way there with some of the details in the commit message
There was a problem hiding this comment.
Ahh, I thought I hit all the boxes re: docstrings! I know you've made this comment before--will make sure to add one.
| raise NotImplementedError | ||
|
|
||
| def prepare_batch(self, *, synced: bool = False) -> tuple[list, ...]: # noqa: ARG002 | ||
| """Prepare a batch folder in the DSC S3 bucket. |
There was a problem hiding this comment.
I think it'd be handy if the docstring explained that a) we use thread parallelization to query the Wiley API, and b) why it's needed.
| try: | ||
| self._download_bitstream( | ||
| item_identifier=item_submission.item_identifier, | ||
| output_dir=output_dir, | ||
| ) | ||
| self._get_crossref_metadata( | ||
| item_identifier=item_submission.item_identifier, | ||
| output_dir=output_dir, | ||
| ) | ||
| except ( | ||
| exceptions.ItemBitstreamsNotFoundError, | ||
| exceptions.ItemMetadataNotFoundError, | ||
| ) as exception: | ||
| item_submission.status = ItemSubmissionStatus.CREATE_FAILED | ||
| item_submission.status_details = str(exception) | ||
| else: | ||
| item_submission.status = ItemSubmissionStatus.CREATE_SUCCESS |
There was a problem hiding this comment.
Any reason why bitstream + metadata are grouped under the same try? Is it to allow the fallthrough else statement?
Even if so, given those are pretty major operations, it feelsl like they would warrant their own try/except block.
An option could be pretty boring and explicit, with an early return if an error is had:
try:
self._download_bitstream(
item_identifier=item_submission.item_identifier,
output_dir=output_dir,
)
except exceptions.ItemBitstreamsNotFoundError as exception:
item_submission.status = ItemSubmissionStatus.CREATE_FAILED
item_submission.status_details = f"Bitstream download failed: {exception}"
return item_submission
try:
self._get_crossref_metadata(
item_identifier=item_submission.item_identifier,
output_dir=output_dir,
)
except exceptions.ItemMetadataNotFoundError as exception:
item_submission.status = ItemSubmissionStatus.CREATE_FAILED
item_submission.status_details = f"Crossref metadata fetch failed: {exception}"
return item_submission
item_submission.status = ItemSubmissionStatus.CREATE_SUCCESSThis would change the behavior your mention of a successful PDF download, but not metadata, where the PDF would enter the batch but would just be ignored. If that's an improvement, great! If a regression, than this form wouldn't work.
All said, totally optional.
| logger.exception(f"Failed to retrieve content from {url}") | ||
| raise exceptions.ItemBitstreamsNotFoundError from exception | ||
|
|
||
| content_type = response.headers.get("content-type", "") |
There was a problem hiding this comment.
Empty strings always make me very nervous.
What about:
content_type = response.headers.get("content-type")
if not content_type or not content_type.startswith("application/pdf"):
logger.error(
f"Expected PDF but retrieved {content_type or 'no content type'} instead"
)
raise exceptions.ItemBitstreamsNotFoundError| filepath = ( | ||
| Path(output_dir) | ||
| / item_identifier.replace("/", "-") | ||
| / f"{item_identifier.replace('/', '-')}.pdf" | ||
| ) |
There was a problem hiding this comment.
Maybe normalize the identifier once, then reuse?
normalized_item_identifier = item_identifier.replace("/", "-")
filepath = (
Path(output_dir)
/ normalized_item_identifier
/ f"{normalized_item_identifier}.pdf"
)| logger.info(f"Created batch folder in temporary directory: {tmp_dir.name}") | ||
| return str(tmp_batch_path) | ||
|
|
||
| def _download_bitstream(self, item_identifier: str, output_dir: str) -> None: |
There was a problem hiding this comment.
I realize it's not explicitly used (at least I don't think it is), but what if this method returned the filepath string of where it wrote the file? That can be handy for testing, logging, or even invoking directly. Totally optional.
There was a problem hiding this comment.
Good call and I would rename this method, bitstream is a DSpace term so I would assume this is downloading from DSpace not the Wiley server, maybe _get_file_from_wiley_server or _get_pdf_from_wiley_server?
| file.write(response.content) | ||
| logger.info(f"Saved PDF to {file.name}") | ||
|
|
||
| def _get_crossref_metadata(self, item_identifier: str, output_dir: str) -> None: |
There was a problem hiding this comment.
See comments above for Wiley PDF downloading! Very similar method. Whatever is applied above, recommended to copy that for this method.
There was a problem hiding this comment.
Agreed on harmonizing these methods
| } | ||
|
|
||
|
|
||
| class Wiley(Workflow): |
There was a problem hiding this comment.
Agreed, I think you're part way there with some of the details in the commit message
| # create temporary directory | ||
| tmp_batch_path = self._create_tmp_batch_dir() | ||
|
|
||
| # copy csv of DOIs into temp batch folder |
There was a problem hiding this comment.
Why do we need to download these to a temp folder? I think they could be read in memory and then you wouldn't need the new download_file method
| def _create_tmp_batch_dir(self) -> str: | ||
| """Create temporary directory for batch preparation.""" | ||
| tmp_dir = tempfile.TemporaryDirectory(delete=False) | ||
| tmp_batch_path = Path(tmp_dir.name) / self.batch_id | ||
| os.makedirs(tmp_batch_path) | ||
| logger.info(f"Created batch folder in temporary directory: {tmp_dir.name}") | ||
| return str(tmp_batch_path) |
There was a problem hiding this comment.
This could also be removed if you read the CSVs in memory
| logger.info(f"Created batch folder in temporary directory: {tmp_dir.name}") | ||
| return str(tmp_batch_path) | ||
|
|
||
| def _download_bitstream(self, item_identifier: str, output_dir: str) -> None: |
There was a problem hiding this comment.
Good call and I would rename this method, bitstream is a DSpace term so I would assume this is downloading from DSpace not the Wiley server, maybe _get_file_from_wiley_server or _get_pdf_from_wiley_server?
| filepath = ( | ||
| Path(output_dir) | ||
| / item_identifier.replace("/", "-") | ||
| / f"{item_identifier.replace('/', '-')}.pdf" | ||
| ) |
| file.write(response.content) | ||
| logger.info(f"Saved PDF to {file.name}") | ||
|
|
||
| def _get_crossref_metadata(self, item_identifier: str, output_dir: str) -> None: |
There was a problem hiding this comment.
Agreed on harmonizing these methods
| filepath = ( | ||
| Path(output_dir) | ||
| / item_identifier.replace("/", "-") | ||
| / f"{item_identifier.replace('/', '-')}.json" | ||
| ) |
There was a problem hiding this comment.
Echoing @ghukill 's recommendation for reusing the normalized identifier
Purpose and background context
The Wiley workflow requires a batch creation process that must be capable of performing the following functions:
How this addresses that need:
Highlights
Though they share similarities, I would propose any work to create a mutual parent for these two workflows for sometime later!
status="ingest_success"was performed per item submission, but that resulted in the batch creation process taking 40+ mins to run. For this reason, I opted to retrieve the item identifiers from records in DynamoDB and store them in a list before proceeding to prepare each item submission. With this change, the batch creation process now takes ~3 mins to complete!✨ Note: 693 of the 1K+ lines changed are
pyproject.tomlanduv.lockrelated.How can a reviewer manually see the effects of these changes?
Review added unit tests for Wiley workflow
Ran the
createstep using the DSO step function forbatch_id=2022-10-wiley-am. The CSV file I uploaded represents the most recent CSV file in thewiley-devS3 bucket (s3://wiley-files-dev-222053980223/archived/MIT_Automatic_Article_List_10.10.2022.csv). I renamed the file to one that the DSC app expects.From logstream:
Wiley.prepare_batch. Planning to create a follow-up ticket to print summaries during thecreatestep across all workflows, which will allow us to reorganize when this log appears!This means, 1053 of the records from the most recent file we received from Wiley were previously processed via the v1 workflow with https://github.com/MITLibraries/wiley-deposits/blob/main/. I would take the numbers above with a grain of salt and a closer look at these counts will be more valuable once we get our first upload from Wiley! The main point of sharing these numbers is to show that the DSC workflow can skip items previously ingested/completed.
Includes new or updated dependencies?
YES
Changes expectations for external applications?
YES - Implementing the DSC workflow for Wiley will allow us to sunset the standalone https://github.com/MITLibraries/wiley-deposits app.
What are the relevant tickets?
Code review