-
Notifications
You must be signed in to change notification settings - Fork 125
Remove unused in-place progress logging mode #3811
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
In-place mode was designed to update job progress in place using ANSI escape codes (e.g., showing 'PENDING' → 'RUNNING' → 'TERMINATED' on the same line). However, acceptance tests show jobs typically output only a single state transition: 'Run URL: <url>' followed by '[TIMESTAMP] "job-name" TERMINATED', suggesting the job completes before multiple states can be observed. The default mode selection logic required log-file to not be stderr AND stderr to be a terminal to enable in-place mode, which is an uncommon configuration. Additionally, only JobProgressEvent supported in-place updates while all other events (URLs, errors, pipeline events) fell back to append mode, making the feature inconsistent. The implementation added complexity with ANSI escape codes, terminal detection, and an IsInplaceSupported() interface method across all event types. Since the feature provided minimal practical value and likely was rarely (if ever) enabled by default, it has been removed in favor of the simpler append mode.
11 failing tests:
|
lennartkats-db
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this in this stage is fine. I would like us to revisit having a nicer progress updater, though. Maybe that nicer implementation should take inspiration from what we had here?
|
@lennartkats-db That's the idea. To set the stage for improvements, existing bits need to be cleaned up first. |
denik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay, red diff :)
## Changes The progress logger now only supports `append` mode. ## Why JSON mode was added in #276 (March 2023). The intent was to make progress events machine readable but this didn't materialize (we didn't end up using it in the VS Code extension). This functionality would print events to stderr if the user specified `--progress-format json`, or set the an equivalent environment variable. Because the flag is hidden, and there are no online references to the functionality, I believe it is safe to remove. If users take a dependency on JSON output, it should be enabled via the existing `--output json` flag and be written to stdout. After this change is merged, the remaining functionality can be moved into the core `cmdio` types, and the "progress logger" can be removed. Once there is a single type for all I/O, we have a better shot at improving it. Related change: #3811. ## Tests Tests pass.
## Changes This continues the progress logger simplification by migrating from the `Logger.Log()` event-based system to direct cmdio calls. This builds on #3811 and #3812, which made the progress logger effectively only write strings to stderr. ## Why This simplifies the codebase by removing the (unused) event abstraction layer while maintaining functionality. The compatibility layer provides a clean migration path and will eventually be removed once the functionality in those functions have a dedicated new home. ## Tests Tests pass. Analysis of the diff suggests that before/after is functionally equivalent. A minor note is that we no longer have a mutex around `.Log()` calls, but there are no concurrent calls to the function.
## Release v0.275.0 ### Notable Changes * Python support for Databricks Asset Bundles is now generally available. ### CLI * Remove `inplace` mode for the `--progress-format` flag ([#3811](#3811)) * Remove `json` mode for the `--progress-format` flag ([#3812](#3812)) * Deprecate the `--progress-format` flag ([#3819](#3819)) ### Bundles * Add support for `--bind` flag in `bundle generate` ([#3782](#3782)) * Add `pydabs` template replacing `experimental-jobs-as-code` template ([#3806](#3806)) * You can now use the top-level `python` section instead of `experimental/python` ([#3540](#3540))
Changes
The progress logger now only supports
append(default) andjsonmodes.The
jsonmode is not used anywhere and is up for removal next.Why
In-place mode was added in #276 (March 2023) to update job progress on the same line using ANSI escape codes. The intent was to (eventually) show a rich UI with job status and task status but this didn't materialize. It was only ever implemented for job run status events, where the number of states to cycle through is minimal.
The feature was effectively disabled after commit 54e16d5 (#2213, Feb 2025) changed the default log level from
disabledtowarn. In-place mode required(log.level == "disabled" OR log.file != "stderr") AND stderr.IsTerminal(). With the new default, this is only met whenlog.file != "stderr"(uncommon). Additionally, onlyJobProgressEventsupported in-place updates while other events fell back to append mode.Before (when forcing in-place mode):
After:
Related: the escape codes that were used are not VT100 compliant and don't work in Ghostty.
Tests
Tests pass. Manually confirmed
bundle runworks as expected.