Removing the change of behavior regarding SIGPIPE from the top-level

[jblespiau]

Hi,

this is about the code at https://github.com/mjpost/sacrebleu/blob/master/sacrebleu/sacrebleu.py#L51-L60 which is changing the say SIGPIPE is handled by any binary transitively importing sacrebleu.

1) I have understood it's here to fix "broken pipe" errors when using sacrebley as a command line:

• https://github.com/mozilla/sacreBLEU/commit/13966eadb4d7133330b1a40a02476ac7fe74c311 (I know it's a different repo, but I expect the history to be the same).
• https://stackoverflow.com/a/30091579/891821

2) I don't think the full library should be changing the way SIGPIPE is handled. It's explicitly siad in the Python documentation to not do that. See https://docs.python.org/3/library/signal.html#note-on-sigpipe

What about: e.g.

• moving the code block to main,
• or remove the code block in favor of the method described in https://docs.python.org/3/library/signal.html#note-on-sigpipe, for the `main() function?

Thanks.

[martinpopel]

Thanks for the suggestion. Can you send a PR? The signal(SIGPIPE,SIG_DFL) line was added in 2017 when sacrebleu.py was a single-file command-line utility (even then it could be used as a library, but perhaps it was not the primary use case). I agree that now when the library use case is officially supported, we should move the code block below the if __name__ == '__main__' condition and/or catch specifically the BrokenPipeError exception as suggested in the Python documentation. We don't support Python 2 anymore.

[jblespiau]

In practice, I was working on an older version of sacrebleu, and the code at HEAD is correct, because the Signaling is only done in sacrebleu/sacrebleu.py which is the binary specifically.

So it's already done in a wya I would have like it to be: the fact it's at top-level and not in main is ok, because the file is only used and imported when executing the binary.

Resolving this bug and sorry for the confusion.