feat(lint): accept model file paths in the lint command - #6053
tripleaceme wants to merge 8 commits into
Conversation
`sqlmesh lint` could only select models by name with `--model`, so path-based tooling such as pre-commit — which passes the names of the changed files — could not drive it without a wrapper that mapped paths back to model names. Add a positional `PATHS` argument to `sqlmesh lint`, mirroring the shape of `sqlmesh format`. Each path is resolved to the model(s) defined in that file, and paths can be combined with `--model`; a model selected both ways is linted once. Linting with no selection still lints every model. Model file paths are plumbed into the load path alongside `model_fqns`, so `--use-project-index` scopes a path-based selection the same way it scopes `--model`: only the selected models and their transitive upstream dependencies are loaded, resolved and validated. A path that defines no models is rejected with a clear error rather than silently falling back to linting the whole project. Closes SQLMesh#6021 Signed-off-by: Adegbite Ayoade <tripleaceme@gmail.com>
|
Two things worth a look before merge: 1. Perf: the indexed path lookup does a filesystem call per model, not per selection In selected.update(
fqn for fqn, path in model_to_path.items() if path.resolve() in model_paths
)
Suggest normalizing the index side the same cheap way the paths are already built ( 2. Docs: the added description text doesn't match
|
Matching a selected path walked every entry in the index and called path.resolve() on each one, so selecting a single file cost one filesystem call per model in the project — the opposite of what --use-project-index is for. Resolve the project root once and key the index by that instead, turning the match into a dict lookup per path the user actually asked for. A model file that is itself a symlink is no longer covered by joining onto the resolved root, so the previous resolve-based scan is kept as a fallback for paths left unmatched. Also move the path-selection sentence into the lint docstring so that it is real --help output, and mirror it in the CLI reference rather than documenting text the command never prints. Signed-off-by: Adegbite Ayoade <tripleaceme@gmail.com>
|
Thanks, both good catches — fixed in 27057ae. 1. Path lookup You're right, and it was worse than a normalisation mismatch: the index side was already being built lexically ( Now the project root is resolved once and the index is keyed by One thing worth flagging, since it wasn't only a perf change: the old Added two tests: one asserting no 2. Docs Agreed the page should stay an accurate mirror. It's now a second paragraph on the I left the rest of that block alone, though while I was in there I noticed the page's |
Signed-off-by: Adegbite Ayoade <tripleaceme@gmail.com>
|
@mday-io — gentle nudge on this one, since it's been sitting since the 11th. Both points from your review are addressed in 27057ae:
Two questions from my reply are still open, and they're the only things that would change the diff further:
Branch is merged up with main as of the 13th. Still showing zero checks while the workflow run waits on approval. |
|
Hey there. Three days - especially over the weekend - likely won't be enough time for pull requests or issues to work their way through the pipeline. This is not a full-time maintained project. If you have a truly urgent problem that is disrupting your organization then feel free to call it out, otherwise it could take one to many weeks to get a PR reviewed (especially more than one time). |
Signed-off-by: Adegbite Ayoade <tripleaceme@gmail.com>
|
Thanks for the feedback @mday-io |
Signed-off-by: Adegbite Ayoade <tripleaceme@gmail.com>
|
Thanks for this, and for the pre-commit example in the docs. That makes the use case clear. A question to check we're solving the same problem: is the main goal pre-commit, i.e. tools that pass the changed file names to sqlmesh lint? If so, positional paths are the right interface, and I'm happy to take this approach once the items below are addressed. For the other use case, linting only the models changed in CI, we'd rather reuse the existing model selector (git:main, tag:, + for upstream/downstream, etc.) that plan, run and dag already support. lint doesn't accept --select-model today, so we're opening a separate issue for that: . It doesn't need to be part of this PR. I've suggested a small docs tweak so the two options are positioned clearly. Blocking: the flaky test assertion (inline comment on |
|
The three "Merge branch 'main' into lint-model-paths" commits ( |
The two assertions on the linted file name compared against raw console output. The console wraps at 80 columns, so a long enough temporary path splits the name across lines and the assertion fails. Under xdist the path gains a popen-gwN segment, which is what makes it reproducible there. Reproduced directly: a path of the right length renders the line as "models/seed_model.sq\nl:". Both assertions now compare against the output with the wrapping removed. A directory now says so instead of reporting that no models were found in it, which read as though the directory were empty. The docstring records that relative paths resolve against the current working directory rather than the project path. That is what path-based tools pass, but it means the Python API only accepts relative paths when called from inside the project. The linter guide now frames path selection as the option for tools that hand over file names, and points CI use towards a model selector instead of paths from git diff, so that the models affected by a change are linted rather than only the files that were edited. Signed-off-by: Adegbite Ayoade <tripleaceme@gmail.com>
e2e34b1 to
8cd5ca5
Compare
The relative-path cases ran from the project directory, so they could not tell "resolved against the current working directory" apart from "resolved against the project path". Both now run from a directory that is not the project and assert the path is not found there, then repeat from the project. Checked against the alternative: resolving relative paths against self.path now fails both, where before it passed everything. Moves the "file exists but defines no models" case to the Python API test, which owns that error, rather than leaving the CLI as its only coverage. Drops two tests. One asserted that no .sql path is resolved while matching off the index, by patching pathlib.Path.resolve globally and counting calls; it would break on a behaviour-preserving refactor and pass on a slowdown that stats paths another way. The other duplicated a case already covered earlier in the indexed test, differing only in a private kwarg being None. Also corrects a comment: an unknown path is rejected by the is_file() check before the index is consulted, not off the index. Signed-off-by: Adegbite Ayoade <tripleaceme@gmail.com>
|
Thanks @mday-io — all of it addressed across 8cd5ca5 and ecf41ac, and the three merge commits are signed off now. On the goal: yes, pre-commit is the target. Tools that hand over changed file names, plus editor hooks. I agree the CI case belongs with the model selector rather than paths from The flaky assertion. Reproduced it directly rather than taking it on trust: at the right path length the console renders the line as so The relative-path blind spot — this was the useful one. You were right, and it was worse than a gap in coverage: the case changed directory to the project path, so it asserted nothing about which of the two resolutions was in use. Both the API and CLI cases now run from a directory that is not the project, assert the path is not found there, then repeat from the project. I checked against your exact alternative — resolving relative paths against The two tests you flagged are gone. The Moved the "file exists but defines no models" case to the Python API test, so the test that owns the error in Directories now say so: The docstring records that relative paths resolve against the current working directory rather than the project path, and what that means from the Python API. On DCO: the three merge commits were authored as "Adegbite Ayoade Abel" with no sign-off. Rewritten with the sign-off and the name matching the rest, and I verified the tree is byte-identical to before the rewrite. |
|
|
||
| Models can be selected by name with `--model`, by model file path, or by both at once. | ||
|
|
||
| Selecting by path is intended for tools that hand `sqlmesh lint` a list of file names, such as |
There was a problem hiding this comment.
This reads better. One problem with the new sentence: lint doesn't support --select-model yet, so readers can't actually use git:main with it today. Also, git:main selects the models whose files changed (the same set as the paths), not the downstream "affected" models; that would be git:main+.
Could we trim it to just the first part until selector support lands? Something like:
Selecting by path is intended for tools that hand
sqlmesh linta list of file names, such as pre-commit hooks and editor integrations.
We can add the CI guidance when lint --select-model is implemented.
mday-io
left a comment
There was a problem hiding this comment.
Thanks for signing off the earlier merge commits. The latest one, 74c65e2 ("Merge branch 'main' into lint-model-paths"), still doesn't have a Signed-off-by line, and it's authored as "Adegbite Ayoade Abel". Could you sign that one off too?
Description
Closes #6021.
sqlmesh lintcould only select models by name with--model, so path-based tooling such as pre-commit — which passes the names of the changed files — could not drive it without a wrapper that mapped paths back to model names.This adds a positional
PATHSargument tosqlmesh lint, mirroring the shape ofsqlmesh format:Behaviour:
--model. A model selected both ways is linted once.--modelstill lints every model, unchanged.--localand--use-project-indexkeep working when the selection comes from paths.Implementation note
--use-project-indexneeds the paths before models are parsed, so that only the selected files and their upstream dependencies are loaded. The persistent index can only be read from insideLoader.load()(_model_index_id()depends on the macro/signal/audit mtimes that the load tracks), so rather than resolving paths up front, the selected paths are plumbed throughContext.load→Loader.load→_load_modelsalongside the existingmodel_fqns.SqlMeshLoader._selected_model_pathsthen seeds its selection from both the requested FQNs and the requested paths, and the existing upstream-closure and stale-index fallbacks apply unchanged.The final model set is always resolved from the loaded models via
Model._path, which is what produces the "no models at this path" error and keeps the multi-project case (oneLoaderper--paths) correct.Test Plan
New tests:
tests/core/test_context.py::test_lint_models_by_path— a path selects only the models in that file; unrelated violations are not reported; Python model files work; relative paths resolve against the cwd; paths and--modelcombine and de-duplicate; an unknown path raises; error-level violations still raise.tests/core/test_context.py::test_lint_models_by_path_with_project_index— asserts_load_sql_modelsis called withselected_paths == {a.sql, b.sql}for a path-selected model with one upstream, and that an unknown path is rejected before any models are loaded.tests/core/test_context.py::test_lint_models_by_path_without_index_falls_back_to_full_load— a missing index falls back to a full load.tests/cli/test_cli.py::test_lint_paths,test_lint_relative_path,test_lint_unknown_path— CLI coverage for single/multiple paths, paths plus--model, relative paths,--local/--use-project-index, and both unknown-path cases.tests/cli/test_cli.py::test_lint_model_scopes_validation_with_multiple_projects— extended to cover selecting the same model by path in a multi-project context.Also verified by hand on a fresh
sqlmesh init duckdbproject withrules: "ALL"and an addedSELECT *model: linting a single file reports only that model, linting with no arguments still reports all three, an unknown path and an audit file both error out, and--use-project-indexproduces the same result on the run that builds the index and on the run that reads it.make fast-testpasses (2619 passed). The 5 pre-existing failures intests/utils/test_git_client.pyandtest_expand_git_selection_integrationreproduce identically on an unmodified checkout in my environment and are unrelated to this change.Checklist
make styleand fixed any issuesmake fast-test)git commit -s) per the DCO