Repository navigation
fix: open each dataset once per request - #171
Conversation
XarrayMosaicBackend opened every source to validate mosaic compatibility, closed it, and rio-tiler's tile/part/point paths then opened it again. For a large Icechunk store each open costs ~1.5s, which doubled tile and point latency. Keep the validation readers open for the life of the backend, route rio-tiler's per-asset reader calls to them, and close them when the request's backend context exits (or when validation fails part-way). Duplicate URLs now share one reader. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
/info?show_times=true opened the dataset a second time to read its time coordinate; read it from the reader the mosaic backend already holds open. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The opens fixture patched guess_opener on whichever titiler.multidim.reader module was imported when it ran. The app fixture drops every titiler.multidim module from sys.modules and re-imports, so listing opens before app in a test signature left the patch on the discarded module and opens stayed empty. Depending on app orders the fixtures regardless of how a test lists them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…equest # Conflicts: # src/titiler/multidim/mosaic.py
|
I'm going to release v0.10.0 without this PR so that it doesn't block the deployment for AIR4US, since they need #166. I'd like to still target releasing this performance improvement this week. I've been defaulting to requiring a review before merging PRs, but also don't want to use up too much of other people's time. Please let me know if I should go based on a different norm regarding reviews. |
Releasing with the changes from #166 sounds good @maxrjones. Regarding PR reviews before merging, I'd prefer thorough reviews beforehand, as they can uncover critical issues. Off the top of my head, the ReDoS issue I flagged in #166 comes to mind. I have started reviewing this PR and have uncovered what I believe to be a performance problem. Given that this PR is specifically to address performance, I'd like to finish this review, and have you make adjustments, before merging. Generally speaking, I'd rather "use up too much of other people's time" up front, because it is generally much less time and cost spent up front to avoid problems to begin with than it is to debug and fix such problems later. |
Thanks, Chuck, that sounds good to me. I tried to determine the norms from the PR history, but it's much more reliable to hear it first hand. I'll update the repo settings accordingly. Please give me a heads up if you expect that the problem you uncovered will make working on this PR extend into next week, so that I can communicate to the AIR4US devs who requested this work. |
chuckwondo
left a comment
chuckwondo
left a comment
There was a problem hiding this comment.
The five suggestions labeled "ExitStack N of 5" in mosaic.py replace the close() loop with contextlib.ExitStack, so please apply them as one batch. The loop stops at the first reader whose close() raises, which leaves every later reader open, both when a request ends and when validation fails partway. An ExitStack calls every registered __exit__ even when some of them raise, and it also takes over the try/except BaseException cleanup.
Co-authored-by: Chuck Daniels <cjdaniels4@gmail.com>
Co-authored-by: Chuck Daniels <cjdaniels4@gmail.com>
test_backend_closes_readers_when_validation_fails makes the same two failing constructions with the same expected exceptions, and also checks the readers get closed, so test_backend_rejects_unreadable_and_incompatible_sources no longer adds coverage. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-authored-by: Chuck Daniels <cjdaniels4@gmail.com>
@chuckwondo do you want to always be pinged again after I address all your comments for another review? i.e., should we continue the cycle until we get to a ✅? The alternative is for me to use my judgement based on the scope of your initial review and how much the PR has changed, and sometimes ask for another review and other times go ahead with merging. |
I'm always happy to take another pass. As I mentioned earlier, I'd prefer taking more time up front rather than rushing something out the door. For what we've done so far for this one, I'm also happy for you to use your best judgement at this point. |
awesome, thanks. Well, I do like to see a ✅ and since you don't mind, I'll stick with the loop 😄 |
chuckwondo
left a comment
chuckwondo
left a comment
There was a problem hiding this comment.
Thanks @maxrjones! I'll leave it up to you as to whether or not you want to apply the perf. gain in the gist I linked, or do it separately (or simply wait for rio-tiler to settle #1022 before bumping).
that rio-tiler maintainer seems super responsive, so I'm happy to wait 😆 |
main now opens each dataset once per request (#171), so /tiles, /point and /info?show_times=true pin 1 repository and 1 dataset open and 9 (or 8) metadata requests instead of 2 and 17 (16). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This PR keeps readers open for the duration of the backend, to be shared for both validation and rio-tiler's calls. It also makes it so URLs provided multiple times to the mosaic route share one reader.
This is motivated by poor performance observed in the AIR4US portal, which Slesa tracked to multiple openings of the same dataset. This is expensive in high-latency environment, especially for Icechunk stores where opening is a more expensive operation.
Separately, we could explore sharing open datasets across requests, but that requires a more comprehensive solution.
Testing
PR checks
run-cdk-checkslabel to this PR.deploy-devlabel. It smoke-tests tiles from the native MUR, virtual MUR, and virtual NLDAS Icechunk stores after deployment.