Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/croissant_baker/files.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,12 @@

logger = logging.getLogger(__name__)

# Cap on example paths retained for the hidden-directory skip debug log. The
# skipped count is always exact; only this example list is bounded, so a dataset
# with a huge hidden tree (an accidental .git or .ipynb_checkpoints, say) cannot
# accumulate an unbounded list of paths we only ever sample from.
_MAX_SKIPPED_EXAMPLES = 5


def discover_files(
dir_path: str,
Expand Down Expand Up @@ -34,7 +40,7 @@ def discover_files(
raise FileNotFoundError(f"{dir_path} is not a directory")

skipped_count = 0
skipped_examples = []
skipped_examples: List[str] = []

files = []
for file in directory.rglob("*"):
Expand All @@ -45,7 +51,8 @@ def discover_files(

if any(part.startswith(".") for part in rel_path.parts):
skipped_count += 1
skipped_examples.append(str(rel_path))
if len(skipped_examples) < _MAX_SKIPPED_EXAMPLES:
skipped_examples.append(str(rel_path))
continue

files.append(rel_path)
Expand Down
18 changes: 12 additions & 6 deletions src/croissant_baker/metadata_generator.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""Croissant metadata generator for datasets."""

import json
import logging
import tempfile
from collections import defaultdict
from datetime import datetime
Expand All @@ -12,6 +13,8 @@
from croissant_baker.files import discover_files
from croissant_baker.handlers.registry import find_handler, register_all_handlers

logger = logging.getLogger(__name__)

# Register all handlers
register_all_handlers()

Expand Down Expand Up @@ -124,10 +127,13 @@ def visit(node: object) -> None:

for name, count in match_counts.items():
if count > 1:
print(
f"Warning: field mapping '{name}' applied to {count} fields. "
f"If '{name}' means different things in different RecordSets, "
"rename the columns or split the bake."
logger.warning(
"field mapping '%s' applied to %d fields. If '%s' means "
"different things in different RecordSets, rename the columns "
"or split the bake.",
name,
count,
name,
)


Expand Down Expand Up @@ -283,7 +289,7 @@ def generate_metadata(self, progress_callback=None) -> dict:
meta["relative_path"] = str(file_path)
file_metadata.append((handler, meta))
except Exception as e:
print(f"Warning: Failed to process {file_path}: {e}")
logger.warning("Failed to process %s: %s", file_path, e)
else:
ext = full_path.suffix.lower()
if ext in {".dcm", ".dicom"}:
Expand Down Expand Up @@ -382,7 +388,7 @@ def generate_metadata(self, progress_callback=None) -> dict:
distributions.extend(filesets)
record_sets.extend(rs)
except Exception as e:
print(f"Warning: {type(_h).__name__}.build_croissant failed: {e}")
logger.warning("%s.build_croissant failed: %s", type(_h).__name__, e)

_assert_unique_node_ids(distributions, record_sets)

Expand Down
42 changes: 24 additions & 18 deletions tests/test_cli.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""Tests for Croissant Baker CLI."""

import json
import logging
import re
from pathlib import Path

Expand Down Expand Up @@ -836,32 +837,37 @@ def test_usage_info_accepts_any_uri_scheme(
assert json.loads(output.read_text())["usageInfo"] == uri


def test_field_mapping_warns_on_multiple_matches(tmp_path: Path) -> None:
"""A mapping that hits more than one field warns the user."""
def test_field_mapping_warns_on_multiple_matches(
tmp_path: Path, caplog: pytest.LogCaptureFixture
) -> None:
"""A mapping that hits more than one field warns the user (via logging)."""
dataset_dir = tmp_path / "ds"
dataset_dir.mkdir()
# Two CSVs both with an 'id' column — same name, different semantics.
(dataset_dir / "patients.csv").write_text("id,age\n1,40\n2,50\n")
(dataset_dir / "diagnoses.csv").write_text("id,code\n1,X\n2,Y\n")

output = tmp_path / "out.jsonld"
result = runner.invoke(
app,
[
"--input",
str(dataset_dir),
"--output",
str(output),
"--creator",
"Test",
"--field-mapping",
"id=http://www.wikidata.org/entity/Q577",
"--no-validate",
],
)
with caplog.at_level(logging.WARNING, logger="croissant_baker.metadata_generator"):
result = runner.invoke(
app,
[
"--input",
str(dataset_dir),
"--output",
str(output),
"--creator",
"Test",
"--field-mapping",
"id=http://www.wikidata.org/entity/Q577",
"--no-validate",
],
)
assert result.exit_code == 0, result.output
combined = (result.output or "") + (result.stderr or "")
assert "field mapping 'id' applied to 2 fields" in combined
assert any(
"field mapping 'id' applied to 2 fields" in r.getMessage()
for r in caplog.records
), caplog.records


def test_field_mappings_yaml_rejects_unknown_keys(
Expand Down
24 changes: 24 additions & 0 deletions tests/test_files.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Tests for file discovery utilities."""

import logging
from pathlib import Path
import pytest
from croissant_baker.files import discover_files
Expand Down Expand Up @@ -53,6 +54,29 @@ def test_discover_files_skips_hidden_dirs(tmp_path: Path) -> None:
assert set(files) == expected


def test_discover_files_caps_skipped_examples(
tmp_path: Path, caplog: pytest.LogCaptureFixture
) -> None:
"""The skipped-file count is exact, but the example list is bounded.

Regression guard: previously every hidden-directory path was appended to an
in-memory list used only for a debug log, so a large hidden tree (e.g. a
stray .git) grew the list without bound.
"""
(tmp_path / "keep.txt").write_text("x")
(tmp_path / ".hidden").mkdir()
for i in range(20):
(tmp_path / ".hidden" / f"f{i}.txt").write_text("x")

with caplog.at_level(logging.DEBUG, logger="croissant_baker.files"):
files = discover_files(str(tmp_path))

assert set(files) == {Path("keep.txt")}
rec = next(r for r in caplog.records if "hidden directories" in r.getMessage())
assert rec.args[0] == 20 # exact skipped count preserved
assert len(rec.args[1]) == 5 # example list capped


def test_discover_files_include_patterns(tmp_path: Path) -> None:
"""Test discover_files only returns files matching include patterns."""
(tmp_path / "data").mkdir()
Expand Down
38 changes: 38 additions & 0 deletions tests/test_generator_diagnostics.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
"""MetadataGenerator diagnostics go through ``logging``, like the handlers do.

Warnings previously went to stdout via ``print()`` while every handler logged
through a module logger. These tests pin the consistent behaviour: warnings are
emitted on the ``croissant_baker.metadata_generator`` logger (controllable,
off-stdout) rather than printed.
"""

import logging

from croissant_baker.metadata_generator import _apply_field_mappings

_GEN_LOGGER = "croissant_baker.metadata_generator"


def test_field_mapping_multi_match_warns_via_logging(
caplog,
) -> None:
"""A field mapping that resolves to multiple fields logs a WARNING."""
metadata = {
"recordSet": [
{"field": [{"@type": "cr:Field", "name": "age"}]},
{"field": [{"@type": "cr:Field", "name": "age"}]},
]
}

with caplog.at_level(logging.WARNING, logger=_GEN_LOGGER):
_apply_field_mappings(metadata, {"age": {"equivalent_property": "wdt:P3629"}})

matching = [
r
for r in caplog.records
if r.name == _GEN_LOGGER
and "age" in r.getMessage()
and "2 fields" in r.getMessage()
]
assert matching, caplog.records
assert matching[0].levelno == logging.WARNING