Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows docker image uses pyreadline causing deprecation warnings #511

Closed
dirk-thomas opened this issue Aug 31, 2020 · 11 comments · Fixed by #519
Closed

Windows docker image uses pyreadline causing deprecation warnings #511

dirk-thomas opened this issue Aug 31, 2020 · 11 comments · Fixed by #519
Assignees
Labels
bug Something isn't working

Comments

@dirk-thomas
Copy link
Member

See https://ci.ros2.org/view/All/job/test_ci_windows/198/python-deprecation-warnings/

@brawner
Copy link
Contributor

brawner commented Sep 2, 2020

Digging through the docker image, here is the python dependency tree run with pipdeptree

- colcon-core [required: Any, installed: 0.6.0]
  - coloredlogs [required: Any, installed: 14.0]
     - humanfriendly [required: >=7.1, installed: 8.2]
        - pyreadline [required: Any, installed: 2.1]

@brawner
Copy link
Contributor

brawner commented Sep 2, 2020

Opened issue at pyreadline. Package is likely unmaintained at this point.
pyreadline/pyreadline#65

Opened issue at humanfriendly for conditionally depending on pyreadline for specific python versions.
xolox/python-humanfriendly#44

@brawner
Copy link
Contributor

brawner commented Sep 2, 2020

If this isn't fixed upstream, colcon-core may need to disable coloredlogs for windows with Python >= 3.9

@dirk-thomas dirk-thomas assigned dirk-thomas and unassigned brawner Sep 2, 2020
@dirk-thomas
Copy link
Member Author

I created xolox/python-humanfriendly#45 to avoid the dependency for Python 3.9. If the proposed patch get merged and released we can use the new version of python-humanfriendly and manually uninstall pyreadline while still using Python 3.8 to avoid the deprecation warning.

@jacobperron
Copy link
Member

I'm trying out patching CI as a temporary workaround: #518

@jacobperron
Copy link
Member

I'm trying to use xolox/python-humanfriendly#45 and manually uninstall pyreadline in #518, but it looks like this is not a sufficient solution. I think either pyreadline should be removed completely as a dependency or the deprecation warning selectively silenced. I'll look into doing the latter.

jacobperron added a commit that referenced this issue Oct 5, 2020
Fixes #511

Relevant issue upstream: pyreadline/pyreadline#65

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@dirk-thomas
Copy link
Member Author

I'm trying to use xolox/python-humanfriendly#45 and manually uninstall pyreadline in #518, but it looks like this is not a sufficient solution.

This will only be effective if Python 3.9 is being used. With Python 3.8 the deprecation warning is still around (since effectively the referenced PR doesn't change anything with Python < 3.9).

the deprecation warning selectively silenced.

That sounds reasonable when the Python version is < 3.9.

@jacobperron
Copy link
Member

jacobperron commented Oct 6, 2020

So, we can silence the warning by setting PYTHONWARNINGS='ignore::DeprecationWarning:pyreadline[.*]' (for example), however, this will not fix the rclpy test failures since it is passing the option -We, which has precedence over PYTHONWARNINGS...

I also looked into programmatically ignoring the warning with something like:

with warnings.catch_warnings():
    warnings.filterwarnings("ignore", category=DeprecationWarning, module='pyreadline')

But it is not obvious to me where this kind of warning suppression logic should live.

@jacobperron
Copy link
Member

Here's a couple not-so-great solutions I can think of:

  1. Fork pyreadline and fix the warnings.
  2. Fork python-humanfriendly and suppress the warnings.

@clalancette
Copy link
Contributor

1. Fork pyreadline and fix the warnings.

It seems to me that we should just do this. It's not ideal, but given that there have been no commits to that repository for 5 years, I think we'll have to do it. The question becomes how we convince pip to install our custom version, but there must be a way to do this.

@jacobperron
Copy link
Member

Installing a fork of pyreadline fixes the deprecation warnings (and test failures in rclpy): #519

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants