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

loader.py: use urllib3, support custom timeout #8602

Open
az75014 opened this issue Feb 1, 2024 · 5 comments
Open

loader.py: use urllib3, support custom timeout #8602

az75014 opened this issue Feb 1, 2024 · 5 comments
Labels
Component: Widgets kivy/uix, style.kv Type: Feature Issue is a feature request

Comments

@az75014
Copy link

az75014 commented Feb 1, 2024

Is your feature request related to a problem? Please describe.

My app returns a search result in RecycleView with each result containing a small thumbnail logo of ~15kb. The short 60 second loader cache timeout combined with the inefficient image loading means that by the time the last image is loaded the first images are already flushed from the cache so restart loading again.

I "hacked" loader.py on my setup: I set timeout to 180, replaced urllib.request with urllib3 and instantiated a pool manager. The image load is massively more efficient now. (My images are all hosted on one server).

Describe the solution you'd like

Replace the old urllib: urllib3 has a pool manager, which massively improves efficiency and speed when reusing the same connection (e.g. images hosted on the same server).

The timeout for keeping images in cache is currently hard coded to 60 seconds. This should be configurable depending on use-case, same as num_workers and max_upload_per_frame are now. e.g. Loader.timeout = 180

@Julian-O
Copy link
Contributor

Julian-O commented Feb 1, 2024

I am working on a refactor of loader.py in the background, so listening with interest. Do you want to share the changes you tried?

@Julian-O Julian-O added Component: Widgets kivy/uix, style.kv Type: Feature Issue is a feature request labels Feb 1, 2024
@az75014
Copy link
Author

az75014 commented Feb 2, 2024

Sure! To prove to myself that adding pooling would make the difference for me I created a MyAsyncImage/myasyncimage.py so that I could make it load my hacky MyLoader/myloader.py rather than the Kivy code one. It was basically copy/paste, apart from the changes indicated below.

This was just a test, I'm sure it needs more thought than this.

...
import urllib3
pool_mgr = urllib3.PoolManager(maxsize=10)
"""
Note: I set maxsize to 10 because I was getting warnings due to too many concurrent requests:
[WARNING] [Connection pool is full, discarding connection]
I tested this with a value of 1000 also with seemingly no ill effect. 
Maybe it should match the Loader.num_workers value? 
It's set to 10 in my case as the images I am loading are rather small.
I left num_pools unchanged as not relevant in my case: there was only ever going to be 1 pool,
namely the connection to my image server. (default is 10)
"""
Cache.register('kv.loader', limit=500, timeout=500)
...
    def _load_urllib(self, filename, kwargs):
        '''(internal) Loading a network file. First download it, save it to a
        temporary file, and pass it to _load_local().'''	
        import tempfile
        global pool_mgr
        if not pool_mgr:
            pool_mgr = urllib3.PoolManager(maxsize=10)
...
        try:
            _out_filename = ''
            """
            Note: in my hacked version I removed a whole section here about smb, 
            headers, custom context as not relevant to my test
            """
            fd = pool_mgr.request("GET", filename)

            if '#.' in filename:
                # allow extension override from URL fragment
                suffix = '.' + filename.split('#.')[-1]
            else:
                ctype = fd.headers['Content-Type']
...
            idata = fd.data
            # removed: fd.close()
            # removed: fd = None
...

@NomadDemon
Copy link
Contributor

And how it behave when you change default network backend to "requests"? Its single line in config file

Can someone benchmark it?

@az75014
Copy link
Author

az75014 commented Feb 8, 2024

The loader.py currently doesn't use the config file settings (apart from user agent) -- urlib.request is hardcoded.

There is no session mechanism for the "requests" implementation, so that shouldn't make a difference?

On my app I make several requests to my API on start up, I had a huge efficiency boost after I implemented sessions for that.

Currently, I create a session in my App class, save it as a class attribute and then pass the session object as an argument to e.g. my API request module. It works fine but seems a bit hacky to do it this way.

There might be scope to handle this better?

@NomadDemon
Copy link
Contributor

The loader.py currently doesn't use the config file settings (apart from user agent) -- urlib.request is hardcoded.

There is no session mechanism for the "requests" implementation, so that shouldn't make a difference?

On my app I make several requests to my API on start up, I had a huge efficiency boost after I implemented sessions for that.

Currently, I create a session in my App class, save it as a class attribute and then pass the session object as an argument to e.g. my API request module. It works fine but seems a bit hacky to do it this way.

There might be scope to handle this better?

it uses

Try

Config.set("network", "implementation", "requests")

it should work from kivy 2.2.0

implementation_map = {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Widgets kivy/uix, style.kv Type: Feature Issue is a feature request
Projects
None yet
Development

No branches or pull requests

3 participants