Repository: huggingface/tokenizers (language: rust)

Issue:
Print warnings to stderr, not stdout
I have a python script which reads some data from stdin and then outputs to stdout. However, my output is getting garbled (and therefore following steps in my pipeline break), because this library is outputting warnings to stdout as well. Specifically, I was getting [this warning](https://github.com/huggingface/transformers/issues/5486). I managed to solve this problem by setting TOKENIZERS_PARALLELISM as suggested, but I think in general that this problem should be resolved another way:

Can we adjust this library to print warnings (and maybe other diagnostic/logging info) to stderr instead of stdout? I believe that this is more in line with what stdout and stderr are intended for. Not sure what consequences this would have, I haven't read this code in detail and don't know how users use it.

I could submit a PR. Thanks for the consideration and your work!


Discussion / hints:
Well, things as critical as potential deadlocks, or more recently potential holes/corrupted tokenizers are so critical that we don't mind putting them in `stdout`. The other solution is to purely panic which isn't great either (especially since under some circumstances we can recover, but not others, and we cannot to the best of our  knowledge know if we will recover or not, so we yell, and attempt to recover).
The problem about not being vocal in this library, is that users would just see their program hanging if it hits the deadlock, and unable to progress our understand what's happening. If `stderr` is discarded you wouldn't see anything (and that happens, sometimes unknowingly).

That being said, aside from those CRITICAL sections, which should never be encountered in regular usage, we shouldn't be ever printing (either to `stdout` or `stderr`) unless you use `TOKENIZERS_LOG` env variable to activate our logging system.

We're not closed to making evolutions to printing and logging, but this is why it's currently done that way.

Small note:  Relying on `stdin`, `stdout` for doing any business logic is asking for trouble IMO, the logic should be in the python itself as much as possible. Libraries printing information is wether we like it or not, extremely common.
Thanks @Narsil for the explanation. OK, without anyone else besides me thinking this is a problem, then this probably isn't worth it. If others find this to be a problem, they can comment here.

I can see how `stderr` could get accidentally discarded, but in the same way `stdout` can get discarded too. Do you think people are more likely to accidentally discard `stderr` than they are to accidentally discard `stdout`? My gut instinct would say that this would be slightly more likely, but not hugely so.
> I can see how stderr could get accidentally discarded, but in the same way stdout can get discarded too. Do you think people are more likely to accidentally discard stderr than they are to accidentally discard stdout? My gut instinct would say that this would be slightly more likely, but not hugely so.

Yes I see what you're coming from, I think it's doable to switch to `stderr`, but it's worrying enough that we definitely need to catch user's attention on this.

Can you confirm, that you do not have these warnings anymore on regular usage (using `TOKENIZERS_PARALLELISM=0`) ?
This is the most important way to fix it I think. This library should be quiet otherwise.
> Can you confirm, that you do not have these warnings anymore on regular usage (using `TOKENIZERS_PARALLELISM=0`) ?

I set `TOKENIZERS_PARALLELISM=true` and indeed this library was quiet after that (assuming from reading source code that setting to`0` would do the same). So I'm not blocked by this issue at all.

How about this library just is careful to do the same thing: if any other warnings are ever printed to `stdout`, there is always a way to configure it to make it be quiet. (Of course, anything printed due to `TOKENIZERS_LOG` being set don't need this silencing option.) I am just worried that there might be some other instances (that I just haven't run into, but might later, or due to a change in this library) where this library might emit to `stdout`. But this explicit policy would make that solvable.

Closing, since there isn't really an action I'm looking for at this point, just wanted to get this use case on your radar. Thank you so much for your work!
Hi, apologies for commenting an on old issue but came here for the same reason: this warning message is interfering with some programs that use stdout for data output.

I'm afraid I don't really understand the argument for keeping the message in stdout:

> Well, things as critical as potential deadlocks, or more recently potential holes/corrupted tokenizers are so critical that we don't mind putting them in stdout. 
> ...
> Yes I see what you're coming from, I think it's doable to switch to stderr, but it's worrying enough that we definitely need to catch user's attention on this.

If you're piping / redirecting program output (i.e. stdout), you actually won't see this message the way you would if it went to stderr (until and unless you look at your data 😱). And in a Jupyter notebook, stderr gets highlighted in red so IMO it's actually the more salient stream. Conversely, I don't know of any normal scenarios where stderr is _less_ likely to catch the user's attention than stdout.

(Also, FWIW, in my experience the program doesn't deadlock, rather it just appears to single-thread the tokenization. The warning is annoying but registers more as a heads up that things might run slower than expected and in some circumstances I'd be inclined to just deal with it rather than stop things to set the environment variable. If there are actual risks of corrupted data (I'm not sure what "holes" mean here) it might be good to say that explicitly?)

Fix patch:
diff --git a/bindings/python/src/lib.rs b/bindings/python/src/lib.rs
index bf1adbdff..7bac5f90a 100644
--- a/bindings/python/src/lib.rs
+++ b/bindings/python/src/lib.rs
@@ -29,12 +29,12 @@ static mut REGISTERED_FORK_CALLBACK: bool = false;
 extern "C" fn child_after_fork() {
     use tk::parallelism::*;
     if has_parallelism_been_used() && !is_parallelism_configured() {
-        println!(
+        eprintln!(
             "huggingface/tokenizers: The current process just got forked, after parallelism has \
             already been used. Disabling parallelism to avoid deadlocks..."
         );
-        println!("To disable this warning, you can either:");
-        println!(
+        eprintln!("To disable this warning, you can either:");
+        eprintln!(
             "\t- Avoid using `tokenizers` before the fork if possible\n\
             \t- Explicitly set the environment variable {}=(true | false)",
             ENV_VARIABLE


Test patch: