-
Notifications
You must be signed in to change notification settings - Fork 12
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
Buenos Aires: MemoryError #154
Comments
This also happens with buenos-aires collection (@yolile). By the way, shouldn't this issue be on the kingfisher-process project? |
I don't know - is the MemoryError occurring during a crawl, and then getting sent to kingfisher-process' |
Apparently there is no errors sent during a crawl, see this log. API calls are not returning error codes neither (I've seen HTTP error codes when sending files with incorrect formats). |
@romifz is this something that you'd like someone like @odscjames to look into? How urgent is it? |
It's not urgent, for now I've been downloading the files using kingfisher-scrape locally and loading them to kingfisher-process using local-load. |
When I run this locally on Django debug server I report a memory useage of 681 MB for processing the largest file. UWSGI is currently capped at 500MB. (Tho I am puzzled this isn't the memory error I've seen before from UWSGI) Anyway, the short term fix is simply to increase memory. I think that server has 64GB of RAM after all. The long term fix would be to use the Redis que - when a large file comes in, the web process simply stores it, returns a response to the caller as quick as possible and puts a message on the que. A Redis worker then processes it - they have no memory limits, I think, and they can take a longer time to process larger files without an HTTP agent waiting for a response. |
Limit now increased to 1GB. We also have a 10 min limit per request, but we haven't seen that being broken yet. |
@odscjames I checked Buenos Aires file and it has ~800MB (uncompressed), so the memory limit should work for now! This file gets updated every 15 days, so it will grow with time, but I think it would take a while to reach the 1GB cap. |
The memory use of Python will be more to load the file than the size of the file on disk - but I'm not sure how much more it will be. I'll increase the limits a bit for now anyway. |
I increased the RAM limit to 4GB and Buenos Aires still errors. This needs looking at more. |
I have looked into this and something isn't right here. I think there is actually only one place in the code that would get this to crash the way it is:
I tested locally and I got 3.3GB memory use opening the 800MB Buenos Aires file - which is big, but I tried upping the limit to 16GB on the live server and it still crashed, so something else is going on. Need to dig into this more. (We hear that local load works, so an incremental improvement to what's currently there along the lines explored in open-contracting/kingfisher-process#171 would still be good to discuss too) |
I have tried the code section above with the Buenos Aires data file via a UWSGI process and it worked fine on my desktop. So more digging into this is required. |
Marking as blocked as the fix is in Kingfisher Process. Updating issue title as ONCAE works now, but BA still fails. |
@jpmckinney I think that I can do something similar to what we did for Portugal, but using ijson as you suggested |
I'm fine with fixing this on the Kingfisher Scrape side, as it will be some time before Kingfisher Process if fixed to handle large files. |
@jpmckinney Buenos Aires uses a big release package, should we add to each release the package metadata before send it to kingfisher-process? |
Yes, that seems best. |
Hmm, I'm remembering it's not so easy to solve in the general case with streaming input (but we should be able to do it for just BA, where we can read a file from disk multiple times). Note: Instead of sending a package with a single release, we can create packages with as many releases as we can without running out of memory. Now, the challenge is extracting the metadata in an iterative fashion, and then extracting the releases in an iterative fashion. I wrote up a general solution here, which requires that all metadata fields occur before the releases/records field: open-contracting/ocdskit#118 Another solution is to:
Does that make sense? |
I'm also remembering that OCDS Kit has a grouper method used in the |
done in #366 |
@romifz:
@odscjames:
The text was updated successfully, but these errors were encountered: