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

Add --report argument to __main__.py to omit supported formats #7818

Merged
merged 14 commits into from Mar 30, 2024

Conversation

nulano
Copy link
Contributor

@nulano nulano commented Feb 20, 2024

Changes proposed in this pull request:

  • Add __main__.py to output basic format and support information #3870 added pilinfo to help in diagnosing issues. However, because it prints a list of all supported formats, I have sometimes found it difficult to explain which lines can be relevant when asking users to provide its output, e.g. in Thai font not supported anymore #7747 (comment). Adding a --bugreport option (feel free to suggest other names) to suppress printing the list of supported formats would make it easier to explain (and should be fine for older versions which will just ignore the new option).

  • Add sys.executable, sys.prefix, sys.base_prefix to pilinfo. This can help resolve the occasional confusion when there are multiple Python versions / virtual environments involved, or highlight when Anaconda Python is used.

    For example:

    -------------------------------------------------------------------
    Pillow 10.3.0.dev0
    Python 3.11.8 (v3.11.8:db85d51d3e, Feb  6 2024, 18:02:37) [Clang 13.0.0 (clang-1300.0.29.30)]
    --------------------------------------------------------------------
    Python executable is /Library/Frameworks/Python.framework/Versions/3.11/bin/python3
    System Python files loaded from /Library/Frameworks/Python.framework/Versions/3.11
    --------------------------------------------------------------------
    Python Pillow modules loaded from /Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages/PIL
    Binary Pillow modules loaded from /Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages/PIL
    --------------------------------------------------------------------
    
  • Add the command to the issue template -- perhaps this change should wait until after the next release.

@@ -48,6 +48,10 @@ Thank you.
* Python:
* Pillow:

```text
please paste the output of running `python3 -m PIL --bugreport` here
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given some of the issues that we've dealt with, I can imagine users running python3 -m PIL --bugreport even if they're using a copy of Python other than python3 to invoke their code.

Would it be better to ask users to paste the output of from PIL import features;features.pilinfo(supported_formats=False)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. That also avoids the issue of --bugreport not being present in older versions.

However, I think adding the option python3 -m PIL --bugreport is still useful on its own.

Copy link
Member

@radarhere radarhere Feb 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should --bugreport be documented somewhere then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created nulano#31

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we suggest both? I like the idea of making it extremely easy as a first suggestion:


Paste the output of python3 -m PIL --bugreport or the output of the following Python code:

from PIL import features; features.pilinfo(supported_formats=False)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just concerned about users following python3 -m PIL --bugreport so verbatim that they don't change the Python command to what they are using to run their script, e.g. #7532 (comment)

However, maybe it is better to make most users lives easier than to cater to absolutely everyone.

@hugovk
Copy link
Member

hugovk commented Feb 20, 2024

I've seen other projects have some simple command to run to print out some handy debug info for bug reports, and have been meaning to open a PR for it but never got round to it, so thank you for this!

I agree a short command would be good rather than asking to import something.

A very common issue we get is people having multiple Python interpreters, and different Pillow versions for each (or only Pillow for one), like https://snarky.ca/why-you-should-use-python-m-pip/

So having a command they can run could mean they use the same python or python3 or py or whatever they're using for their script. But again, maybe they copy/paste python3 and it's not. However, the interpreter name in the output so that could help.

@aclark4life
Copy link
Member

Nice! Also a missing --bugreport would be useful as an indicator of a version older than 10.3. 🤔

assert lines[1].startswith("Python executable is")
lines = lines[2:]
if lines[0].startswith("Environment Python files loaded from"):
lines = lines[1:]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for the record, this happens for... virtual environments?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct: https://docs.python.org/3/library/sys.html#sys.base_prefix

I'm not sure if this can also happen outside of a virtual environment, e.g. on Conda, so I didn't want to explicitly state "Virtual Environment Python files...".

@hugovk
Copy link
Member

hugovk commented Mar 4, 2024

Naming things bike shed corner:

I was going to suggest --report over --bugreport, because it's shorter and could be more generally useful.

I've seen other projects have some simple command to run to print out some handy debug info for bug reports...

Here's what some projects use:

pipenv --support
brew doctor
scoop checkup
hatch self report  # coming soon: https://hatch.pypa.io/dev/how-to/meta/report-issues/

I think either --report or --support, what do you think?

@hugovk
Copy link
Member

hugovk commented Mar 4, 2024

And I just remembered the original one I was thinking of, pip debug. For example:

Details
pip debug
WARNING: This command is only meant for debugging. Do not use this with automation for parsing and getting these details, since the output and options of this command may change without notice.
pip version: pip 24.0 from /Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/site-packages/pip (python 3.12)
sys.version: 3.12.2 (v3.12.2:6abddd9f6a, Feb  6 2024, 17:02:06) [Clang 13.0.0 (clang-1300.0.29.30)]
sys.executable: /Library/Frameworks/Python.framework/Versions/3.12/bin/python3
sys.getdefaultencoding: utf-8
sys.getfilesystemencoding: utf-8
locale.getpreferredencoding: UTF-8
sys.platform: darwin
sys.implementation:
  name: cpython
'cert' config value: Not specified
REQUESTS_CA_BUNDLE: None
CURL_CA_BUNDLE: None
pip._vendor.certifi.where(): /Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/site-packages/pip/_vendor/certifi/cacert.pem
pip._vendor.DEBUNDLED: False
vendored library versions:
  CacheControl==0.13.1
  colorama==0.4.6
  distlib==0.3.8
  distro==1.8.0
  msgpack==1.0.5
  packaging==21.3
  platformdirs==3.8.1
  pyparsing==3.1.0
  pyproject-hooks==1.0.0
  requests==2.31.0
  certifi==2023.07.22
  chardet==5.1.0
  idna==3.4
  urllib3==1.26.17
  rich==13.4.2 (Unable to locate actual module version, using vendor.txt specified version)
  pygments==2.15.1
  typing_extensions==4.7.1 (Unable to locate actual module version, using vendor.txt specified version)
  resolvelib==1.0.1
  setuptools==68.0.0 (Unable to locate actual module version, using vendor.txt specified version)
  six==1.16.0
  tenacity==8.2.2 (Unable to locate actual module version, using vendor.txt specified version)
  tomli==2.0.1
  truststore==0.8.0
  webencodings==0.5.1 (Unable to locate actual module version, using vendor.txt specified version)
Compatible tags: 582
  cp312-cp312-macosx_14_0_arm64
  cp312-cp312-macosx_14_0_universal2
  cp312-cp312-macosx_13_0_arm64
  cp312-cp312-macosx_13_0_universal2
  cp312-cp312-macosx_12_0_arm64
  cp312-cp312-macosx_12_0_universal2
  cp312-cp312-macosx_11_0_arm64
  cp312-cp312-macosx_11_0_universal2
  cp312-cp312-macosx_10_16_universal2
  cp312-cp312-macosx_10_15_universal2
  ...
  [First 10 tags shown. Pass --verbose to show all.]

So we can throw debug into the bikeshed.

Here was a cut down version of their debug output I'd started on last October, but not got very far with:

Details
import importlib.metadata
import locale
import os
import platform
import sys
from typing import Any


def show_value(name: str, value: Any) -> None:
    print(f"{name}:\t{value}")


def get_pip_version() -> str:
    pip_pkg_dir = os.path.join(os.path.dirname(__file__), "..", "..")
    pip_pkg_dir = os.path.abspath(pip_pkg_dir)

    __version__ = importlib.metadata.version("pip")

    return f"pip {__version__} from {pip_pkg_dir}"


def debug_info() -> None:
    # print(f"sys.version\n\t{sys.version}\n")
    # print(f"platform.platform()\n\t{platform.platform()}\n")
    # print(f"platform.version()\n\t{platform.version()}\n")
    # print(f"sys.implementation.name\n\t{sys.implementation.name}\n")

    show_value("pip version", get_pip_version())
    show_value("sys.version", sys.version)
    show_value("sys.executable", sys.executable)
    show_value("sys.getdefaultencoding", sys.getdefaultencoding())
    show_value("sys.getfilesystemencoding", sys.getfilesystemencoding())
    show_value(
        "locale.getpreferredencoding",
        locale.getpreferredencoding(),
    )
    show_value("sys.platform", sys.platform)
    show_value("sys.implementation.name", sys.implementation.name)

    show_value("platform.platform", platform.platform())
    show_value("platform.version", platform.version())

    show_value("sys.maxsize", sys.maxsize)
    show_value("64 bit", sys.maxsize > 2**32)


debug_info()

I think we can stick with what we have for now and consider adding new output later.

@nulano
Copy link
Contributor Author

nulano commented Mar 4, 2024

I think either --report or --support, what do you think?

I'm currently leaning towards --report.

Out of context I would prefer --support, but I think that could be confusing given that it is setting supported_formats=False.

@radarhere radarhere modified the milestone: 10.3.0 Mar 9, 2024
@radarhere
Copy link
Member

Add the command to the issue template -- perhaps this change should wait until after the next release.

python3 -m PIL --report still runs successfully without this PR, just without the new information and truncated output, so I'm not really concerned about users executing it before 10.3.0.

radarhere
radarhere previously approved these changes Mar 9, 2024
@hugovk hugovk changed the title Add --bugreport argument to __main__.py to omit supported formats Add --report argument to __main__.py to omit supported formats Mar 9, 2024
@hugovk
Copy link
Member

hugovk commented Mar 9, 2024

I think either --report or --support, what do you think?

I'm currently leaning towards --report.

Out of context I would prefer --support, but I think that could be confusing given that it is setting supported_formats=False.

I don't necessarily think that it's setting supported_formats=False is something we need to worry about, I see that more as an internal detail, and we're exposing a command for users to run.

I'm fine with either.


I was thinking, could we make this even easier: instead of python3 -m PIL --report, provide python3 -m PIL report?

But we can go another step further! Revert the changes to src/PIL/__main__.py and create src/PIL/report.py (or 🚲🎨 support.py):

from __future__ import annotations

from .features import pilinfo

pilinfo(supported_formats=False)

And then:

$ python3 -m PIL --report  # Not this

$ python -m PIL.report     # Do this

And it's an even better improvement for the REPL/a script:

from PIL import features                   # Not this
features.pilinfo(supported_formats=False)  # Not this

from PIL import report                     # Do this

@nulano
Copy link
Contributor Author

nulano commented Mar 9, 2024

python -m PIL.report

The issue with this is that it would only work for new versions, whereas adding an argument to __main__ is backwards compatible (in that the argument is ignored on old versions). That said, we can still add it and switch to asking for python -m PIL.report later.

@hugovk
Copy link
Member

hugovk commented Mar 9, 2024

python -m PIL.report

The issue with this is that it would only work for new versions, whereas adding an argument to __main__ is backwards compatible (in that the argument is ignored on old versions).

That's fine :) We only support the latest version anyway. If someone reports an issue for an old version, a common question is "does it work with the latest version?"

That said, we can still add it and switch to asking for python -m PIL.report later.

After how long could we switch? Would we consider --report part of the public API and need deprecating --report for a year+ before removal, or could we remove that it warning? (I'd lean to the latter.)

I think it'd be easier just to have python -m PIL.report now instead of python -m PIL --report.

We can always ask people to run python -m PIL to get the full output if for some reason we need it for an old version.

@radarhere radarhere dismissed their stale review March 9, 2024 19:56

Changes still in progress

@nulano
Copy link
Contributor Author

nulano commented Mar 19, 2024

python -m PIL.report

The issue with this is that it would only work for new versions, whereas adding an argument to __main__ is backwards compatible (in that the argument is ignored on old versions).

That's fine :) We only support the latest version anyway. If someone reports an issue for an old version, a common question is "does it work with the latest version?"

That said, we can still add it and switch to asking for python -m PIL.report later.

After how long could we switch? Would we consider --report part of the public API and need deprecating --report for a year+ before removal, or could we remove that it warning? (I'd lean to the latter.)

I think it'd be easier just to have python -m PIL.report now instead of python -m PIL --report.

We can always ask people to run python -m PIL to get the full output if for some reason we need it for an old version.

@radarhere Do you have an opinion on this?

@radarhere
Copy link
Member

Taking a quick look through closed issues, most users do file reports using the latest version of Pillow - but not all. On March 1 for example, there was an issue from a user with Pillow 8.4.0. However, not all users follow our issue template either.

We ask users in the issue template what version of Pillow they're running. That seems odd if you can't follow the instructions in the issue template for anything except the latest version.

So i guess I lean towards the backwards compatible version.

@hugovk
Copy link
Member

hugovk commented Mar 21, 2024

Taking a quick look through closed issues, most users do file reports using the latest version of Pillow - but not all. On March 1 for example, there was an issue from a user with Pillow 8.4.0.

For that issue, we asked if they could retest with the latest version (they didn't answer) and for more info with a feature check (they didn't answer), and it was auto-closed due to lack of feedback.

Anyway, in the template, shall we put this:

Please paste here the output of running:

python3 -m PIL.report
or
python3 -m PIL --report

Or the output of the following Python code:

from PIL import report
# or
from PIL import features
features.pilinfo(supported_formats=False)

Then after a couple of releases or so, just have the new one?

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

src/PIL/features.py Outdated Show resolved Hide resolved
Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
@radarhere
Copy link
Member

I think test_report.py can be combined into test_main.py. I've created nulano#34

@hugovk hugovk merged commit a4e5dc2 into python-pillow:main Mar 30, 2024
56 of 57 checks passed
@hugovk
Copy link
Member

hugovk commented Mar 30, 2024

Thank you!

@nulano nulano deleted the bugreport branch March 30, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants