minerllabs / minerl

MineRL Competition for Sample Efficient Reinforcement Learning - Python Package
http://minerl.io/docs/
Other
711 stars 153 forks source link

Require `typing` for old Python versions only #602

Closed ntoxeg closed 2 years ago

ntoxeg commented 3 years ago

As per the description of the typing package (https://pypi.org/project/typing/):

For package maintainers, it is preferred to use typing;python_version<"3.5" if your package requires it to support earlier Python versions. This will avoid shadowing the stdlib typing module when your package is installed via pip install -t . on Python 3.5 or later.

In some cases the shadowing can occur and it results in errors like this:

Traceback (most recent call last):
  File "/usr/lib/python3.10/runpy.py", line 187, in _run_module_as_main
    mod_name, mod_spec, code = _get_module_details(mod_name, _Error)
  File "/usr/lib/python3.10/runpy.py", line 146, in _get_module_details
    return _get_module_details(pkg_main_name, error)
  File "/usr/lib/python3.10/runpy.py", line 110, in _get_module_details
    __import__(pkg_name)
  File "/home/opencog/.local/lib/python3.10/site-packages/pip/__init__.py", line 1, in <module>
    from typing import List, Optional
  File "/home/opencog/.local/lib/python3.10/site-packages/typing.py", line 1359, in <module>
    class Callable(extra=collections_abc.Callable, metaclass=CallableMeta):
  File "/home/opencog/.local/lib/python3.10/site-packages/typing.py", line 1007, in __new__
    self._abc_registry = extra._abc_registry
AttributeError: type object 'Callable' has no attribute ‘_abc_registry'

And it’s very serious because it prevents you from using pip itself.

Of course, I conditioned the requirement on the actual version you need.

Miffyli commented 3 years ago

Good catch! I was first going to mention that this seems minor, but if it can really break pip, then ooooh boy that is nasty.

How can I quickly test this? I assume I should try installing MineRL on Python version older than 3.6.6, judging by the commit?

ntoxeg commented 3 years ago

@Miffyli if you want to reproduce my issue, it’s a bit contrived:

  1. Let’s assume you do some user installs with pip, including minerl.
  2. Create and switch to other user.
  3. Now you want to reuse whatever is installed by that user (like, maybe you are using Github Actions and wanting to access that).
  4. Add a path like /home/<user>/.local/lib/python<ver>/site-packages to PYTHONPATH.
  5. Enjoy not being able to do anything anymore with pip...

The problem is that PYTHONPATH adds paths to sys.path before stdlib. That means that typing will shadow the standard library.

Even more confusingly you can reproduce this with a new Python version. In the above error I am on Python 3.10… The weird thing is that installing typing through pip only installs something like typing-3.7.4.3. I don’t really want to spend time figuring out why that is…

The conditional requirement I added will not break anything — if you are on Python >= 3.6.6 then you have everything in stdlib anyway (typing is just a back-port). Thus, it’s safe to limit this requirement only to those that actually have an older Python version. It will spare some people with unusual setups a lot of work.

Miffyli commented 2 years ago

Hey! Sorry for taking so long on this (just returned from the vacations). Things look good but I have but one question: the recommendation is to do <=3.5, but you have 3.6.6. Could you quickly explain this?

Sidenote: MineRL is not specifically made to support older Pythons (e.g. tests have format-strings), but this is still nice to have to avoid breaking things horribly 😅

ntoxeg commented 2 years ago

@Miffyli because you install typing>=3.6.6, in other words you require that the typing module is at least from the Python version of 3.6.6. So I added a condition of installing it iff actual Python version is less than that, because if you have at least 3.6.6, then installing typing does nothing.

If you have Python, say, 3.5 (or even 3.6.0), it is not certain that not having typing will work — there might be some changes in the later versions. Ergo, the correct rule in general is to write typing>=X; python_version<“X”

Miffyli commented 2 years ago

Ah right, I learned something new today! Thank you for the explanation :). LGTM and merging.