Repository navigation
[refactor] refactor cli and internal api - #17
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The current DataIO.do_preview() stdout behavior and the import-time DEFAULT_UI env evaluation are likely to break deterministic CLI output and runtime behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refactors the CLI and internal I/O API by replacing the old OutputActions/inline PathIO implementation with a new DataIO + dedicated PathIO module, and updates CLI stdout snapshots accordingly.
Changes:
- Refactor CLI wiring to use
DataIOfor preview/save/upload and reorganize Typer help panels/options. - Extract filesystem/status logic into
src/prep/api/pathio.pyand media-writing helpers intosrc/prep/api/formatutils/media.py. - Update CLI snapshot tests for new save/upload messaging and remove the prior
tests/api/test_dataio.pycoverage.
File summaries
| File | Description |
|---|---|
| tests/cli/test_verl.py | Updates expected CLI stdout snapshot for new save/upload messaging. |
| tests/cli/test_show.py | Updates expected CLI stdout snapshot for new save/upload messaging. |
| tests/cli/test_sft.py | Updates expected CLI stdout snapshot for new save/upload messaging. |
| tests/cli/test_eval.py | Updates expected CLI stdout snapshot for new save/upload messaging. |
| tests/cli/test_cls.py | Updates expected CLI stdout snapshot for new save/upload messaging. |
| tests/api/test_dataio.py | Removes prior unit tests covering output/path I/O behavior. |
| src/prep/transform/convert.py | Adds an RGB conversion transform registered via @transform. |
| src/prep/constants.py | Adds DEFAULT_UI and changes SKIP marker value. |
| src/prep/cli.py | Refactors CLI options and switches to DataIO + runtime imports. |
| src/prep/api/pathio.py | Introduces a standalone PathIO for status tables and filesystem scanning. |
| src/prep/api/formatutils/media.py | Adds guess_ext() and write_to_file() utilities for media persistence. |
| src/prep/api/formatutils/init.py | Exposes write_to_file via the formatutils package API. |
| src/prep/api/formatter.py | Removes the implicit args.peek(d) preview side-effect during load. |
| src/prep/api/dataio.py | Refactors I/O into DataIO and replaces previous save/upload/preview behavior. |
| src/prep/api/init.py | Updates public API exports to include DataIO and new PathIO. |
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+130
to
134
| rprint(d) | ||
|
|
||
| save_root = self.default_save_path( | ||
| pipeline, as_parquet=False, id_override=id_override | ||
| ) |
| IMAGE_TAG = "<image>" | ||
| FORMATTING_PATTERN = re.compile(r"ANSWER:|<answer>|</answer>|\\boxed\{") | ||
| ID_PATTERN = re.compile(r"^[a-zA-Z0-9\._-]+$") | ||
| DEFAULT_UI = None if is_env_enabled("UI", "0") else False |
Comment on lines
+15
to
+44
| @dataclass(frozen=True) | ||
| class PathIO: | ||
| save_root: Path | ||
|
|
||
| def per_fmt_status( | ||
| self, | ||
| target_fmt: DataFormat, | ||
| registered_ids: dict[tuple[DataFormat, str], str | None], | ||
| ) -> dict[str, list]: | ||
| out: dict[str, list] = defaultdict(list) | ||
| for (fmt, id_), default_src in sorted(registered_ids.items()): | ||
| if fmt != target_fmt: | ||
| continue | ||
|
|
||
| local_dir = PathIO.dataset_dir(self.save_root, fmt, id_) | ||
| # show unregistered as None (N.A.) instead of False (not processed) | ||
| local_splits = self.scan_splits( | ||
| local_dir | ||
| ) | FormatterPipeline.unregistered_splits(id_=id_, target_format=fmt) | ||
|
|
||
| if not local_dir.exists() or not any(local_dir.glob("*")): | ||
| local_dir = None | ||
|
|
||
| out["ID"].append(id_) | ||
| out["Default Source"].append(default_src or "") | ||
| out["Local Path"].append(str(local_dir or "")) | ||
| out["Size"].append(self.size(local_dir)) | ||
| for split in get_valid_splits(): | ||
| out[split].append(local_splits.get(split)) | ||
| return out |
Comment on lines
+5
to
+51
| def guess_ext(raw: bytes, src_path: str | None) -> str: | ||
| from filetype import guess_extension | ||
|
|
||
| ext = guess_extension(raw) | ||
| if ext is None and src_path is not None: | ||
| ext = Path(src_path).suffix.lower().lstrip(".") | ||
| if ext is None: | ||
| raise ValueError("Cannot guess file extension from raw bytes or source path") | ||
|
|
||
| return ext | ||
|
|
||
|
|
||
| def write_to_file(entry: Any, dest: Path) -> Path: | ||
| import shutil | ||
|
|
||
| from PIL import Image as PILImage | ||
|
|
||
| if isinstance(entry, PILImage.Image): | ||
| ext = (entry.format or "PNG").lower() | ||
| if ext == "jpeg": | ||
| ext = "jpg" | ||
| img = entry | ||
| if ext == "jpg" and img.mode not in ("RGB", "L"): | ||
| img = img.convert("RGB") | ||
| out = dest.with_suffix(f".{ext}") | ||
| img.save(out) | ||
| return out | ||
|
|
||
| if isinstance(entry, dict): | ||
| raw, src_path = entry.get("bytes"), entry.get("path") | ||
| if raw: | ||
| out = dest.with_suffix(f".{guess_ext(raw, src_path)}") | ||
| out.write_bytes(raw) | ||
| return out | ||
| if src_path and Path(src_path).exists(): | ||
| src = Path(src_path) | ||
| out = dest.with_suffix(src.suffix.lower()) | ||
| shutil.copyfile(src, out) | ||
| return out | ||
|
|
||
| if isinstance(entry, (str, Path)) and Path(entry).exists(): | ||
| src = Path(entry) | ||
| out = dest.with_suffix(src.suffix.lower()) | ||
| shutil.copyfile(src, out) | ||
| return out | ||
|
|
||
| raise NotImplementedError(f"Cannot write entry of type {type(entry)} to file") |
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
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.
Before submitting
Changes