fix: bound discovery memory and route generator warnings through logging#113
Merged
tompollard merged 1 commit intoJul 1, 2026
Merged
Conversation
Two small robustness/consistency fixes in one change: - files.discover_files appended every hidden-directory path to an in-memory list used only for a debug log, so a dataset with a large hidden tree (a stray .git or .ipynb_checkpoints) grew the list without bound. Cap the retained examples at 5; the skipped count stays exact. - metadata_generator emitted its warnings via print() to stdout, while every file handler already logs through a module logger. Route the three warning-level diagnostics (failed extraction, failed build_croissant, multi-match field mapping) through a module logger instead -- off stdout and controllable, matching the handler convention. The DICOM skip summary stays a stdout print: it is a deliberate user-facing summary with its own test. Tests: cap assertion in test_files; logging assertion in test_generator_diagnostics; the existing field-mapping CLI test now asserts via caplog, like the handler-warning tests already do. Full suite (271) passes with no change to committed example outputs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
elementalcollision
marked this pull request as ready for review
June 30, 2026 17:35
rafiattrach
approved these changes
Jun 30, 2026
rafiattrach
left a comment
rafiattrach
left a comment
Collaborator
There was a problem hiding this comment.
Thanks! That seems reasonable!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft PR opened by Chimera (an autonomous code agent operated by @elementalcollision), as part of a review of croissant-baker. Opened as a draft for maintainer review.
Two small robustness/consistency fixes, bundled.
1. Bounded-memory file discovery
files.discover_filesappended every hidden-directory path toskipped_examples, a list used only to print a few examples in a debug log. On a dataset with a large hidden tree — an accidental.git, a big.ipynb_checkpoints— that list grows without bound for output we only ever sample. Cap retained examples at 5; the skipped count stays exact.2. Consistent observability (logging, not print)
Every file handler already logs through a module logger (
croissant_baker.handlers.*), butmetadata_generatoremitted its warnings viaprint()to stdout. This routes the three warning-level diagnostics — failed extraction, failedbuild_croissant, multi-match field mapping — through acroissant_baker.metadata_generatorlogger: off stdout, controllable, and consistent with the handlers.The DICOM skip summary deliberately stays a stdout
print— it's a user-facing summary line with its own dedicated test (test_dicom_skip_summary_printed_…), not a warning.Tests
test_files.py(20 hidden files → exact count, 5 examples).test_generator_diagnostics.py: the field-mapping warning is emitted on the logger.caplog— the same pattern the parquet/image/fhir handler-warning tests already use.tests/data/output/*.jsonld.Note on overlap
This touches the same
metadata_generator.py"Failed to process" warning line that #112 (parallel extraction) relocates. The two overlap by one line; whichever merges second needs a trivial rebase. The changes are otherwise independent.