Polish benchmarks CLI error messages#1030
Merged
Merged
Conversation
Apply the recommendations from the benchmarks error-message analysis: quote file paths consistently, end messages with a period, reword action hints to be more direct, and surface the right follow-up command in timeout and failure paths. Notable behavior changes (not pure copy): - The server-side error text appended to a creation-failure message is now shown only with --verbose. - The Task Info dump in the "not ready to run" error is now shown only with --verbose. No new tests: changes are wording-only or guarded by --verbose, both of which are easier to verify by reading the diff than by pinning exact strings.
2 tasks
The Phase 1 polish gated the server-side error text behind --verbose in two paths: - `_poll_task_creation` creation-failed message - `benchmarks_tasks_run_cli` "not ready to run" message That hid useful debugging info from the default path. Undo the gate but keep the newline-prefixed formatting so the appendage still reads as a separate indented sub-line under the main error.
Move the empty-list check up to `benchmarks_tasks_list_cli` so it can
see the filter context. Print:
- "No tasks found matching the given filters." when --name-regex
and/or --status are set,
- "No tasks found. Use 'kaggle b t push' to create one." when no
filters are set.
Previously the unfiltered hint was printed in both cases, which was
misleading after a filtered search.
Also drops the now-unreachable empty-check branch from
`_paginated_task_display` (its only caller is the one above and we
now guard before calling).
Five tests pinned exact wording that this PR rewords in the source. Loosen the regex/substring assertions to match the new phrasing without re-pinning to the exact new string.
The Skipped-run output unconditionally appended a hint to run 'kaggle b t models' after every skip reason, but the backend's RunSkippedReason field is a free-form string that includes a generic ex.Message fallback (Kaggle.Services.Benchmarks/Handlers/ BenchmarkTaskRunService/BatchScheduleBenchmarkTaskRunsHandler.cs L187). Pointing the user at the models list when the real reason is, say, a quota error is misleading. Gate the hint behind a substring match on "model version", which covers all three known model-related reasons: - "Unsupported model version" (api-layer rewrite) - "No matching model version." (service) - "Can not access model version." (service) Also rstrip a trailing period from the reason before wrapping it in parens, so the formatted line doesn't double up the punctuation (`(... model version.).` → `(... model version).`).
dolaameng
reviewed
May 28, 2026
| print(f" {model_slug}: Skipped ({res.run_skipped_reason})") | ||
| reason = (res.run_skipped_reason or "").rstrip(".") | ||
| msg = f" {model_slug}: Skipped ({reason})." | ||
| if "model version" in reason.lower(): |
Contributor
There was a problem hiding this comment.
This is really hardcoded, we should use a proper error message? @nl917
Contributor
Author
There was a problem hiding this comment.
Good call — dropped the substring-matched hint entirely; we now just print the backend's skip reason. Filing the proper fix (structured hint/code from the backend) as a follow-up for @nl917 — the CLI shouldn't be sniffing free-text reasons to decide what hint to show.
| def _paginated_task_display(self, tasks, page_size=20, interactive=True): | ||
| """Display *tasks* one page at a time with an interactive n/p/q prompt.""" | ||
| total = len(tasks) | ||
| if total == 0: |
Contributor
There was a problem hiding this comment.
I think it's ok to move the print message out but now the caller will need to remember to check the total, instead of leave it to _paginated_task_display to print.
Maybe a better way is to add a customized parameter empty_message to _paginated_task_display?
Contributor
Author
There was a problem hiding this comment.
Done — added an empty_message parameter to _paginated_task_display and reverted the caller's pre-check. The list-cli site now just passes its filter-aware message in.
stevemessick
requested changes
May 28, 2026
| raise ValueError( | ||
| f"Task '{task}' not found. Check the task name and try again. " | ||
| f"Use 'kaggle b t list' to see your tasks." | ||
| f"Task '{task}' not found. Verify the name or run 'kaggle b t list' to see available tasks." |
Contributor
There was a problem hiding this comment.
It's better to use full names rather than abbreviations (b, t) in error messages.
Contributor
Author
There was a problem hiding this comment.
Done — expanded kaggle b t ... to kaggle benchmarks tasks ... in all error messages (this site plus two others). Left the success-path print hints alone for now to keep this PR scoped; happy to do a sweep of those in a follow-up if you'd like them expanded too.
- Drop substring-matched 'model version' skip-reason hint; print the backend-provided reason verbatim. Follow-up: have the backend return a structured hint/code rather than a free-text reason. - Move empty-list handling back into _paginated_task_display via an empty_message parameter so callers don't each need to pre-check. - Expand 'kaggle b t ...' to 'kaggle benchmarks tasks ...' in error messages (print hints unchanged).
nl917
reviewed
May 28, 2026
simsryan-google
added a commit
that referenced
this pull request
May 28, 2026
## Summary Phase 2 of the Kaggle benchmarks CLI error-message polish. Five benchmarks messages currently print to stdout but represent warnings or errors. Routing them to stderr keeps stdout clean for piped/parsed output. - `benchmarks tasks delete` — "Delete is not supported by the server yet." - `benchmarks tasks log` — "(No logs available — server returned N)" fallback when a per-run log fetch 404s. - `benchmarks init` — "Example file already exists at '…', skipping." and "Reference file already exists at '…', skipping." (also quoted the path for consistency with Phase 1). - `benchmarks tasks publish` — "Note: No backing notebook is associated with this task." Scoped to benchmarks-only call sites; no shared infrastructure touched. ## Note on overlap with #1030 (Phase 1) I deliberately omitted the two `Timed out…` messages in `_poll_task_creation` / `_poll_runs` — Phase 1 (#1030) already edits those lines, and rerouting them here would conflict. Happy to follow up with a small rebase PR once #1030 lands if reviewers want those routed to stderr too. ## Test plan - [x] Existing test suite passes (`pytest tests/` — 45/45) - [ ] Spot-check one of the affected paths (e.g. `kaggle benchmarks init` in a directory where the example file already exists) renders the warning to stderr
stevemessick
approved these changes
May 28, 2026
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
4 participants
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.
Summary
Test plan