Fix distutils.LooseVersion DeprecationWarning #88524

Jasha10 · 2022-11-04T22:35:26Z

Fixes #84712

pytorch-bot · 2022-11-04T22:35:29Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/88524

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 0dd19a1:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

Jasha10 · 2022-11-05T03:21:48Z

@milutter, I tried opening this PR based on the diff from your comment on issue #84712.
I'm seeing some test failures: module 'packaging' has no attribute 'version'

Traceback (most recent call last):
  File "test_monitor.py", line 104, in setUp
    from torch.utils.tensorboard import SummaryWriter
  File "/opt/conda/lib/python3.7/site-packages/torch/utils/tensorboard/__init__.py", line 4, in <module>
    if not hasattr(tensorboard, "__version__") or packaging.version.parse(
AttributeError: module 'packaging' has no attribute 'version'

(here's an example CI job failure: https://github.com/pytorch/pytorch/actions/runs/3397514945/jobs/5649878533#step:10:3456)

MaKaNu · 2022-11-08T09:30:15Z

the module version needs to be imported explicitly. The Devs from "neutron" had a similar issue.

Simply

from packaging import version

...
if not hasattr(tensorboard, "__version__") or version.parse(
        tensorboard.__version__
) < version.Version("1.15"):
...

could fix this.

torch/utils/tensorboard/__init__.py

Jasha10 · 2022-11-08T20:27:57Z

Thanks @MaKaNu! I've updated the PR based on your comments.

milutter · 2022-11-09T01:46:53Z

Thanks everybody for the effort! The explicit import of version, is a bit weird but seems to work. Is the current problem that packaging is not listed as a dependency in https://github.com/pytorch/pytorch/blob/master/requirements.txt?

Can we try adding it to the requirements? It's a bit hard to debug without knowing specifically why is running where.

Jasha10 · 2022-11-09T04:59:55Z

Thanks for the tip, @milutter! I've added packaging to requirements.txt.

MaKaNu · 2022-11-09T09:20:31Z

@milutter I wondered what may caused the necessity to explicit import version. I think the AttributeError gives a good hint:

module 'packaging' has not attribute 'version'

but which module is meant? Packaging is a package which next to other modules the __init__.py and version.py includes. So if we import packaging only we don't import the package instead we import the package as module, which results in the __init__.py. Since the module __init__ only has the choosen attributes of the module __about__.py and the attribute __all__, it does not know anything about the module version.py.

If we instead call from packaging import version , we reference now on the package directly.

milutter · 2022-11-15T18:19:18Z

Thanks @MaKaNu for the explanation.

I looked a bit deeper into resolving the problem. I still believe this is a requirements problem such that MYPY does not find the types stubs. Adding it to requirements.txt is false because this requirements.txt is only for development. I personally could not find where the dependencies need to be added.

To get it past the pylint one could try the really hacky version:

if not hasattr(tensorboard, "__version__") or tuple(int(x) for x in tensorboard.__version__.split("."))[:2] < (1, 15):

Jasha10 · 2022-11-16T20:11:28Z

To get it past the pylint one could try the really hacky version:

Hopefully we can get eyes on this PR from someone who knows how the lints work.

MaKaNu · 2022-11-16T21:43:16Z

I ran a small test env and used the package as torch used it. Worked fine and ran mypy manuall over it -> No Errors. I am not completly sure about it, but I will find it again, but this might be a wrong configured mypy setup: I don't have the link at the moment, but will search it, but I have read about stubs and things for builtin packages. If stubs are absolutley necessary aswell for builtin, we need to add them. Stay tuned for further informations tomorrow.

MaKaNu · 2022-11-21T11:48:32Z

I looked into it again and found the conversations, but I do not understand completely: python/mypy#638. On the other hand I found another issue python/mypy#4542, which is pointing to the issue, which also mention PEP 561 as a solution. I did not had the time to dig deeper into this, but this might help to find the issue.

milutter · 2022-12-07T02:50:36Z

I looked at the links from @MaKaNu and they seem to be related. The different search path between python and MyPy seem like a big mess. Even when reading the complete posts, I do not fully understand the problem or see a possible solution. From other blogpost, I have seen two hacks that might mitigate the problem (https://stackoverflow.com/questions/68695851/mypy-cannot-find-implementation-or-library-stub-for-module):

Import the complete function, i.e., from packaging.version import parse
Tag line with # type: ignore to disable MyPY for this line.

@Jasha10 Would you be able to try these different solutions?

Jasha10 · 2022-12-09T13:48:09Z

Looks like the # type: ignore comment worked, the CI is green now.
I tried the other approach (from packaging.version import parse) in commit 92d684f, which led to CI failure.

milutter

Thanks for trying and fixing it. I am happy to get this finally resolved.

Do you know the process of actually merging this PR? Does somebody within PyTorch need to approve & merge the PR. How does one get their attention for this?

MaKaNu · 2022-12-09T14:19:29Z

I don't know but hope, that now as every checks have passed an authorized user get informed about it. At the end I still wonder why mypy even has a problem with the builtin lib. I think something is not correctly configured for mypy, since a non configured mypy install does not react on this.

MaKaNu · 2022-12-09T14:21:18Z

torch/utils/tensorboard/__init__.py

@@ -1,12 +1,12 @@
 import tensorboard
-from distutils.version import LooseVersion
+from packaging import version  # type: ignore[import]


still think the ignore solves the mypy issue but should not.

I don't know why I have looked into, but there are actually 3 mypy.ini configs. I understand why we use mypy.ini and mypy-strict.ini. But the uncommented mypy-nofollow.ini? From the commit message I got a hint.

Which of them is used in this pipeline? I don't know but it should be the normal mypy.ini or mypy-nofollow.ini but not the strict one.

It might be the mypy-nofollow.ini since there are less ignore_missing_imports = True.

Jasha10 · 2022-12-09T20:40:41Z

At the end I still wonder why mypy even has a problem with the builtin lib.

FYI @MaKaNu, packaging is not built-in:

$ python -q
>>> import packaging
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'packaging'

MaKaNu · 2022-12-12T10:29:02Z

FYI @MaKaNu, packaging is not built-in

this is weird and you are right. Somehow on my wsl installation packaging is installed without environment.

milutter · 2022-12-22T03:34:07Z

@Jasha10 I just saw that you are part of the PyTorch organization, would you be able to get some movement into this PR, such that it is reviewed and accepted?

< 67E6 div data-view-component="true" class="comment-reactions js-reactions-container js-reaction-buttons-container social-reactions reactions-container d-none">

Jasha10 · 2022-12-24T11:15:27Z

@milutter I'm honored to be a part of the pytorch org, but TBH I don't know why I'm part of the pytorch org. (I was added to it randomly 🤷🏼)

soumith · 2022-12-26T13:19:47Z

@pytorchbot merge

pytorchmergebot · 2022-12-26T13:21:36Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

pytorchmergebot · 2022-12-26T13:21:36Z

Merge failed

Reason: This PR is too stale; the last push date was more than 3 days ago. Please rebase and try again. You can rebase by leaving the following comment on this PR:
@pytorchbot rebase

Details for Dev Infra team

Raised by workflow job

milutter · 2022-12-26T13:55:08Z

@pytorchbot rebase

pytorch-bot · 2022-12-26T13:55:10Z

You don't have permissions to rebase this PR, only people with write permissions may rebase PRs.

pytorchmergebot · 2022-12-26T23:10:24Z

Successfully rebased patch-2 onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout patch-2 && git pull --rebase)

soumith · 2022-12-27T00:45:36Z

@pytorchbot merge

pytorchmergebot · 2022-12-27T00:47:21Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

soumith · 2022-12-27T00:48:07Z

eh, I just realized I can't merge this, as packaging is not an inbuilt (I just saw the comment from @MaKaNu above).

Can we use something that doesn't add a dependency?

soumith

shouldn't be using packaging

pytorchmergebot · 2022-12-27T00:49:13Z

The merge job was canceled. If you believe this is a mistake,then you can re trigger it through pytorch-bot.

soumith · 2022-12-27T00:51:47Z

we could add packaging as a dependency too, but it isn't added in as a dependency right now in this PR

Jasha10 · 2022-12-27T01:14:01Z

We can parse the version number by hand:

>>> import tensorboard
>>> tuple(map(int, tensorboard.__version__.split(".")))
(2, 11, 0)

The advantage of packaging.version.parse is that it's robust against different styles of version strings, e.g. it could handle "v1.2.3.dev0". Maybe this extra functionality doesn't matter for tensorboard though.

milutter · 2022-12-27T01:18:25Z

thanks @soumith for taking a look at this and trying to merge the PR!

I would prefer to add packaging as a dependency because this package is recommend by the deprecated python system package distutils and avoiding this package would result in brittle code (it would not be very generic code or handle edge-cases which are hard to enumerate in this case).

What seems weird to me that the tests are passing when packaging is not a dependency?
In an initial draft the CI failed on the packaging import (see #88524 (comment)). More precisely, test-reports/python-unittest/test_public_bindings failed with AttributeError: module 'packaging' has no attribute 'version' (I copied the full output below). Once the import got fixed CI passed, which points in the direction that there is a rudimentary CI test that should fail when the packaging package is not installed. So somehow packaging could already be a dependency of Pytorch?!

Nevertheless, where are the python dependencies listed in pytorch? I looked into the classical requirements.txt, but this states that it only contains the dev dependencies.

Older CI Failure:

 Test results will be stored in test-reports/python-unittest/test_public_bindings
  test_correct_module_names (__main__.TestPublicBindings)
An API is considered public, if  its  `__module__` starts with `torch.` ... ERROR (3.636s)
  test_correct_module_names (__main__.TestPublicBindings)
An API is considered public, if  its  `__module__` starts with `torch.` ...     test_correct_module_names errored - num_retries_left: 3
Traceback (most recent call last):
  File "/var/lib/jenkins/workspace/test/test_public_bindings.py", line 387, in test_correct_module_names
    for _, modname, ispkg in pkgutil.walk_packages(path=torch.__path__, prefix=torch.__name__ + '.'):
  File "/opt/conda/lib/python3.10/pkgutil.py", line 107, in walk_packages
    yield from walk_packages(path, info.name+'.', onerror)
  File "/opt/conda/lib/python3.10/pkgutil.py", line 92, in walk_packages
    __import__(info.name)
  File "/opt/conda/lib/python3.10/site-packages/torch/utils/tensorboard/__init__.py", line 4, in <module>
    if not hasattr(tensorboard, "__version__") or packaging.version.parse(
AttributeError: module 'packaging' has no attribute 'version'

soumith · 2022-12-27T02:00:45Z

you should add packaging as a dependency here and we're good: https://github.com/pytorch/pytorch/blob/master/setup.py#L980-L983

The reason CI passes is because the CI is pre-populated with a bunch of packages (for a variety of reasons)

Jasha10 · 2022-12-27T02:38:10Z

Done!

soumith · 2022-12-27T11:44:19Z

@pytorchbot merge

pytorchmergebot · 2022-12-27T11:45:56Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

soumith · 2022-12-27T13:00:21Z

thanks for the fix @Jasha10 !

facebook-github-bot · 2023-01-02T21:08:46Z

This pull request has been reverted by cce577b. To re-land this change, please open another pull request, assignthe same reviewers, fix the CI failures that caused the revert and make sure that the failing CI runs on the PR by applying the proper ciflow label (e.g., ciflow/trunk).

milutter · 2023-01-02T22:52:55Z

@weiwangmeta: This PR got reverted by your PR #91548. All the test failures link to internal meta resources, which make it hard to debug. Could you copy the test failure error message to investigate the failure?

soumith · 2023-01-03T02:59:52Z

yea the failures are around having to adjust internal build files. one of us will have to take it on. Can you reopen the PR

soumith · 2023-01-03T03:11:40Z

actually, looks like @Jasha10 is at Meta. Jascha, you can open this PR as a diff in FBCode and add packaging as a dependency to the caffe2/TARGETS file. then once you put up the diff, click "export to Github" in the UI, then all will be well to land it.

Jasha10 · 2023-05-17T11:46:19Z

@soumith I am no longer at Meta. Sorry I couldn't get to this while I was still there.

Jasha10 mentioned this pull request Nov 5, 2022

Fix deprecation warning in torch.utils.tensorboard import #84712

Closed

MaKaNu suggested changes Nov 8, 2022

View reviewed changes

torch/utils/tensorboard/__init__.py Outdated Show resolved Hide resolved

torch/utils/tensorboard/__init__.py Outdated Show resolved Hide resolved

torch/utils/tensorboard/__init__.py Outdated Show resolved Hide resolved

torch/utils/tensorboard/__init__.py Outdated Show resolved Hide resolved

milutter approved these changes Dec 9, 2022

View reviewed changes

MaKaNu approved these changes Dec 9, 2022

View reviewed changes

soumith approved these changes Dec 26, 2022

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Dec 26, 2022

Jasha10 added 2 commits December 26, 2022 23:10

qualify type: ignore comment

2339ee0

remove packaging from requirements.txt

a386cb8

soumith closed this Dec 27, 2022

soumith reopened this Dec 27, 2022

soumith requested changes Dec 27, 2022

View reviewed changes

add packaging to install_requires

0dd19a1

soumith approved these changes Dec 27, 2022

View reviewed changes

pytorchmergebot added the Merged label Dec 27, 2022

pytorchmergebot closed this in 1c681f4 Dec 27, 2022

Jasha10 deleted the patch-2 branch December 27, 2022 16:35

facebook-github-bot added the Reverted label Jan 2, 2023

Fix distutils.LooseVersion DeprecationWarning #88524

Fix distutils.LooseVersion DeprecationWarning #88524

Uh oh!

Conversation

Uh oh!

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/88524

✅ No Failures

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Merge started

Uh oh!

Merge failed

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Merge started

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Merge started

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!