skorch icon indicating copy to clipboard operation
skorch copied to clipboard

PyPI Readme Rendering

Open Ball-Man opened this issue 11 months ago • 5 comments

Hello,

I wanted to notify that the PyPI page of skorch is not rendering correctly. In fact, it looks like it the long description is being treated as plain text, while it is actually an rst document.

The problem

I did investigate this on a private fork, and by running a twine check this is what I get:

twine check dist/*

Checking dist/skorch-1.0.1.dev0-py3-none-any.whl: FAILED
ERROR    `long_description` has syntax errors in markup and would not be rendered on PyPI.
         line 10: Warning: Cannot scale image!
           Could not get size from "https://github.com/skorch-dev/skorch/workflows/tests/badge.svg":
           Requires Python Imaging Library.
           Reading external files disabled.
WARNING  `long_description_content_type` missing. defaulting to `text/x-rst`.
Checking dist/skorch-1.0.1.dev0.tar.gz: FAILED
ERROR    `long_description` has syntax errors in markup and would not be rendered on PyPI.
         line 10: Warning: Cannot scale image!
           Could not get size from "https://github.com/skorch-dev/skorch/workflows/tests/badge.svg":
           Requires Python Imaging Library.
           Reading external files disabled.
WARNING  `long_description_content_type` missing. defaulting to `text/x-rst`.

I did notice myself that long_description_content_type is missing. It may or may not be a good idea to make it explicit, even though it is inferred automatically by the renderer.

The real problem seems to be related to image scaling, in particular of the badges (coverage, etc.). It looks like this is a common issue, notified earlier this year by other devs (https://github.com/pypa/twine/issues/1102). The issue has reached the actual readme_rendered devs (https://github.com/pypa/readme_renderer/issues/304) but it looks like it still remains unsolved.

A possible solution

What some of the devs in the cited threads suggest, is to remove scaling from the badges. I tried this solution on my fork, and it does solve the rendering issue. I still get some artifacts, but it may be related to the fact that I was testing on TestPyPI.

A practical example:

.. |coverage| image:: https://github.com/skorch-dev/skorch/blob/master/assets/coverage.svg
    :alt: Test Coverage
.. to be removed
    :scale: 100%

My fork on GitHub shows that there is no difference in the visualization of the badges without the explicit scaling factor (at least, not on my desktop): https://github.com/Ball-Man/skorch/tree/no_scaling_readme.

If this is of interested, I can quickly apply draft a PR using this branch.

Ball-Man avatar Dec 20 '24 14:12 Ball-Man

Thanks a lot for reporting this issue. Your suspicion appears to be correct. We do a check in our deployment script for this exact thing:

https://github.com/skorch-dev/skorch/blob/5bd84bd45d3f1c4793a05370e0e6d2cc31a5d6be/scripts/deploy.sh#L66

I just ran this and it reports the same error as you mentioned. It is a bit strange that we didn't catch that in the last release, maybe the renderer was changed after that?

Let me know if you're interested in providing a PR to fix the issue, this would be really great.

BenjaminBossan avatar Dec 21 '24 20:12 BenjaminBossan

I would happily draft a PR in the next few days. I am checking the docs to see if there is anything specific I need to know before doing so, or feel free to give me explicit pointers about it. I am also including the explicit content type metadata. We may discuss further these details in the PR thread.

Ball-Man avatar Dec 22 '24 13:12 Ball-Man

@Ball-Man Are you still interested in working on this?

I am checking the docs to see if there is anything specific I need to know before doing so, or feel free to give me explicit pointers about it.

No, I don't think that apart from your suggested change, there is anything that needs to be considered. We can ensure that the docs render correctly by invoking python -m readme_renderer README.rst.

BenjaminBossan avatar Jan 07 '25 15:01 BenjaminBossan

Yes, I am sorry about the delay for such a quick fix. I was back today from winter holidays. I can probably submit the PR tonight or tomorrow (UTC + 1).

Ball-Man avatar Jan 07 '25 15:01 Ball-Man

Fantastic, thanks a lot. This was just intended as a reminder, don't feel pressured.

BenjaminBossan avatar Jan 07 '25 15:01 BenjaminBossan