Repository navigation
feat(fetch,pull): --csv target lists; drop dt get --filter (0.22.0) - #179
Merged
Merged
Conversation
Row selection belongs upstream of dt. A user who wants a subset of a sample sheet can produce a subset sheet -- csvgrep, awk, pandas -- which they already know how to do and which leaves an artifact recording exactly what was selected. Carrying a filter grammar in dt bought little and cost its own error cases plus a second way to end up with a selection you did not intend. --filter reached only `dt get --csv`; `dt import --csv` never took it. It landed one day ago in 9323eef (0.18.0), so there is no meaningful exposure and no case for deprecating rather than deleting. parse_row_filters and row_matches go with it -- nothing else called them. Also extracts _read_csv_rows (locate the file, insist on the path column, reject a header-only file) and adds read_csv_target_list on top of it, the reader for the forthcoming `dt fetch --csv` / `dt pull --csv`. Those act on the current repo, where a row names a target and there is no destination to choose, so it returns plain strings and ignores every other column. Its one sharp edge is deliberate: a blank path cell is fatal rather than a reportable per-row failure. read_csv_targets keeps such rows so dt get can report them, but here an empty-string target resolves to the working directory, and both callers read an empty target list as "operate on the whole repo". Together with the header-only check that makes an empty return impossible, so callers may rely on the result being non-empty. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both commands took targets on argv only, so the moment a selection lived in a
sample sheet you were back to `$(cut -d, -f2 sheet.csv | tail -n+2)` -- hand-
counting columns and losing the header names that made the sheet readable.
dt pull --csv samples.csv
dt pull --csv samples.csv --path-col fq_dir
dt fetch --csv samples.csv --regular --dry
This is narrower than `dt get --csv`. That reads two meaningful columns, a
source path in a remote repository and an optional `output` naming where to put
it. fetch and pull act on the current repo, where the destination is fixed by
the repo layout, so --csv is only a way to build the target list and every
column but --path-col is ignored -- `output` no differently from `sample_id`.
A sheet written for `dt get` stays usable: if its `path` holds registry paths
and `output` the local ones, `--path-col output` is the invocation.
The hazard worth the code. Both commands read targets=None as *every stage*,
which is right for a bare invocation and disastrous for a CSV -- a sheet that
yielded nothing would silently widen to a whole-repo fetch or pull. So
read_csv_target_list cannot return an empty list (a header-only file and a
blank path cell are both errors), and _resolve_targets refuses to build one
anyway. The redundancy is deliberate: `x if x else None` already appears three
times in this file and is what a later edit would reach for by reflex.
Also adds _csv_context, which appends the CSV, the column, and the target count
to FetchError/PullError. DVC names the first unresolvable target and stops; over
a 400-row sheet that is one run per typo, and nothing in the message said the
path came from a file at all.
Not building per-target pre-validation. Reporting all misses at once would mean
collect_stages(targets=None) first, and a full collect on a large repo is
exactly the work that passing targets avoids.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Adds
--csv/--path-coltodt fetchanddt pull, and removes--filterfromdt get --csvso every CSV-consuming command agrees on the same two flags.Design record:
docs/csv-targets.md.Why this is narrower than
dt get --csvdt get/dt importread two meaningful columns — a source path in a remote repository, and an optionaloutputnaming where to put it locally.dt fetch/dt pullact on the current repo, where the destination is fixed by the repo layout. So--csvhere is only a way to build the target list, and every column but--path-colis ignored —outputno differently fromsample_id.A sheet written for
dt getstays usable: if itspathcolumn holds registry paths andoutputthe local ones,--path-col outputis the invocation. No new flag, just the existing one pointed elsewhere.The hazard worth the code
Both commands read
targets=Noneas every stage — correct for a bare invocation, disastrous for a CSV. A sheet that yielded nothing would silently widen into a whole-repo pull.Two layers, deliberately redundant:
read_csv_target_listcannot return an empty list. A header-only file is already an error, and a blank cell in the path column is now one too (an empty-string target resolves to the working directory). It names the line, since a hand-edited sheet is the usual source._resolve_targetsrefuses to build one anyway.x if x else Nonealready appears three times incli.pyand is exactly what a later edit would reach for by reflex.TestBadCsvNeverWidensTheOperationparametrises every way a CSV can be wrong × both commands, and asserts the command never calls through.Removing
--filterRow selection belongs upstream.
csvgrep/awk/pandas already do it well, and doing it beforehand leaves an artifact recording exactly what was transferred. Carrying a filter grammar cost its own error cases and a second way to end up with a selection you didn't intend.It reached only
dt get --csv—dt import --csvnever took it — and landed one day before this branch in 9323eef (0.18.0), so there is no meaningful exposure and no case for deprecating rather than deleting.parse_row_filtersandrow_matchesgo with it; nothing else called them.Also
_csv_contextappends the CSV, the column, and the target count toFetchError/PullError. DVC names the first unresolvable target and stops; over a 400-row sheet that's one run per typo, with nothing saying the path came from a file.Not built: per-target pre-validation. Reporting all misses at once means
collect_stages(targets=None)first, and a full collect on a large repo is exactly the work that passing targets avoids. Revisit if one-typo-per-run proves annoying.Cut from scope:
dt push --csv. In simple mode push never parses targets at all —ctx.argsgoes todvc pushverbatim — so--csvwould make it the first place push builds its own argv, with validation landing in DVC. The use case is also thin. Reasoning recorded in the design doc's non-goals.Tests
2100 passed, 2 skipped, run under PBS via
qxub, not on the login node. 38 new tests intests/unit/test_csv_targets.py.Two pre-existing failures under PBS, not from this branch:
TestFetchUrlImport::test_successful_updateand::test_failed_update.fetch._fetch_url_importmakes a realcheck_network_connectivityprobe that the tests don't mock, so they pass on the login node and fail on a compute node.dt/fetch.pyanddt/pull.pyare byte-identical tomainon this branch — the fix is a two-line patch in those tests, worth doing separately.🤖 Generated with Claude Code