brentyi / tyro

CLI interfaces & config objects, from types
https://brentyi.github.io/tyro
MIT License
467 stars 23 forks source link

Argument Errors (rich panel) to be printed on stderr instead of stdout #141

Closed andreicozma1 closed 4 months ago

andreicozma1 commented 4 months ago

In parallelized or distributed runs, it's fairly common to separate logs from stdout and stderr or disable logging stdout altogether.

However, Tyro seems to print argument errors (such as "Unrecognized arguments") on stdout rather than stderr, which feels counterintuitive by default and results in useful errors being omitted from stderr logs under such use cases described above.

I suggest the intuitive stream for printing errors such as this should be stderr by default. Alternatively, a toggle could be available for this to change it seamlessly when tyro is initialized.

brentyi commented 4 months ago

Hi Andrei, thanks for filing this issue! Yes, printing these things to stderr is standard and I'll fix this.

On the topic of distributed / parallel runs, would an argument for toggling console outputs entirely be useful for you? For parallelization with multiple processes via torchrun, accelerate, deepspeed, etc, helptext and parsing errors are currently printed from every single process.

I'm considering an API like this:

# w/ Pytorch DDP
tyro.cli(Config, enable_console_outputs=(rank == 0))

# w/ HF Accelerate
tyro.cli(Config, enable_console_outputs=accelerator.is_main_process)

Does this make sense to you?

andreicozma1 commented 4 months ago

Thank you for the quick reply. Regarding the functionality for distributed / parallel runs, I think it would certainly be helpful for uncluttering log outputs even further, given that each worker process would just duplicate the same error/help message.

The API suggested seems straight-forward. One potential alternative could be the following:

# Manually setting the stream (preferably defaulting to stderr)
tyro.cli(Config, console_output_stream=sys.stderr)

where console_stream could be something like type FileDescriptorOrPath

Maybe this syntax could keep tyro.cli's arguments general and flexible enough for any use-case.

For example, extending it to the HF accelerate example would be as simple as:

tyro.cli(Config, console_output_stream=sys.stderr if accelerator.is_main_process else os.devnull)

Let me know your thoughts on this.

Thanks!

brentyi commented 4 months ago

Makes sense, I appreciate the feedback!

After looking into it a bit, the enable_console_outputs= argument makes more sense to me. It turns out that argparse uses stdout for helptext and stderr for error messages; I'd like to make things consistent with this and it seems cumbersome to do with an arg like console_output_stream= (or args like console_error_stream= / console_helptext_stream=).

If configuring specific streams turns out to be critical we can add more args later. Or, folks can wrap tyro.cli() with contextlib.redirect_stdout / contextlib.redirect_stderr.