Asap 204 change all file handling to requests - #239
Conversation
| from werkzeug import Request, Response | ||
|
|
||
| """ | ||
| Tests to assert that our file getting method works with some known curveballs. |
There was a problem hiding this comment.
I added a few tests to try and cover issues we've run into in the past. I'm honestly not sure how helpful these are. We did not keep specific records of every file download error we ran into, many of which were on the Rails side. It's also difficult to replicate some of the server behavior faithfully in a test setup. The header check test fails with urllib, but that is a bit of a false flag because it's difficult to know which headers Georgia has configured Cloudflare to look for. The rest of tests pass with either approach.
If we run into any other issues like this, we can add a new test and document them here.
There was a problem hiding this comment.
I wonder if utilizing Jira might be a better approach than a spreadsheet, since we already document issues there. I'm not super familiar, but maybe we could create a new label so it's easier for us to find these issues?
Here are a couple of previous examples that seem relevant: ASAP-161, ASAP-35, ASAP-70.
There was a problem hiding this comment.
Good thinking. I added a label to ASAP-204 and the other issues. ASAP-35 and ASAP-70 are related to the Rails app and crawler, nothing we should add test coverage for here. I think the test for the 308 redirect covers the same scenario as the .cfm files from ASAP-161.
allisonmorgan
left a comment
allisonmorgan
left a comment
There was a problem hiding this comment.
This looks great! Thank you for mocking up the tests and for digging into this. I confirmed that this URL getting approach worked for the Georgia sites you flagged, while also still working for SLC.
I did notice more failures on Austin documents, but I don't think this is related to the code changes here because they appear to fail when I navigate to them on their site via my web browser. Since that lead has gone cold, I don't think it's worth figuring out what's happening there.
Yes, I noticed this as well. I think they may have increased security on their document URLs. The .cfm files give me this error when I visit them with the browser. 
|
2 participants
Some of the stakeholder sites have some different redirect strategies and security measures in place for their URLs. The requests library is a more robust option for downloading the files than the core urllib. We should implement a solution that makes the LLM features work for Georgia.
This PR changes both the document inference and evaluation codebases to use the same requests library-based download function. I also made the document inference container mount the codebase for easier development. I also added failure messages to the logs (eventually CloudWatch). Right now they are just sent back in the API response.
Rebuild the containers, add the API key and test the LLM features.
Georgia reported issues with the following sites:
Georgia: Office of the Child Advocate
Georgia: Nonpublic Postsecondary Education Commission
Georgia: Department of Human Services Division of Family & Children Services
We should test these out specifically.
No