Add a --timeout option to the CLI API

[quique]

I'm thinking of something like
weaseyprint --timeout 30 https://weasyprint.org /tmp/weasyprint-website.pdf

The provided value would just have to be passed to the default_url_fetcher(url, timeout=10, ssl_context=None) method.

[liZe]

That’s a good idea. It should be possible to pass the default URL fetcher with timeout set to given value in __main__.py. Anyone interested to add this feature?

[MalanB]

Hey, interested in picking this issue

[liZe]

Hey, interested in picking this issue

Cool! Don’t hesitate to ask if you want some hints!

[staticvoidzach]

Wondering if there should be multiple types of timeout options? When looking at weasyprint --timeout 30 in.com out.pdf my first though is the timeout applies to the entire process, so the command will exit prematurely if the fetch + resource gather + render process takes more than 30 seconds.

Although, I'd also like to have the ability to control the timeouts of individual stages, e.g. --timeout-for-initial-page-fetch and --timeout-for-each-individual-asset-fetch-and-load and --timeout-for-pdf-render, perhaps with shorter names however...

I'd also consider what the error behaviour would be? Should that be configurable? Potentially someone doesn't want their entire pdf generation to die if 1 out of 10 images takes longer than 10 seconds to fetch. Seems like an undesirable situation, yes, I wouldn't my company logo to be missing on 1 random report out of 100, however there's always a situation for these things.

Perhaps if timeout control is introduced for url fetching, potentially control for retries also needs to be considered?

Not sure about the best way to implement this though, as adding 8 new obscure arguments may be overkill and clutter the help menu. On the other hand, all of my favourite CLI tools completely fill my screen when I run --help, so possibly not the worst idea?

[liZe]

I think that WeasyPrint can have a timeout option that will be used for each request, because it’s both useful (at least to avoid waiting forever when there’s a network problem) and easy to implement (just a parameter to give to the standard library.)

An option to control the overall time is useful to, but it’s much harder to implement. Moreover, other tools already exist to handle this, like timeout.

For more complex cases, we won’t be able to handle everything users will want using simple CLI options. That’s why users can use their own URL fetchers and do whatever they want in Python.

So, my advice is to do what’s in the original comment: a timeout option that is passed to the default URL fetcher.

[azhar316]

Hey @MalanB, are you still working on this? If not I would love to have a go at it.

[MalanB]

@azhar316 please go ahead, day job is hectic and I don't seem to find time till now :(

[azhar316]

Thanks! @MalanB

Hey @liZe, I have some questions it would be really helpful if you can answer them.

1. I created the setup in my local and ran the tests but 2 of them are failing(output below) before I have even made any modifications to the codebase. Is this any system specific issue?

assert_same_renderings = <function assert_same_renderings.<locals>.<lambda> at 0x10ccf4040>

    @assert_no_logs
    def test_image_exif(assert_same_renderings):
>       assert_same_renderings(
            '''
                <style>@page { size: 10px }</style>
                <img style="display: block" src="not-optimized.jpg">
            ''',
            '''
                <style>@page { size: 10px }</style>
                <img style="display: block" src="not-optimized-exif.jpg">
            ''',
            tolerance=25,
        )

    @assert_no_logs
    def test_font_stretch():
        page, = render_pages('''
          <style>
            p { float: left; font-family: %s }
          </style>
          <p>Hello, world!</p>
          <p style="font-stretch: condensed">Hello, world!</p>
        ''' % SANS_FONTS)
        html, = page.children
        body, = html.children
        p_1, p_2 = body.children
        normal = p_1.width
        condensed = p_2.width
>       assert condensed < normal
E       assert 101.546875 < 101.546875

FAILED tests/draw/test_image.py::test_image_exif - AssertionError: Pixel (8, 8) in image_exif_1: expected rgba(210, 210, 210), got rgba(75, 75, 75)
FAILED tests/layout/test_inline.py::test_font_stretch - assert 101.546875 < 101.546875

2. I am planning to accomplish this functionality by adding the below piece of code:

url_fetcher = default_url_fetcher
if args.timeout:
    url_fetcher = (lambda url, timeout=args.timeout, ssl_context=None: 
        default_url_fetcher(url, timeout, ssl_context))

del args.timeout

is this good or should I explore a different method?

3. I assume that I should add tests for this in the 'test_command_line_render' function of 'test_api.py' file but how should I go about making sure that the request takes more than x seconds.

[liZe]

Hi @azhar316,

1. I created the setup in my local and ran the tests but 2 of them are failing(output below) before I have even made any modifications to the codebase. Is this any system specific issue?

For the second test, you need to install the DejaVu fonts, including condensed ones. I’ve just updated the documentation about that!

But for the first one, I’ve never seen someone complain (tests pass for sure on my computer, on Linux distributions packaging platforms and on CI). That’s strange. That’s be interesting to find the reason for that, but you can safely ignore the problem if you don’t want to fall down the rabbit hole!

2. I am planning to accomplish this functionality by adding the below piece of code:
   is this good or should I explore a different method?

This idea should work!

You can:

• remove the del statement (that was only to deprecate --optimize-size and be sure that it’s not used anywhere else),
• use functools.partial instead of a lambda function, that’ll be cleaner and future-proof 🤞🏽.

3. I assume that I should add tests for this in the 'test_command_line_render' function of 'test_api.py' file but how should I go about making sure that the request takes more than x seconds.

That would be complex to test the timeout, but we can at least test that the option is accepted and gives the right document as a first step.

[azhar316]

But for the first one, I’ve never seen someone complain (tests pass for sure on my computer, on Linux distributions packaging platforms and on CI). That’s strange. That’s be interesting to find the reason for that, but you can safely ignore the problem if you don’t want to fall down the rabbit hole!

Will do it as and when I find some time (past one week has been quite hectic work wise!).

remove the del statement (that was only to deprecate --optimize-size and be sure that it’s not used anywhere else),
use functools.partial instead of a lambda function, that’ll be cleaner and future-proof 🤞🏽.

Done.

That would be complex to test the timeout, but we can at least test that the option is accepted and gives the right document as a first step.

Have added the below lines of code to the 'test_command_line_render' function of 'test_api.py' file:

_run('combined.html out23.pdf --timeout 30')   
assert tmpdir.join('out23.pdf').read_binary() == pdf_bytes

Let me know if this is acceptable. I will create a PR!

[liZe]

@azhar316 That would be perfect!

[azhar316]

@liZe Done! PR #1971