Improve the handling of multiple metric columns in datasets - #274
Improve the handling of multiple metric columns in datasets#274jtuomist wants to merge 8 commits into
Conversation
…tions; delete redundant nodes
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #274 +/- ##
==========================================
- Coverage 48.05% 48.04% -0.01%
==========================================
Files 342 342
Lines 45386 45444 +58
Branches 6794 6808 +14
==========================================
+ Hits 21809 21835 +26
- Misses 21895 21921 +26
- Partials 1682 1688 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![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: e8e6b285a5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| input_dataset: str | None = None | ||
| """DVC dataset identifier override (when different from ``id``).""" | ||
| column: str | None = None | ||
| columns: list[str] | None = None |
There was a problem hiding this comment.
When a configuration supplies columns: [], validation accepts it and to_transform_pipeline() emits an explicit select_columns operation, while _select_dataset_columns() tests the list by truthiness and therefore treats it as if columns were unset, returning the entire dataset. The editor export similarly treats the non-None empty list as explicit and creates no dataset ports, so runtime computation and the persisted graph disagree. Require at least one item or consistently test is not None.
Useful? React with 👍 / 👎.
| raise Exception( | ||
| 'Cannot drop non-existent column(s) %s; available columns: %s' % (', '.join(missing), ', '.join(self.columns)) | ||
| ) |
There was a problem hiding this comment.
For a strict drop of a missing literal column, this now raises a generic Exception before Polars can raise its documented ColumnNotFoundError. Any caller that catches the Polars exception to distinguish a missing column from unrelated failures will no longer handle this case, despite the change being intended to improve only the error message. Raise pl.exceptions.ColumnNotFoundError with the enhanced text instead.
Useful? React with 👍 / 👎.
1 participant
Description
Claude's summary of the two cleanups, ready for review as a separate change from the longmont-dev deploy:
PathsDataFrame.drop() (common/polars.py:180-205) — now pre-validates requested columns against self.columns before calling polars' drop(), raising Cannot drop non-existent column(s) X; available columns: Y, Z instead of polars' generic error. Purely additive (only changes the error message for calls that were already going to fail); tested manually against single/list/missing-column cases.
Explicit multi-column dataset selection — new columns: list[str] field alongside the existing singular column: str, mutually exclusive (validated), across nodes/defs/node_defs.py (InputDatasetDef, DatasetPortSpec, plus a SelectColumnsDatasetTransformOp for the DB-spec pipeline), nodes/datasets.py (DatasetWithFilters, refactored _filter_and_process_df/_select_dataset_columns into a shared helper to keep ruff's complexity check happy), and nodes/spec_export.py (editor port-binding logic). When set, it whitelists those metric columns, drops rows where all of them are null, and drops any dimension column left entirely null afterward — generalizing the "empty dimensions get dropped automatically" behavior that today only applies to the singular column: case (this is exactly the gap that made the blank-Building energy class approach fail for benchmark_energy_use earlier).
Verified: ruff check/ruff format --check/mypy clean on all four touched files, pytest nodes/tests/test_model_editor.py nodes/tests/test_datasets.py nodes/tests/test_dataset_provenance.py nodes/tests/test_compare_dataset.py all pass, and a full pytest nodes/ run shows only pre-existing failures unrelated to this change (test_destructively_trim_db.py, a Django oauth2_provider app-registration error that reproduces in isolation with no code changes at all — an environment issue, not something introduced here).
Screenshots/Videos (if applicable)
Add screenshots or videos demonstrating the changes if applicable.
Related issue
E.g. Link to asana, sentry, slack thread etc.
Requirements, dependencies and related PRs
Describe or link possible requirements, dependencies and related PRs here
Additional Notes
Any additional information that reviewers should know about this PR.
✅ Pre-Merge Checklist
Type of Change
Testing
Manual testing instructions
If feature requires manual testing by reviewer, you can provide instructions here.Internationalization & Accessibility
Dependencies
Screenshots/Videos (if applicable)
Add screenshots or videos demonstrating the changes if applicable.
Additional Notes
Any additional information that reviewers should know about this PR.