Skip to content
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

progress: add option to remove spinner & steady tick #12

Closed
utkarshgupta137 opened this issue Jan 31, 2024 · 10 comments
Closed

progress: add option to remove spinner & steady tick #12

utkarshgupta137 opened this issue Jan 31, 2024 · 10 comments

Comments

@utkarshgupta137
Copy link
Contributor

We use rust-parallel in an airflow job. Because of the way airflow saves the logs of the jobs, it creates a new line every time the spinner is updated, as it doesn't support ANSI I think. This results in a lot of spam, since this crate has a steady tick of 100ms, we get 2.1M useless log lines, which not only makes the logs useless, it also increases the log storage costs.
I've a fork where I've removed both of these, but I would like to raise a PR with an option to disable this at runtime either via a CLI arg or environment variable (such as TERM or NO_COLOR).
What kind of an interface should we have for this.

@aaronriekenberg
Copy link
Owner

aaronriekenberg commented Feb 1, 2024

I agree the progress bar can be more configurable than current hard-codings, I think at least we want these options:

  1. Ability to disable steady tick
  2. Ability to remove spinner from PROGRESS_STYLE

We may even want to make the entire PROGRESS_STYLE configurable as there are may options that Indicatif suppports.

I like using CLI arguments for above options. Would be happy to review a PR @utkarshgupta137

Thanks!

@aaronriekenberg
Copy link
Owner

aaronriekenberg commented Feb 11, 2024

I am thinking we can add these 2 command line options easily:

--progress-bar-disable-steady-tick
--progress-bar-disable-spinner

@utkarshgupta137 please let me know if you would like to raise a PR for above, if not I am happy to make this change also.

thanks!

@utkarshgupta137
Copy link
Contributor Author

I would personally prefer an environment variable for this, otherwise it would crowd out the command line too much. I feel like this is something that you either always want on or always want off & not toggle it on a per invocation basis. Can have both too.
FYI, there is precedent for this kind of stuff being toggled on or off using environment variables, for example NO_COLOR or CI environment variables.

@aaronriekenberg
Copy link
Owner

I pushed a commit here: 24b8257

This disables the steady tick and spinner if stderr is not a terminal, using the builtin std::io::stderr().is_terminal()

@utkarshgupta137 can you please test if this behaves well in your airflow job example?

thanks!

@utkarshgupta137
Copy link
Contributor Author

No, doesn't seem to be working. This could partly be because I use ssh command with a pseudo-terminal enabled, so that tasks get killed when I kill them from airflow.

@aaronriekenberg
Copy link
Owner

aaronriekenberg commented Feb 25, 2024

Thanks for testing @utkarshgupta137

One question to understand - is this the way you are running your jobs?

airflow -> ssh -t (force pseudo-terminal) -> rust-parallel (execute commands in parallel)

@utkarshgupta137
Copy link
Contributor Author

Thanks for testing @utkarshgupta137

One question to understand - is this the way you are running your jobs?

airflow -> ssh -t (force pseudo-terminal) -> rust-parallel (execute commands in parallel)

Yeah, it uses get_pty arg for paramiko.

aaronriekenberg added a commit that referenced this issue Feb 29, 2024
… a terminal."

This reverts commit 24b8257.

Did not fix this issue: #12 (comment)
@aaronriekenberg
Copy link
Owner

Learned that tracing subscriber (which rust-parallel uses for logging) uses NO_COLOR environment variable to decide if terminal supports ANSI. This is a useful thing on its own, if NO_COLOR=1 environment variable is set, rust-parallel logging will not use ANSI color sequences.

References:

  1. fmt subscriber should honor NO_COLOR by default tokio-rs/tracing#2388
  2. fmt::Subscriber NO_COLOR support tokio-rs/tracing#2647

Pushed a commit to make the progress bar check if NO_COLOR environment variable is set to non-empty value. If yes then we decide the terminal does not support ANSI, so disable steady tick and spinner in progress bar: 10d6019

Example usage:

$ export NO_COLOR=1
$ rust-parallel -d all -p sleep ::: 1 2 3

OR

$ NO_COLOR=1 rust-parallel -d all -p sleep ::: 1 2 3

@utkarshgupta137 can you please test this? Thanks!

@aaronriekenberg
Copy link
Owner

aaronriekenberg commented Mar 9, 2024

In v1.17.0 added PROGRESS_STYLE environment variable to allow for user-configurable progress styles.

See progress bar manual section for more details.

@utkarshgupta137 I think PROGRESS_STYLE=simple should work well for your jobs - this will disable spinner and steady tick.

Thanks!

@utkarshgupta137
Copy link
Contributor Author

Thank you, it is indeed working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants