Conversation
Introduces a stable, agent-friendly CLI exposed via the `featcopilot`
console script (and `python -m featcopilot`). All subcommands accept
`--json` for machine-readable stdout; user-facing errors are written
to stderr with a non-zero exit code so agents (e.g. Copilot tool-use,
shell scripts, CI pipelines) can parse failures deterministically.
Subcommands:
* `info` — print version, supported engines, selection methods,
leakage guards, and supported I/O formats.
* `transform` — read CSV / Parquet / JSON, run AutoFeatureEngineer,
write engineered features. Supports `--config` JSON,
`--engines`, `--max-features`, `--selection-methods`,
`--correlation-threshold`, `--leakage-guard`,
`--gate-n-jobs`, `--no-selection`, `--include-target`,
and explicit `--input-format` / `--output-format`
overrides. Emits a JSON status payload (rows, features,
engines, selection_applied, ...) when `--json` is set.
* `explain` — fit AutoFeatureEngineer and print a JSON document with
`{name, explanation, code}` per feature so an LLM can
consume the result directly.
Files:
* `featcopilot/cli.py` — argparse-based CLI (no new dependencies).
* `featcopilot/__main__.py` — enables `python -m featcopilot`.
* `pyproject.toml` — `[project.scripts]` entry point.
* `tests/test_cli.py` — 18 tests covering info/transform/explain,
CSV/Parquet/JSON round-trips, `--config`
handling and override precedence, and all
user-facing error paths.
* `README.md` — new "Command-Line Interface" section.
Coverage: featcopilot/cli.py at 94 %; project total 88.95 %
(`--cov-fail-under=85`).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5 +/- ##
==========================================
+ Coverage 88.90% 89.24% +0.34%
==========================================
Files 36 38 +2
Lines 3937 4351 +414
==========================================
+ Hits 3500 3883 +383
- Misses 437 468 +31
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a new command-line interface for FeatCopilot so the library can be invoked from shells, CI, and agent/tool workflows without custom Python wrappers. It adds a package entry point plus tests and README guidance around the new info, transform, and explain commands.
Changes:
- Add a new
featcopilotargparse-based CLI withinfo,transform, andexplainsubcommands. - Wire up module/package entry points so the CLI can run via both
featcopilotandpython -m featcopilot. - Add CLI-focused tests and README documentation for command usage, formats, and config precedence.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
featcopilot/cli.py |
Implements the new CLI, argument parsing, config loading, I/O handling, and subcommand execution. |
featcopilot/__main__.py |
Adds python -m featcopilot module dispatch to the CLI entry point. |
pyproject.toml |
Registers the new featcopilot console script. |
tests/test_cli.py |
Adds coverage for CLI output modes, transform/explain flows, config precedence, and error paths. |
README.md |
Documents the new CLI and example invocations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f0b6616f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Addresses all five review comments from Copilot and Codex on PR #5: * explain now actually returns generated features (Copilot review #1, Codex P1). Built-in engines (e.g. tabular) populate `_feature_names` during `transform()`, not `fit()`. `_cmd_explain` now calls `fit_transform(..., apply_selection=False)` so the JSON payload contains the full `{name, explanation, code}` records the subcommand advertises. Test asserts `n_features > 0` for tabular. * main(argv) -> int contract honored on parse errors (Copilot review #2). `argparse.parse_args` raises `SystemExit` for usage errors, `--help` and `--version`. `main` now traps those and returns the exit code so programmatic and agent callers always get an int. Tests cover `--version` (rc=0), `--help` (rc=0), no-subcommand (rc=2) and unknown-flag (rc=2). * Real subprocess test for python -m featcopilot (Copilot review #3). `test_dunder_main_subprocess_invocation` and `test_dunder_main_subprocess_version_flag` spawn a real `python -m featcopilot ...` subprocess and assert stdout JSON, so a regression in `__main__.py` actually breaks the suite. * Parquet `ImportError` -> clean exit 2 (Codex P2). `_read_table`/`_write_table` now wrap parquet calls and convert `ImportError` into a `ValueError` with a friendly install hint; the top-level handler routes that to the deterministic `exit 2` user-error path instead of the generic `exit 1` backstop. `test_transform_parquet_missing_engine_returns_exit_2` exercises this via `monkeypatch` of `DataFrame.to_parquet`. * Pre-commit black: re-applied formatting from the pinned `black 24.1.1` hook (joined two long string raises) so the CI pre-commit job passes. Tests: 23 (+5 new) in tests/test_cli.py, 796 passed full suite. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@copilot please review the entire PR (not only the latest commit). Round-1 review comments from both Copilot and Codex have been addressed in commit b999555 and the diff against main reflects all CLI changes. Looking for any remaining concerns across eatcopilot/cli.py, eatcopilot/main.py, ests/test_cli.py, pyproject.toml, and the README CLI section. |
Full PR review complete. All three round-1 concerns were correctly addressed in b999555: 1. 2. 3. No remaining concerns across the five changed files:
|
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Round-1 review feedback from both Copilot and Codex (5 comments total) has been addressed in commit b999555 and all 5 review threads have been replied to and resolved. CI is green (pre-commit + tests on Python 3.10/3.11/3.12/3.13 + codecov). Codex re-reviewed and explicitly confirmed Didn't find any major issues. (#5 (comment)). @copilot-pull-request-reviewer please perform a fresh full-PR review against the current head (b999555), covering the entire diff against \main\ — not only the latest commit. |
No code changes. Triggers another round of automated review from the Copilot/Codex review bots so the full PR (not just the latest commit) can be re-evaluated against the current head after round-1 fixes in b999555 (which addressed all 5 review comments and resolved all 5 review threads). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses all five new review comments from copilot-pull-request-reviewer on PR #5: * info now reflects parquet engine availability at runtime. `_parquet_engine_available()` probes `pyarrow` / `fastparquet`; parquet is included in `supported_input_formats` / `supported_output_formats` only when an engine is importable. Adds a new `"parquet_available"` boolean in the payload so agents can branch on it deterministically. Tests cover both the engine-present and engine-missing cases via `monkeypatch`. * _build_engineer no longer mangles misconfigured configs. Removed the `list(...)` coercion of `engines` / `selection_methods`: a misconfigured `"engines": "tabular"` (string) used to be silently expanded into `['t','a','b','u','l','a','r']` and bubble up as a confusing "unknown engines" error. It now flows straight into `AutoFeatureEngineer.__init__`'s precise type validation ("engines must be a list or tuple of strings"), surfaced via the standard exit-2 user-error path. * Empty config lists now produce the documented exit 2. Replaced `pick(...) or [default]` with a tri-state `pick` that honors explicit empty values from the config: `"engines": []` / `"selection_methods": []` now propagate into the transformer where `_validate_configuration` raises "must contain at least one ..." and the CLI returns exit 2 — instead of being silently rewritten into the defaults. * README parquet caveat. Switched the example to CSV/CSV (no surprise dependency for base installs) and added an explicit note that parquet I/O requires installing `pyarrow` or `fastparquet`, with a pointer to `info`'s `parquet_available` flag for runtime detection. * Console script test. New `test_console_script_subprocess_invocation` and `test_console_script_version_flag` use `shutil.which` to locate the installed `featcopilot` script and run it through `subprocess`; a typo or packaging regression in `[project.scripts]` now actually breaks the suite. Tests skip cleanly when the script is not on PATH. Tests: 30 (+7 new) in tests/test_cli.py, 803 passed full suite. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Round-2 review feedback (5 new comments from copilot-pull-request-reviewer at 07:27Z) has been addressed in 92fcd09 and all 5 round-2 threads have been replied to and resolved. CI is running. @codex review the whole PR (not only the latest commit). Diff against \main\ covers \ eatcopilot/cli.py, \ eatcopilot/main.py, \ ests/test_cli.py, \pyproject.toml, and the README CLI section. @copilot-pull-request-reviewer please re-review the entire PR against the current head (92fcd09) — not only the latest commit. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92fcd090fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Addresses both new comments from chatgpt-codex-connector on PR #5 (commit 92fcd09): * Validate `llm_config` is a JSON object (Codex P2). `_build_engineer` now type-checks the `llm_config` value from `--config` before forwarding it. A non-mapping (e.g. a string) previously made it all the way into `AutoFeatureEngineer._create_engine` where `self.llm_config.get(...)` raised `AttributeError` — bypassing the structured exit-2 user-error path and surfacing as exit 1 `unexpected error`. The new check raises a precise `ValueError` so the CLI returns exit 2 with a clean stderr message. Test `test_non_dict_llm_config_returns_exit_2` covers this. * Normalize `--config` user-input mistakes to exit 2 (Codex P3). Pointing `--config` at a directory used to raise `IsADirectoryError` from `path.open(...)` and fall into the generic exit-1 backstop. `_load_config` now: - rejects directories explicitly, - converts `json.JSONDecodeError` into a clean "is not valid JSON" message, - converts other read errors (`OSError`) into a clean "could not be read" message. All paths return exit 2 so automation can handle config errors consistently. Tests `test_directory_as_config_returns_exit_2` and `test_malformed_json_config_returns_exit_2` cover the new branches. Tests: 33 (+3 new) in tests/test_cli.py, 806 passed full suite. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Round-3 review feedback (2 P2/P3 comments from Codex on 92fcd09) has been addressed in 0072e5b and both threads have been replied to and resolved. @codex review the whole PR (not only the latest commit). Diff against \main\ covers all CLI files. @copilot-pull-request-reviewer please re-review the entire PR against the current head (0072e5b). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses all three new comments from round-19 reviewers on PR #5 (commit f710fbe): * No false sampling warning at exact-boundary (Codex gZs1 P3, Copilot gmOw — same root cause). The previous `n_sampled >= sample_size` check fired the "metadata may differ" warning even when the input had EXACTLY `sample_size` rows (no truncation actually happened), causing agents to misinterpret an unsampled run as degraded metadata. `_cmd_explain` now reads with `nrows = sample_size + 1` so the returned length is a strict proof of the file size relative to the cap: `len(df) > sample_size` means at least one row was dropped. The warning fires only in that case; the post-read truncation to exactly `sample_size` happens at the same time. CSV memory bound is preserved (`pd.read_csv(nrows=N+1)` still caps at N+1, no full load). Three new tests cover the three boundary cases: - input exactly = cap -> no warning, n_rows_used == cap - input < cap -> no warning, n_rows_used == input size - input = cap + 1 -> warning fires, n_rows_used == cap * --explain-sample-size help text describes actual semantics (Copilot gmPC). The help previously said "deterministic seed", implying a seeded random sample, but the implementation is now a deterministic head slice (`read_csv(nrows=N)` / `df.iloc[:N]`). The help and the docstring both updated to say "deterministic head slice (the first N rows of the input)" and to call out explicitly that this is NOT a random sample. New `test_explain_sample_size_help_text_describes_head_slice_not_random_seed` asserts the new wording and a regression guard against the old. Tests: 116 (+4 new) in tests/test_cli.py, 889 passed full suite. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Round-19 review feedback (1 Codex P3 + 2 Copilot, 3 threads on commit f710fbe) has been addressed in bfb5da8 and all 3 threads have been replied to and resolved. @copilot please review the entire PR (not only the latest commit) — full diff against @codex review the whole PR (not only the latest commit). Diff against |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfb5da8373
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Addresses the Codex P2 comment on PR #5 (commit bfb5da8): * Reject empty / header-only inputs with exit 2 (Codex gtYr). When a CSV has headers but zero data rows, `pd.read_csv` returns an empty DataFrame rather than raising `EmptyDataError`. Same for an empty JSON array (`[]`) or a parquet file with schema but zero rows. The CLI then passed that frame into `TabularEngine`, which divides by `len(X)` while fitting categorical encoding and the command exited via the generic exit-1 `unexpected error` path instead of a clean user-input error. `_read_table` now performs an explicit `df.empty` check after every supported format read and raises `ValueError("Input file ... is empty (zero data rows). Feature engineering requires at least one row of data.")` -> exit 2 with a precise stderr message. Four new tests cover the new branch: - header-only CSV (transform) - empty JSON array (transform) - parquet with schema but zero rows (transform) - header-only CSV (explain) Tests: 120 (+4 new) in tests/test_cli.py, 893 passed full suite. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Round-20 review feedback (Codex P2 on commit bfb5da8) has been addressed in 167b490 and the thread has been replied to and resolved. @copilot please review the entire PR (not only the latest commit) — full diff against @codex review the whole PR (not only the latest commit). Diff against |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
All previous review comments and check failures have been addressed and resolved (60 review threads resolved, 0 unresolved). The PR is currently green: all 7 CI checks pass, 120 CLI tests in @copilot please review the entire PR (not only the latest commit) — full diff against @codex review the whole PR (not only the latest commit). Diff against |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses all three new comments from copilot-pull-request-reviewer on PR #5 (commits 167b490 + 5cbb843): * Drop the over-broad single-active-capture fallback (Copilot hFmA). Round-18's `_ThreadCaptureState.get` fallback routed records from ANY thread to the single in-flight CLI capture whenever exactly one capture was active. As Copilot pointed out, that was too broad: unrelated background work using `featcopilot` in the same process would have its log/warning output silently swallowed into the active CLI command's payload, with no ownership tie back to the command. `get` is now strictly per-thread; cross-thread records (e.g. ThreadPoolExecutor workers) bleed onto stderr like any other background log. The class docstring documents the trade-off and the existing options for callers who really need every worker record captured. The previous worker-routing test was inverted into `test_capture_does_not_route_unrelated_thread_records`, which guards against the regression. * Structured single-line argparse error format (Copilot hFmP). `argparse`'s default `error()` writes the multi-line usage banner (`usage: featcopilot ...`) PLUS `prog: error: ...` to stderr before raising `SystemExit`. `main()` was only converting the exit code; the banner still appeared on stderr, breaking the documented "stderr carries one `featcopilot: error: ...` line per failure" contract. New `_StructuredArgumentParser` overrides `error()` to emit exactly one `featcopilot: error: <message>` line with no banner, and is wired into both the top-level parser and all subparsers via `parser_class=_StructuredArgumentParser`. Two new tests cover the contract: single-line stderr for an unknown flag and for a missing subcommand, plus a regression guard against the `usage:` substring appearing in stderr. * Eliminate double truncation warning for parquet/JSON sample (Copilot iH8R). `_cmd_explain` was passing `nrows=sample_size+1` to `_read_table` so it could detect truncation. For parquet/JSON `_read_table` then emitted its own post-read warning saying "truncating to N+1" and trimmed to that, after which `_cmd_explain` trimmed again to N and emitted a second warning. Two near-duplicate warnings with off-by-one numbers — confusing to agents. The fix: - `_read_table` now accepts `suppress_truncation_warning=True` so callers that emit their own consolidated message can silence its post-read notice. - `_cmd_explain` only passes `nrows` to `_read_table` when pandas can natively bound the read (CSV with `nrows=N+1`). For parquet/JSON it reads with `nrows=None` (full file) and handles both detection and trimming itself. - The single `_cmd_explain` UserWarning now uses the user-facing `sample_size` value, AND for parquet/JSON includes the "memory wasn't bounded" caveat as a single sentence — same information, no duplication. Test `test_explain_sample_size_warns_post_read_for_parquet` asserts exactly one truncation notice and that it references `100 rows` (not the internal +1). Tests: 122 (+2 net) in tests/test_cli.py, 895 passed full suite. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Round-22 review feedback (3 comments from auto Copilot reviewer on commits 167b490 and 5cbb843, missed by my previous @copilot please review the entire PR (not only the latest commit) — full diff against @codex review the whole PR (not only the latest commit). Diff against |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses the new comment from copilot-pull-request-reviewer on PR #5 (commit 016fe34): * Restore narrow LLM-only cross-thread fallback (Copilot osJO). Round-22 removed the single-active-capture fallback in response to Copilot's complaint that it was too broad (would misattribute unrelated background `featcopilot` work). Round-23 Copilot now flags that the strict per-thread routing breaks the "stderr is reserved for failures" contract for LLM-backed runs in event-loop environments — the sync LLM clients in `featcopilot/llm/*_client.py` fall back to `ThreadPoolExecutor` there, and their mock-mode startup warnings emit from worker threads that `submit()` spawns. Resolution: a *narrow* fallback that satisfies both reviewers. `_ThreadCaptureState` now exposes `get_for_llm_record(tid, logger_name)`: it returns the calling thread's capture if any, otherwise — and only when the record originates from `featcopilot.llm.*` AND exactly one capture is active in the process — it routes to that single capture. Multiple concurrent captures keep strict isolation (no LLM cross-talk either). Records from non-LLM featcopilot loggers on cross-threads still flow to stderr as before, so unrelated background work is NOT misattributed. Both `_ThreadRoutingHandler.emit` and `_SuppressCapturingFilter.filter` go through `get_for_llm_record`, so the routing handler and the stderr- suppression filter stay in lockstep: anything captured is also suppressed from the original handlers. Three tests cover the policy: - `test_capture_does_not_route_unrelated_thread_records`: non-LLM record from a cross-thread is NOT captured (regression guard against round-22's "too broad" complaint). - `test_capture_routes_llm_client_worker_records_to_single_active_capture`: LLM record from ThreadPoolExecutor + raw threading.Thread workers IS captured. - `test_capture_does_not_apply_llm_fallback_with_multiple_captures`: even for LLM records, two concurrent captures stay strictly isolated. Tests: 124 (+2 net) in tests/test_cli.py, 897 passed full suite. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Round-23 review feedback (1 comment from auto Copilot reviewer on commit 016fe34) has been addressed in 298d4de. The strict per-thread isolation introduced in round-22 was conflicting with round-23''s observation about LLM sync clients in event-loop environments — both concerns are now satisfied by a narrow @copilot please review the entire PR (not only the latest commit) — full diff against @codex review the whole PR (not only the latest commit). Diff against |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 298d4def60
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Round-23's `test_capture_does_not_apply_llm_fallback_with_multiple_captures`
passed locally but failed in CI on Python 3.10/3.12/3.13. Root cause: a
race between the two threads. Without explicit barriers, one thread's
`ThreadPoolExecutor` worker could fire BEFORE the other thread had
pushed its capture onto `_state._per_thread`. At that moment
`len(_per_thread) == 1`, so the narrow LLM fallback (correctly)
routed the worker's record to the only active capture — which is what
the test was checking the multi-capture scenario does NOT do.
Replaced the single `Barrier(2)` with a three-phase protocol:
- `enter_barrier`: both threads start at the same time
- `inside_barrier`: BOTH have pushed their capture (so any
subsequent emit sees `len == 2`)
- `done_barrier`: BOTH workers have completed before EITHER
exits its capture (pins `len == 2` for the entire worker
window)
The contract under test — multi-capture isolation — is unchanged; the
test now reliably exercises the multi-capture branch of
`_ThreadCaptureState.get_for_llm_record`.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Round-24 reviewer feedback addressed:
1. **Codex P1 — atomic routing/suppression decision.**
`_SuppressCapturingFilter.filter()` and `_ThreadRoutingHandler.emit()`
each independently called `state.get_for_llm_record(...)`. Under
concurrent CLI calls, another thread could push or pop a capture
between filter and emit so the two phases saw different state — the
same record could end up both captured AND on stderr (or suppressed
without being captured). Replaced both call sites with
`state.resolve_for_record(record)`, which computes the decision
once and caches it on the record itself; the two phases now always
see the same answer.
2. **Copilot — narrow LLM whitelist was too broad.**
`_LLM_FALLBACK_LOGGER_PREFIXES = ("featcopilot.llm.",)` matched
every `featcopilot.llm.*` logger, including modules that never
hop onto worker threads (`semantic_engine`, `code_generator`,
`transform_rule_generator`, `explainer`). Replaced with an
*exact* set `_LLM_FALLBACK_LOGGER_NAMES` containing only the
three sync-client modules that actually fall back to
`ThreadPoolExecutor` (`copilot_client`, `litellm_client`,
`openai_client`).
3. **Copilot — `_run` did not actually capture handler output.**
`featcopilot/utils/logger.py` installs `StreamHandler(sys.stderr)`
at import time, so `redirect_stderr` only swapped the module
attribute while the handler kept writing to the *original* stream.
That made the file's stderr-cleanliness assertions vacuous. The
`_run` helper now also temporarily re-points every
`StreamHandler` on the `featcopilot` logger at the captured
`err` buffer for the duration of the call, restoring the original
`stream` attribute in `finally`.
New tests:
- `test_capture_does_not_apply_llm_fallback_for_non_whitelisted_llm_loggers`
pins the exact-set whitelist (semantic_engine et al. stay strictly
isolated).
- `test_capture_decision_is_cached_per_record_for_atomic_filter_emit`
proves the resolver computes the decision exactly once per record.
- `test_capture_decision_stable_under_concurrent_pop_between_filter_and_emit`
proves the cached decision survives a concurrent pop.
- `test_run_helper_redirects_featcopilot_stream_handlers` proves the
test helper captures handler-stream writes (so leaks would be
detectable, not silently passing).
Existing test `test_capture_routes_llm_client_worker_records_to_single_active_capture`
updated to use a whitelisted logger name (`openai_client`) since
arbitrary `featcopilot.llm.*` names no longer trigger the fallback.
901 tests pass locally (full suite); 128 in tests/test_cli.py.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Round 24 push at 1005009 addresses all three previously-unresolved threads:
Local: 901 tests pass (full suite), 128 in @copilot @codex please re-review the WHOLE PR (not just the latest commit) — the round-23/24 capture changes touch |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ut cases
Round-25 reviewer feedback addressed:
1. **Copilot — empty-input branch conflated zero-row and zero-column.**
`_read_table` raised the same "zero data rows" error for both
`len(df) == 0` and `len(df.columns) == 0` (both make
`DataFrame.empty` return `True`). A JSON `[{}, {}]` array
produces a frame WITH rows but NO columns ù very different user
error than a header-only CSV. Split into two distinct, accurately
worded error messages: "no columns" vs "zero data rows".
2. **Copilot — read-time warnings leaked to stderr.**
`_cmd_transform` only opened the capture context around
`engineer.fit_transform`. `pd.read_csv` can legitimately emit
`DtypeWarning` on mixed-type CSVs and parquet/JSON readers can
emit pyarrow / pandas warnings on a successful read; those were
bypassing the capture and bleeding to stderr, breaking the
"stderr reserved for failures" contract.
3. **Copilot — write-time warnings also leaked to stderr.**
`_write_table` was likewise outside the capture, so pandas /
pyarrow `FutureWarning` / `UserWarning` from a successful
write could leak. Same contract violation as #2.
The fix wraps the entire `_cmd_transform` pipeline (read +
build_engineer + fit_transform + write) in a single
`_capture_featcopilot_messages` block so warnings from ANY phase end
up in the JSON `warnings` field instead of stderr. `_cmd_explain`
also widened: `explain_features` / `get_feature_code` are now
inside the same capture as the read+fit_transform.
The dead helper `_fit_transform_capturing_warnings` is no longer used
internally; kept as a thin convenience wrapper for external test code
with an updated docstring noting that the CLI now wraps a wider
region.
New tests:
- `test_transform_zero_columns_input_distinguishes_from_zero_rows` ù
pins the column-vs-row error-message distinction.
- `test_transform_zero_rows_input_still_uses_zero_rows_message` ù
guards the existing zero-rows wording for the header-only case.
- `test_transform_read_warning_captured_not_on_stderr` ù patches
`pd.read_csv` to emit a warning, asserts `err == ""` and the
warning lands in JSON `warnings`.
- `test_transform_write_warning_captured_not_on_stderr` ù patches
`DataFrame.to_csv` to emit a warning, same contract.
- `test_explain_features_warnings_captured_not_on_stderr` ù patches
`AutoFeatureEngineer.explain_features` to emit a warning, same
contract for `_cmd_explain`.
906 tests pass locally (full suite); 133 in tests/test_cli.py.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Round 25 push at 99fe807 addresses all three previously-unresolved threads:
Local: 906 tests pass (full suite); 133 in @copilot @codex please re-review the WHOLE PR (not just the latest commit). Thanks! |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except ValueError as exc: | ||
| raise ValueError(f"Failed to read JSON from {str(path)!r}: {exc}") from exc | ||
| except OSError as exc: |
| try: | ||
| with path.open("r", encoding="utf-8") as fh: | ||
| data = json.load(fh) | ||
| except json.JSONDecodeError as exc: | ||
| raise ValueError(f"Config file {config_path!r} is not valid JSON: {exc}") from exc | ||
| except OSError as exc: | ||
| # Catch-all for unreadable files (permission denied, broken symlink, | ||
| # etc.). Surface as a user-facing error rather than the generic | ||
| # exit-1 backstop. | ||
| raise ValueError(f"Config file {config_path!r} could not be read: {exc}") from exc |
| with _install_lock: | ||
| # Logger handler/filter install (truly once — these can't be | ||
| # silently undone by external code in the way ``warnings.showwarning`` | ||
| # can). | ||
| if not _install_done: | ||
| fc_root = logging.getLogger("featcopilot") | ||
| if _routing_handler not in fc_root.handlers: | ||
| fc_root.addHandler(_routing_handler) | ||
| for handler in list(fc_root.handlers): | ||
| if handler is _routing_handler: | ||
| continue | ||
| if _suppress_filter not in handler.filters: | ||
| handler.addFilter(_suppress_filter) | ||
| _install_done = True |
Summary
Adds a stable, agent-friendly
featcopilotcommand-line interface so FeatCopilot can be used from shells, CI pipelines, and agentic / LLM tool-use workflows without writing Python glue. All subcommands accept--jsonfor machine-readable stdout; user-facing errors are written to stderr with a non-zero exit code so agents can parse failures deterministically.Subcommands
featcopilot infoparquet_availableflag.featcopilot transformAutoFeatureEngineer, write engineered features.featcopilot explain{name, explanation, code}per feature for LLM consumption. Selection is intentionally disabled.Module form is also wired up:
python -m featcopilot ....Examples
Pass
--config config.jsonto provide nested keys such asllm_config; explicit CLI flags override config values.Implementation notes
argparseonly — no new dependencies..csv/.parquet/.pq/.parq/.json) with explicit--input-format/--output-formatoverride.transformreturns a structured JSON payload:status,input,output,input_format,output_format,n_rows,n_features,n_input_columns,n_generated_features,engines,selection_methods,max_features,target,selection_applied.0success,2user/input/config error (with clean stderr message — no traceback),1unexpected error,130keyboard interrupt.--targetwhen selection would actually run) are normalized intoValueError-> exit 2 instead of falling into the generic exit-1 backstop.Engineer-config flags
transformexplain--engines--max-features--selection-methods--correlation-threshold--leakage-guard--gate-n-jobs--config--verbose/--no-verbose--no-selection--include-targetexplainrejects--selection-methodsand--correlation-thresholdon the CLI and also skips reading those keys from--config(they are inert at runtime since explain disables selection).Tests
tests/test_cli.py— 68 tests coveringinfo(JSON/text + parquet engine probing),--version/--help,transform(CSV/Parquet/JSON round-trips,--no-selection,--include-target, format overrides),explain,--configprecedence vs CLI flags, and all user-facing error paths (missing input/output, unknown target, unknown extension, malformed config / non-object root / directory / unreadable / non-dictllm_config, scalar-type mismatches,--no-verbose/--verboseoverriding config bool, parquet engine missing on read/write, unwritable output, uncreatable parent dir, missing--targetwhen selection would run,--targetnot required when selection is a no-op).featcopilot/cli.pyat >90%; project total 89.22% (--cov-fail-under=85).Files changed
featcopilot/cli.py(new) — argparse CLI.featcopilot/__main__.py(new) —python -m featcopilotentry.pyproject.toml—[project.scripts] featcopilot = "featcopilot.cli:main".tests/test_cli.py(new) — 68 tests.README.md— new "Command-Line Interface" section.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com