Skip to content

Repository-level data-table linting for data-manager / reference-data bundles - #23229

Draft
jmchilton wants to merge 14 commits into
galaxyproject:devfrom
jmchilton:data_validation
Draft

Repository-level data-table linting for data-manager / reference-data bundles#23229
jmchilton wants to merge 14 commits into
galaxyproject:devfrom
jmchilton:data_validation

Conversation

@jmchilton

Copy link
Copy Markdown
Member

Implements the repository-level lint block proposed in galaxyproject/planemo#1672:
validates a data-manager / reference-data repository as a connected bundle
(data_manager_conf.xml + tool_data_table_conf.xml* + .loc/.loc.sample
fixtures + consumer from_data_table references), rather than only the individual
tool wrappers.

Adds eight linters, a standalone galaxy-tool-data-lint <repo> CLI, and wiring
into Planemo shed_lint. The linters run cleanly across the entire tools-iuc tree
(780 repositories) and surfaced real bugs, each fixed in its own PR. This
consolidates several long-standing lint requests: galaxyproject/planemo#96,
galaxyproject/planemo#531, galaxyproject/planemo#706, galaxyproject/planemo#869.

Architecture

Discovery and analysis are split from linting. build_repository_data_tables(repo_root, ...)
assembles a single RepositoryDataTables model — the realized bundle — from the
manager configs, table configs, loc assets, and expanded consumer tool sources; the
linters are then pure functions over that model and never touch the filesystem. This
keeps each check small and independently testable, and lets one traversal feed every
linter.

Key model facts the linters key on:

  • Loc resolution is tri-state per referenced asset, not a boolean. found is what
    Galaxy's own loader resolves; is_sample marks a production loc satisfied only via
    the loader's .sample fallback; sample_backed recognizes the real Tool Shed layout
    (tool-data/<name>.loc.sample, materialized on install) that the loader's fallback
    misses. Only the combination distinguishes a genuinely missing fixture from a
    reference-data repo that is correctly shipped — without it every such repo would be
    falsely flagged.
  • Assembly is defensive. A table declared with conflicting schemas would make the
    loader's merge raise, so assembly falls back to raw declarations and records the
    conflict as data rather than crashing; ConflictingTableSchema then reports it.
  • Evidence is graded. Only literal, fully macro-expanded names are checked;
    unresolved dynamic/@TOKEN@ references are treated as "not checked," never as
    demonstrably missing (the Data manager update for metaphlan tools-iuc#5003 false-positive guard).

Each linter is dispatched through LintContext.lint(name, ...), so every check is
individually skippable by name the same way Planemo drives the tool linters.
Severity is deliberate: a broken producer/consumer contract provable from static
evidence is an error; a condition that a legitimately external supplier could
explain is a warning (see ConsumerTableDefined). The same model drives all three
entry points — lint_repository_data_tables (model in hand),
lint_repository_data_tables_bundle (caller did discovery), and
find_and_lint_repository_data_tables (one-call discover-and-lint, used by the CLI and
shed_lint).

Linters

Linter Severity Checks
LocRowShape error loc rows use the configured separator and can fill every declared column
MissingLocFixture error a configured table's referenced loc resolves to a file or a shipped sample
ManagerTableConfigured error every data-manager table has a matching tool_data_table_conf entry
OutputRefValid error each output_ref names a real output of the expanded manager wrapper
DuplicateColumnNames error no table declares the same column name twice
ConflictingTableSchema error a table is declared with one consistent schema across the repo
ConsumerTableDefined warning a consumed from_data_table table is defined locally or supplied externally
EmptyLocFile warning a .loc / .loc.sample that is empty carries a format comment

All eight pass across every tools-iuc repository that declares a data-table bundle;
the row, fixture, consumer, and empty-loc linters found real defects.

LocRowShape and MissingLocFixture — proven against real repositories

LocRowShape flags space- instead of TAB-delimited rows and truncated rows that
cannot fill the declared columns: galaxyproject/tools-iuc#8234 (checkm2 data manager),
galaxyproject/tools-iuc#8235 (dram), galaxyproject/tools-iuc#8237 (tools/checkm2),
galaxyproject/tools-iuc#8239 (coverm), galaxyproject/tools-iuc#8240 (vsnp_genbank).
MissingLocFixture flags a configured table whose loc resolves to nothing and no
shipped .sample backs: galaxyproject/tools-iuc#8236 (hisat2 index builder — a stray
bwa_mem_indexes table). Loc resolution honours the shed layout
(tool-data/<name>.loc.sample, materialized on install) so real reference-data
repositories are not falsely flagged. Addresses galaxyproject/planemo#96.

ManagerTableConfigured / OutputRefValid / DuplicateColumnNames / ConflictingTableSchema — structural

Guard rarer structural misconfigurations. All 285 bundle repositories in tools-iuc
pass them (verified by running the linters over the current tree); each is covered by
unit fixtures. ManagerTableConfigured addresses galaxyproject/planemo#706 and
galaxyproject/planemo#531; the remaining three are defensive checks with no live
tools-iuc incident, exercised only by fixtures.
Representative repositories they validate cleanly: data_manager_fetch_genome_dbkeys_all_fasta
(a multi-table producer of all_fasta + __dbkeys__), data_manager_bwa_mem2_index_builder,
and data_manager_star_index_builder (index builders whose output_ref columns and
configured tables all resolve).

ConsumerTableDefined — undefined table references

A warning, never an error: a consumed table may legitimately be supplied by Galaxy
core or another installed repository, so an undefined reference is advisory. The
common core tables (all_fasta, fasta_indexes, __dbkeys__) are treated as
externally supplied by default (DEFAULT_EXTERNAL_TABLE_NAMES); callers extend the
set via external_table_names. Across tools-iuc it flagged one genuine
inconsistency, fixed in galaxyproject/tools-iuc#8255: tools/artic consumed from_data_table="clair3_models"
but declared that table locally as model, shipped tool-data/models.loc.sample
while the config pointed at model.loc, and wired model to
test-data/clair3_models.loc, so its cached-model selector had no local data.

EmptyLocFile — undocumented empty fixtures (galaxyproject/planemo#869)

A warning: a .loc / .loc.sample that is empty and carries no # comment gives
no hint of its expected columns — the remedy is a one-line header, not data, so a
documented-but-dataless fixture passes. Unlike the configured-table linters this
reads a fresh repository-tree walk (model.loc_files) rather than the loader's
resolved loc_assets: an empty .loc.sample never resolves into an asset (the
loader's .sample fallback misses the shed tool-data/ layout), so a walk is the
only way to reach them. Across tools-iuc this flagged 32 header-less files
(14 .loc.sample, 18 test-data/*.loc) out of 660; the corpus cleanup landed in
galaxyproject/tools-iuc#8256, and the 384 comment-only files already passed.

Design decisions

  • ConsumerTableDefined only walks from_data_table references in repositories
    that ship a bundle (a data-manager or tool-data-table config); pure-consumer tool
    repositories are deliberately left uninspected. A pure consumer's table is by
    definition supplied elsewhere, so flagging it as "not defined locally" would warn
    on nearly every tool repo in the shed — noise, not a defect — and loading every
    wrapper only earns its cost when there is a local bundle to validate against. This
    is a sensible boundary for the initial pass; it can be revisited once real usage
    shows whether cross-repo consumer validation (against a supplier index) is wanted.

How to test the changes?

(Select all options that apply)

Unit coverage: test/unit/tool_util/data/test_repository_data_table_lint.py,
test_repository_data_tables.py, and test_data_lint_cli.py, driven by real fixture
repositories under test/unit/tool_util/data/repositories/. The linters were also run
over the full current tools-iuc tree (285 bundle repositories) with no crashes.

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

🤖 Generated with Claude Code

jmchilton and others added 8 commits July 31, 2026 07:47
Step 1 of repository-level data-table linting (galaxyproject/planemo#1672):
assemble producer/config/consumer artifacts of a data-manager repo into one
model for repository-aware linters. Pure assembly, no diagnostics yet.

Composes existing abstractions rather than reinventing:
- manager side via convert_data_tables_xml -> DataTableBundleProcessorDescription
  (output_ref, table names, column maps)
- configured tables + loc resolution + row-error capture via ToolDataTableManager
- macro expansion via get_tool_source

Tests build the model from real fixture repositories (no mocks).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two provable hard-error checks over RepositoryDataTables:
- MissingLocFixture: a configured table references a loc file that
  resolves to nothing (sample fallback still counts as found).
- LocRowShape: a non-comment loc row can't supply every declared
  column index (too-few-fields / wrong-separator), reusing the
  row-shape errors TabularToolDataTable captures at load.

Driven via lint_ctx.lint so each is skippable by name. Fixture-based
tests (no mocks) cover clean / missing / broken / sample-fallback /
skip cases.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cross-component name validation over RepositoryDataTables:
- ManagerTableConfigured: a data manager populates a table that no
  local tool_data_table config (nor known-external table) defines ->
  error (broken producer contract, Planemo galaxyproject#706).
- ConsumerTableDefined: a literal from_data_table reference resolves
  to no local/known-external table -> warn (may be supplied by core
  or another installed repository).

Only literal, fully macro-expanded names are checked; names still
carrying $/@/{} markers are treated as not-checked, guarding the
tools-iuc#5003 false positive where @IDX_DATA_TABLE@ looks
unconfigured but resolves. external_table_names suppresses both.

Fixture tests (no mocks) cover clean bundle, unconfigured manager
table, externally-supplied table, unknown/known consumer, and the
non-literal guard.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
OutputRefValid: a data_manager_conf output_ref that names no output of
the expanded manager wrapper is a hard error, listing the real declared
outputs for the fix. Only runs when the wrapper actually resolved --
ManagerDecl now carries wrapper_resolved so an unparsed wrapper leaves
outputs unknown (not-checked) rather than demonstrably missing.

Fixture test covers a real-output ref (clean), a bad ref (error), and
the unresolved-wrapper guard.

Duplicate-column-name and conflicting-schema detection deferred: a
conflicting same-named table config raises inside
TabularToolDataTable.merge_tool_data_table (the 26.1
assert_data_table_consistency path, outside planemo's <26.1 pin), and
duplicate column names collapse in the parsed columns dict, so both
need model work beyond this slice.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Unlocks the two checks deferred from step 4 via a pre-merge raw view of
each <table> element (RawTableDecl: declared column names kept with
duplicates, separator, comment_char, source), since the parsed columns
dict collapses duplicate names and the loader *raises* when merging
same-named tables with different columns.

- DuplicateColumnNames (error): a <table> declares a column name twice.
- ConflictingTableSchema (error): a table name is declared with
  differing columns/separator/comment across the bundle.

build_repository_data_tables now skips loader enrichment when a column
conflict is present (the loader would crash) and reports it from the raw
declarations instead; table names come from the raw view too, so
cross-component checks stay robust. configured_table_names unions the
loader-enriched and raw declarations.

Fixture tests (no mocks) cover duplicate columns, conflicting columns
(assembly must not crash), conflicting separator, and a clean repo; a
direct-loader check confirms the skip guard is load-bearing.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review-driven cleanups on the repository data-table bundle:

- _raw_column_spec: delegate the name->index map to the canonical
  TabularToolDataTable.parse_column_spec_element (added by 94a027c);
  keep only the ordered-names-with-duplicates list DuplicateColumnNames needs.
- Drop unused TableDecl.instance + RepositoryDataTables.table(); fix the
  stale "reused by downstream" comment (schema fields kept as honest forward
  surface for bundle-completeness).
- Use xml_tree (house standard in the tool linters) via _tool_source_root
  instead of the equivalent root attribute.
- Tie _has_column_conflict's docstring to the canonical loader/merge check.

Add lint_repository_data_tables_bundle(lint_ctx, repo_root, ...paths...): a
discovery-driven entry point that builds the model and lints it, dispatching
the assembly phase through lint_ctx.lint so skip/assembly-failure diagnostics
print. Lets repository linters (Planemo shed_lint) do discovery + one call.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Restore RepositoryDataTables.table(); it is covered by a step-1 model test
  (test_repository_data_tables.py), which the earlier removal missed.
- test_list_linters: 147 -> 154 for the 7 repository data-table linters, and
  import bundles.lint there so the census is deterministic (matches how the
  test already imports every linter module).
- Silence 7 Liskov [override] errors: the bundle linters intentionally target
  a RepositoryDataTables model, not a ToolSource.
- cast the tool_data_table_confs list at the ToolDataTableManager call to work
  around list invariance (str paths vs list[str | PathLike]).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Relocate data-table bundle discovery (find data_manager_conf /
tool_data_table_conf, walk consumer tool sources) out of Planemo into
find_and_lint_repository_data_tables. Add galaxy-tool-data-lint
console_script over it so a data-manager/reference-data repo can be
linted standalone (--skip/--json/--fail-level). CLI tests over fixtures.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jmchilton and others added 6 commits July 31, 2026 07:58
Linter.lint was annotated with ToolSource, so repository data-table linters
(RepositoryDataTables target) tripped mypy [override] and needed per-method
ignores. Framework already dispatches via LintTargetType TypeVar; make Linter
Generic[LintTargetType] to match. Bare Linter subclasses stay Linter[Any];
bundle linters declare Linter[RepositoryDataTables]. 7 ignores removed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Real shed repos ship tool-data/<name>.loc.sample and materialize the real
.loc on install. The loader's own .sample fallback strips the tool-data/
subdir and only looks in tool_data_path root, so it misses these
(found=False) -- MissingLocFixture would then falsely flag nearly every
reference-data repo. Add LocAsset.sample_backed (sibling <ref>.sample or
tool-data/<basename>.sample) and only flag references that are neither
found nor sample-backed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
list_linters() now imports the repository data-table linter module itself
so the registry is populated deterministically wherever it's called (incl.
planemo's --skip name validation), instead of relying on a side-effecting
import in the tool-linter test. Drop that import; count stays 154 (147 tool
+ 7 repository linters). Add a README describing the lint fixture repos.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…efault

find_and_lint seeds external_table_names with the tables Galaxy core / a stock
data manager supply on essentially every deployment (all_fasta, fasta_indexes,
__dbkeys__), so ConsumerTableDefined does not warn on ordinary from_data_table
references to them. Callers can extend the set for repository-specific suppliers.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Warn on a .loc / .loc.sample that is empty and carries no comment
describing the column format. A dataless file with a documenting comment
is the accepted convention (a data manager fills the rows on install), so
only empty-and-undocumented files are flagged; warning, not error.

The check reads a fresh repo-tree walk (model.loc_files), not the resolved
loc_assets: the empty .loc.sample files this targets never resolve into
loc_assets because the loader's .sample fallback misses the shed tool-data
layout. Across tools-iuc this flags 32 files (18 test-data/*.loc, 14
*.loc.sample).

Give the clean fixture's 0-byte test-data/all_fasta.loc a format header so
the canonical happy-path repo passes every linter.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add core_table_consumer fixture (from IUC data_manager_bwa_mem2_index_builder):
defines bwa_mem2_indexes locally, consumes core all_fasta via from_data_table.
find_and_lint seeds all_fasta/fasta_indexes/__dbkeys__ so the core ref doesn't
warn; without the seeding it does (non-vacuous guard).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant