feat: add inlining options and a new inline command
#374
Conversation
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
mikix
force-pushed
the
mikix/inline
branch
4 times, most recently
from
2882da0 to
cce70c5
Compare
| @@ -86,7 +90,15 @@ async def __aexit__(self, exc_type, exc_value, traceback): | |||
| await self._session.aclose() | |||
|
|
|||
| async def request( | |||
There was a problem hiding this comment.
Let me lightly explain some of the refactoring here.
Before, FhirClient.request was a single request. The retry logic was in the bulk export code, which retried on both error cases and the expected 202 status code for "still working on it".
Now, I've separated the bulk export retry logic into error-path and 202-path, and moved the error-path into this FhirClient.request method. So now any code making requests to the server has easy retries (enabled by default for any request).
mikix
force-pushed
the
mikix/inline
branch
3 times, most recently
from
5abc924 to
01b515b
Compare
| sed -i 's/"generated_on": "[^"]*", //g' $OUTDIR/*.ndjson | ||
| sed -i 's/"generated_on":"[^"]*",//g' $OUTDIR/*.ndjson |
There was a problem hiding this comment.
Adjusted to remove expected whitespace because this PR also switches to a more condensed NDJSON output mode for the ETL that removes whitespace (in an attempt to reduce bloat a little bit when inlining).
| # Look at twice the allowed connections - downloads will block, but that's OK. | ||
| # This will allow us to download some attachments while other workers are sleeping | ||
| # because they are waiting to retry due to an HTTP error. | ||
| peek_at=fhir.FhirClient.MAX_CONNECTIONS * 2, |
There was a problem hiding this comment.
This is lightly hostile because if a server is actually overloaded (vs the normal random flakiness we tend to see), we might be hitting it a little more than we ought to... I'm testing right now on this a little to see if removing this bit changes anything on a sample pull against Cerner. But I'm inclined to try running with this for performance reasons (under the assumption that we rarely get an authentic overloaded error and just random flakiness is more likely).
There was a problem hiding this comment.
Just tested this - if we only peek at MAX_CONNECTIONS at a time, we're roughly 10% slower (took 11h instead of 10h for a big inline job).
Inlining here means downloading attachment URLs and insert them as attachment data, allowing you to download the attachment just once, at the cost of extra storage space. This is often useful in preparation for NLP tasks, where you want to often refer back to DocumentReference clinical notes, but do not want to constantly deal with the flakiness of an EHR server. (Or losing access to that server.) New features: - There is a new `inline` command that can inline an existing folder of NDJSON. - The `export` and `etl` commands both now accept an opt-in flag to inline data after performing a bulk export. - If the server provides a Retry-After header that is a timestamp (rather than a number of seconds), we now parse that correctly. - All server requests are retried at least a little bit upon errors. (previously, only bulk export requests were retried - now every time we hit the server, so even for Medication downloads or DocRef attachment downloads during NLP tasks. Behavior changes: - If the server gives us a 429 error, we now log it as an error message, and don't log a successful progress call. - If the server gives us a 429 error, we use the next exponential backoff delay instead of hard coding 60 seconds as the delay. - If the server gives us a Retry-After header on an error message, we no longer unconditionally accept and use it. Rather the requested delay is capped by our next exponential backoff delay. That is, the server's Retry-After time will be used if it's LESS than our next backoff, but if it's longer, we'll still use our own backoff. This is lightly hostile, but (a) it's only done on error cases, (b) our backoff times are generous, and counted in minutes not seconds, and (c) it lets us guarantee a max delay time for callers. - Instead of retrying on 429 and ALL 5xx errors, there's a specific list of error codes that we retry on. Currently it's 408, 429, 500, 502, 503, and 504. - Have the bulk exporter time out after 30 days, rather than one. We've seen Epic exports take a couple weeks.
| "pyarrow < 19", | ||
| "pyarrow < 20", |
There was a problem hiding this comment.
Unrelated, I just stole this from a dependabot PR that was consistently failing CI for no good reason.
Inlining here means downloading attachment URLs and insert them as attachment data, allowing you to download the attachment just once, at the cost of extra storage space.
This is often useful in preparation for NLP tasks, where you want to often refer back to DocumentReference clinical notes, but do not want to constantly deal with the flakiness of an EHR server. (Or losing access to that server.)
New features:
inlinecommand that can inline an existing folder of NDJSON.exportandetlcommands both now accept an opt-in flag to inline data after performing a bulk export.Behavior changes:
Sample Output
Checklist
docs/) needs to be updated