Revise compile dataset after acceptance testing

src/corppa/utils/dataset_prep.py

@@ -891,6 +891,7 @@ def process_gale_work(
             page_num = int(page["id"].rsplit(".", 1)[-1])
             image_path = vol_img_dir / get_gale_image_name(vol_id, page_num)
             if image_path.is_file():
+                # use work id for output path, so excerpts get their own folder in the tar file
                 tar_image_path = f"{work_id}/{image_path.name}"
                 tar.add(image_path, arcname=tar_image_path)
                 # add the tar file name and image path to the page data
@@ -928,7 +929,6 @@ def open_ht_zipfile(zipfile_path: Optional[Path]) -> Iterator[Optional[ZipFile]]
 def process_ht_work(
     work_id: str, pages: list[dict], image_dir: Path, tar: tarfile.TarFile
 ) -> Iterator[dict]:
-    htid = get_volume_id(work_id)
     # a work is an excerpt if its work_id includes with -p; excerpts are not expected to use all pages from the zip file
     is_excerpt = "-p" in work_id
     zipfile_path = get_ht_zipfile_path(work_id, image_dir)
@@ -953,7 +953,8 @@ def process_ht_work(
 
         # when image mapping was returned, add images to tar file and image paths to page data
         zip_image_filenames = get_zip_image_names(ht_zip)
-        encoded_htid = encode_htid(htid)
+        # use work id rather than htid so that each excerpt gets its own folder in the tar file
+        encoded_workid = encode_htid(work_id)
         for page in pages:
             page_id = page["id"]
             # get the corresponding image from the zip, add to the tar file with appropriate name,
@@ -978,7 +979,7 @@ def process_ht_work(
                 # the appropriate path for this page in the tarfile
                 if zip_image_path is not None:
                     img_ext = Path(zip_image_path).suffix
-                    tar_image_path = f"{encoded_htid}/{page_id}{img_ext}"
+                    tar_image_path = f"{encoded_workid}/{page_id}{img_ext}"
                     add_zip_file_to_tar(ht_zip, zip_image_path, tar, tar_image_path)
                     page["image_file"] = tar_filename
                     page["image_path"] = tar_image_path
@@ -1007,7 +1008,7 @@ def process_ht_work(
                 zip_image_path = Path(zip_image_name)
                 img_ext = zip_image_path.suffix
                 basename = zip_image_path.stem
-                tar_image_path = f"{encoded_htid}/unmatched/{basename}{img_ext}"
+                tar_image_path = f"{encoded_workid}/unmatched/{basename}{img_ext}"
                 add_zip_file_to_tar(ht_zip, zip_image_name, tar, tar_image_path)
 
             logger.info(
@@ -1136,8 +1137,8 @@ def process_ht1930_work(
             zip_image_path = image_map.get(page_order)
             if zip_image_path is not None:
                 img_ext = Path(zip_image_path).suffix
-                # set destination name based on volume and page id, but preserve existing extension
-                tar_image_path = f"{encode_htid(htid)}/{page_id}{img_ext}"
+                # set destination name based on work id and page id, but preserve existing image extension
+                tar_image_path = f"{encode_htid(work_id)}/{page_id}{img_ext}"
                 try:
                     add_zip_file_to_tar(ht_zip, zip_image_path, tar, tar_image_path)
                     # if adding succeeded, include the tar file name and image path in the output page data
@@ -1440,25 +1441,8 @@ def main():
             disable=not args.progress,
         ):
             work_id = page["work_id"]
-            current_chunk_id = get_tarfile_group(work_id)
-            # chunk id of None is expected for eebo-tcp work ids, since they have no page images
-            if current_chunk_id is not None and current_chunk_id != tarfile_chunk_id:
-                if tar is not None:
-                    # close previous tar file if one was open
-                    tar.close()
-                # use the chunk id to determine filename; used for output and page image filename in jsonl
-                img_output_path = args.output_dir / f"ppa_images_{current_chunk_id}.tar"
-                # open the new tar file
-                # append to the tar if it exists and continue was requested, otherwise overwiret
-                tar_mode = (
-                    "a" if args.continue_run and img_output_path.exists() else "w"
-                )
-                logger.info("%s %s", tar_mode_verb[tar_mode], img_output_path)
-                tar = tarfile.open(img_output_path, tar_mode)
-                # update active chunk id
-                tarfile_chunk_id = current_chunk_id
-
             # when work id changes, process the previous work pages and reset for the next
+            # NOTE: must process previous work before closing out tar file when work id and tar chunk changes
             if work_id != prev_work_id:
                 if prev_work_id is not None:
                     if skip_work:
@@ -1490,6 +1474,24 @@ def main():
             else:
                 pages.append(page)
 
+            current_chunk_id = get_tarfile_group(work_id)
+            # chunk id of None is expected for eebo-tcp work ids, since they have no page images
+            if current_chunk_id is not None and current_chunk_id != tarfile_chunk_id:
+                if tar is not None:
+                    # close previous tar file if one was open
+                    tar.close()
+                # use the chunk id to determine filename; used for output and page image filename in jsonl
+                img_output_path = args.output_dir / f"ppa_images_{current_chunk_id}.tar"
+                # open the new tar file
+                # append to the tar if it exists and continue was requested, otherwise overwiret
+                tar_mode = (
+                    "a" if args.continue_run and img_output_path.exists() else "w"
+                )
+                logger.info("%s %s", tar_mode_verb[tar_mode], img_output_path)
+                tar = tarfile.open(img_output_path, tar_mode)
+                # update active chunk id
+                tarfile_chunk_id = current_chunk_id
+
         # handle the pages for the last work at end of loop, unless we broke
         # out early on a stop signal (that work was already written before
         # the break)

src/corppa/utils/dataset_refine.py

@@ -115,6 +115,13 @@ def refine_metadata(
             )
             .drop("author_refined", "pub_place_refined")
         )
+    # clean whitespace for all author, pub_place, and publishers, regardless of refinement,
+    # since unrefined values may have trailing whitespace
+    refined_df = refined_df.with_columns(
+        author=pl.col.author.str.strip_chars(),
+        pub_place=pl.col.pub_place.str.strip_chars(),
+        publisher=pl.col.publisher.str.strip_chars(),
+    )
 
     # replace refined fields with unrefined and save the file
     assert refined_df.height == csv_total  # no records lost

tests/test_utils/test_dataset_prep.py

@@ -339,7 +339,8 @@ def test_process_gale_work_adds_image_path_when_image_present(tmp_path):
     from corppa.utils.path_utils import get_gale_image_name, get_vol_dir
 
     vol_id = "CB0127060085"
-    pages = [{"work_id": vol_id, "id": f"{vol_id}.0001", "text": "one"}]
+    work_id = f"{vol_id}-p42"
+    pages = [{"work_id": work_id, "id": f"{work_id}.0001", "text": "one"}]
     image_dir = tmp_path / "images"
     vol_img_dir = image_dir / get_vol_dir(vol_id)
     vol_img_dir.mkdir(parents=True)
@@ -348,11 +349,11 @@ def test_process_gale_work_adds_image_path_when_image_present(tmp_path):
     (vol_img_dir / img_name).write_bytes(b"fake image data")
 
     with tarfile.open(tmp_path / "out.tar", "w") as tar:
-        result = list(process_gale_work(vol_id, pages, image_dir, tar))
+        result = list(process_gale_work(work_id, pages, image_dir, tar))
 
     assert len(result) == 1
     assert result[0]["image_file"] == "out.tar"
-    assert result[0]["image_path"] == f"{vol_id}/{img_name}"
+    assert result[0]["image_path"] == f"{work_id}/{img_name}"
 
 
 def test_process_gale_work_missing_image_file_omits_path(tmp_path):
@@ -426,7 +427,9 @@ def test_process_ht_work_no_zip_yields_pages_unchanged(tmp_path):
 
 def test_process_ht_work_aligned_pages_get_image_paths(tmp_path):
     htid_suffix = "12345678"
-    work_id = f"test.{htid_suffix}"
+    work_id = (
+        f"test.{htid_suffix}-p5"  # test that excerpt is used rather than base work id
+    )
     _make_ht_zip(tmp_path, htid_suffix, PAGE_TEXTS)
     pages = [
         {"work_id": work_id, "id": f"{work_id}.{pid}", "text": text}
@@ -436,7 +439,12 @@ def test_process_ht_work_aligned_pages_get_image_paths(tmp_path):
         result = list(process_ht_work(work_id, pages, tmp_path, tar))
     # all pages returned, each with an image path in the tar
     assert len(result) == len(pages)
+    for p in result:
+        print(p["id"], p.get("image_path"))
     assert all("image_path" in p for p in result)
+    # image path in the tar file should be based ok
+    for p in result:
+        assert p["image_path"].startswith(f"{encode_htid(work_id)}/")
 
 
 def test_process_ht_work_aligned_pages_new_ocr(tmp_path):
@@ -860,7 +868,7 @@ def test_process_ht1930_no_zip_yields_pages_unchanged(tmp_path, caplog):
 
 
 def test_process_ht1930_maps_images_by_order(tmp_path):
-    work_id = "test.12345678"
+    work_id = "test.12345678-p13"  # test excerpt id is used rather than base work id
     # full-work zip names are "{htid-dashes}-{HT id}.zip" (trailing number is an
     # unrelated HT-assigned id), matched by a wildcard on the htid prefix
     _make_ht1930_zip(tmp_path, "test-12345678-1788450816.zip", [1, 2, 3])
@@ -2157,6 +2165,31 @@ def test_main_continue_skips_completed_last_work(corpus_input, main_dirs, caplog
     )
 
 
+def test_main_last_work_in_chunk_uses_its_own_tar(tmp_path, main_dirs):
+    # when the work id crosses a tar chunk boundary, the previous (last) work
+    # in the old chunk must be processed with the old chunk's tar, not the
+    # tar opened for the next chunk
+    corpus_dir = _make_corpus_dir(
+        tmp_path / "corpus",
+        [
+            {"work_id": "abc.1", "id": "abc.1.0001", "text": "a1"},  # a-k chunk
+            {"work_id": "work.B", "id": "workB.0001", "text": "b1"},  # o-y chunk
+        ],
+    )
+    tar_used = {}
+
+    def record_tar(work_id, pages, image_dir, tar, ht1930_work_ids=None):
+        tar_used[work_id] = Path(tar.name).name
+        yield from pages
+
+    _run_main(corpus_dir, main_dirs, process_side_effect=record_tar)
+
+    assert tar_used == {
+        "abc.1": "ppa_images_a-k.tar",
+        "work.B": "ppa_images_o-y.tar",
+    }
+
+
 def test_main_continue_does_not_rename_existing_output(corpus_input, main_dirs):
     _, output_dir = main_dirs
     output_dir.mkdir()