Fix NoReturn return type annotations

[jaywonchung]

Thanks for the great library, and also taking the time to add type annotations for the users.

I've found some wrong uses of typing.NoReturn in the library, so I'm sending a PR to fix them. NoReturn return type annotations are for functions that never return a value, for instance those that exit the Python interpreter (by calling sys.exit(1)) or always raise an exception (by ending with raise ValueError). See: https://www.python.org/dev/peps/pep-0484/#the-noreturn-type

All Python functions that do not have a return statement returns None. Hence the correct return type annotation is type(None), which is equivalent to simply None in the context of typing according to the standard. See: https://www.python.org/dev/peps/pep-0484/#using-none

I wasn't able to generate the documentations. I tried pip install sphinx and make -C docsrc github but it errored with:

$ make -C docsrc github
Running Sphinx v4.4.0
WARNING: html_static_path entry '_static' does not exist
WARNING: sphinx_rtd_theme (< 0.3.0) found. It will not be available since Sphinx-6.0

Theme error:
no theme named 'sphinx_rtd_theme' found (missing theme.conf?)
make[1]: *** [html] Error 2
make: *** [github] Error 2

It'll help a lot for future contributors for the maintainers to add docsrc/requirements.txt or add dev dependencies to setup.py. In the meanwhile, please feel free to directly edit this branch if the documentation needs to be generated.

[bkleyn]

Hi @jaywonchung

Thank you for your interest in the library and your effort to contribute to it!

You are indeed correct that a None return would be more appropriate with NoReturn reserved for functions that never return a value. Unfortunately, we’ve used the NoReturn typing in most of our libraries as well as other dependencies. As such we would prefer to tackle this all together at a later point. We’ll keep this PR open in the meantime.

Thank you for making us aware of the error you encountered with generating the documentation. We’ll add a requirements file per your suggestion in the next release.

Regards,

[jaywonchung]

Sure! You can just close this PR and open a related issue. Whatever's more convenient for you. 👍

[bkleyn]

Closing this PR based on discussion.

[skadio]

@jaywonchung while we could not accommodate this PR at this time, I just want to recognize your effort in this direction!

The team discussed this internally and we are on the same page with you and your suggested changes.

What is binding for us is that there are applications built on top of the library and have organically grown into larger systems. As such, a "behavior" change is susceptible to breaking upstream code --which undesirable, and we try to refrain from that.

This was the main consideration that is holding us back from this PR. The point is well taken (and agreed!) and huge appreciation for your effort and for bringing this to our attention.

Serdar

[jaywonchung]

@bkleyn @skadio Hi! Thanks for the kind words.

I entirely agree that breaking changes in a library depended on by many other applications should be done carefully. However, I feel like there might be a potential misunderstanding about type annotations so I'll leave a short comment, really just in case.

As far as I know, python type annotations do not affect behavior at all. Rather, the purpose of adding type annotations is to allow language servers like pyright to provide better developer experience and offline static type checkers like mypy to sanitize potential bugs before running the code. From https://peps.python.org/pep-0484/#abstract:

While these annotations are available at runtime through the usual __annotations__ attribute, no type checking happens at runtime. Instead, the proposal assumes the existence of a separate off-line type checker which users can run over their source code voluntarily.

For instance, the following two pieces of code have identical runtime behavior.

def fit(self, decisions: list[int], rewards: list[float], contexts: list[int] | None) -> NoReturn:
    self._imp.fit(decisions, rewards, contexts)

def fit(self, decisions: list[int], rewards: list[float], contexts: list[int] | None) -> None:
    self._imp.fit(decisions, rewards, contexts)

This comment is to make sure we are on the same page about type annotations. I have no intention to insist on this PR getting merged; I respect the repository owners' collective decision.

[skadio]

@jaywonchung yes, we are on the same page. Completely agree no type checking happens, and the code will be identical. I used the term "behavior" in the loose sense, not to hint the codes behave differently. So we are on the same page ;)