Repository navigation
Reduce memory needed to read VOTable BINARY and BINARY2 streams - #20509
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
89c4508 to
8d79923
Compare
I don't think "addresses" does anything magical, you might want to change it to "fixes" or "closes" instead (singular works too). |
|
I recall stvoutsin did some recent work on BINARY2, so I pinged him for review. Thanks, all! |
This comment was marked as resolved.
This comment was marked as resolved.
Actually, you're right. It fixes the main issue - memory use. While doing the fix though I found something else. I will file a separate issue and keep things simple. There's probably some more gains to be had for memory use, but those can be follow-ups and the issue closed. |
|
Looks like our benchmarks not catching this improvement. Ah, well. |
Would it be good to add a benchmark? Can you point me in the right direction? |
|
Benchmarks are in https://github.com/astropy/astropy-benchmarks and @stvoutsin had added some in the past (https://github.com/astropy/astropy-benchmarks/pulls?q=is%3Apr+state%3Amerged+votable), so I thought maybe it would show. 🤷♀️ |
|
Looking at the votable benchmarks, they are all I see there are memory benchmarks for some modules, just none for votable. I can add one and see if it shows the difference! |
|
Ok, so I added a peak memory usage benchmark for votable parse. See astropy/astropy-benchmarks#158 And when I run locally: 🎉 |
|
Thanks for this, the memory drop looks great! I compared main and this branch on all the VOTables in io/votable/tests/data that I could read, and the tables and metadata come out identical. The one difference I found is a side effect of clearing the text buffer at each end event. The position reported for a parent's end event now points near its own closing tag instead of reusing the position of the last child's text. I think the new position is actually more accurate, and it only shows up in warnings and validation output, but is it perhaps worth adding a quick line in the changelog about it? Also since Also a very very minor thing, the numbers in 20509.perf.rst (16 -> 6.5 GB) don't match the ones in the PR description (17 -> 6.8 GB). |
It's even better than the 47% above, as both the peak memory measurements include the python interpreter, astropy, numpy asv, etc overhead in both the numerator and denominator. See the
A nice side effect! Though I note that it really just puts it on the line of the closing tag. "TABLE specified nrows=18949, but table contains 1 rows". And it works here because the table is nicely formatted. Whereas if the input file had it would still point to AA, as on main. In the case of this warning, you'd probably prefer it to point to the
I added a blurb there, though I tried to be non-committal. 😅
Agree. I can change from bugfix to api if you think this function is user-facing enough. I'll leave that up to those more knowledgable than myself. And not just end-user-facing, as I know pyvo is built on top of our parser.
Yes, that's a relic of how I was recording the memory usage depending on how it was measured. dividing by 1024 or 1000. The PR message (divide by 1,000,000,000 to get GB) and the perf changelog are now consistent. Though I'm not terribly happy with the changelog, as it's weirdly specific given the file that spawned that issue and this PR. I would much rather just give a bland "parser uses 47% (or is it 38%) of the memory it used to". |
|
So I updated the benchmark to measure the memory usage of just No longer is the whole python interpreter stack included in the numerator and denominator. So for medium to large tables, this PR uses 25 to 40% of the memory as previously, depending on columns. I will updated the |
19d405e to
f608151
Compare
|
Re: bugfix vs api -- The position change in validation output is unlikely to fail any downstream test, right? But one more api fragment won't hurt. Go for it. Thanks! |
|
I've added an API changes snippet. 🚀 |
|
Yeah, a rebase would rerun it. And I agree we should. Thanks! |
df838ff to
d315587
Compare
|
Rebased. Looks like the benchmark picked it up 🚀 |
|
So, I ran the downstream tests via tox of astroquery and pyvo for this branch: This branch does break one doctest downstream in And that is specifically because pyvo currently depends on the closing tag values that is fixed up here in this PR. But a fix on the pyvo side will work with both this branch and main (and previous astropy releases). Essentially pyvo relies on our buggy implementation of the parser, but it doesn't need to. This also argues strongly for a Here's the CI cost when I run them locally:
|
|
Oh hmm... should we wait till downstream is patched first before merging this? |
- bool(job.jobinfo) depended on the text astropy reports at the jobInfo end tag, which was the text of the last nested element - astropy/astropy#20509 stops repeating that text, so a jobInfo holding only child elements became false and `if job.jobinfo:` skipped it - Add __bool__, true when the element has child elements or text - Add tests for a jobInfo with only child elements and an empty jobInfo
|
The pyvo regression above has been fixed and merged. astropy/pyvo#789 🚀 |
- In get_xml_iterator, the end event of an element that contains other
elements now has empty text. Before, it repeated the text of the last
element inside it. This applies to both the C and pure-Python parsers.
This dramatically reduces memory use for reading large XML files.
- For a VOTable BINARY or BINARY2 table, the end events of BINARY2, DATA
and TABLE elements no longer each hold a full copy of the base64
data stream.
- The C parser frees a text buffer that has grown past 1 MB once its
text is handed over, instead of keeping it until parsing ends.
- VOTable reading decodes the stream with binascii.a2b_base64 directly,
which skips a full copy of the stream text. Base64 text is always
ASCII, so valid files decode exactly as before. Only the error for a
malformed stream with a non-ASCII character changes: it is now
ValueError ("string argument should contain only ASCII characters")
instead of UnicodeEncodeError, which is a ValueError subclass.
- Reading the 2.5 GB file from issue astropy#8946 now needs about 6.8 GB of
memory at its peak, down from about 17 GB.
- Update the one test that expected the repeated text, which was just
a coverage test pinning current behavior from PR astropy#19922.
- Add tests for both parsers and for elements holding more than 1 MB
of text.
- Give the memory figures in decimal GB, matching the PR description - Note that end events now report a position near their own closing tag, so VOTable warnings and validation output point at the right line.
d315587 to
3c9155d
Compare
|
aarch64 job should not have taken that long, lemme restart to see if it was transient. |
- bool(job.jobinfo) depended on the text astropy reports at the jobInfo end tag, which was the text of the last nested element - astropy/astropy#20509 stops repeating that text, so a jobInfo holding only child elements became false and `if job.jobinfo:` skipped it - Add __bool__, true when the element has child elements or text - Add tests for a jobInfo with only child elements and an empty jobInfo
Description
Reading a VOTable with an embedded BINARY or BINARY2 stream no longer holds several copies of the stream in memory simultaneously. See issue #8946. In this PR:
</BINARY2>,</DATA>and</TABLE>each held a full copy of the base64 stream.binascii.a2b_base64directly, which skips the full copy made by.encode("ascii").VOTables are read exactly as before. Two small visible differences:
get_xml_iterator, the end event of an element that contains other elements now has empty text, instead of repeating the text of the last element inside it. Nothing in astropy reads that text; one test that recorded the old text is updated. This is where the main memory savings lays.ValueErrornow instead ofUnicodeEncodeError(aValueErrorsubclass). Base64 is always ASCII, so only malformed files are affected. This isn't caught here or anywhere downstream (pyvo or astroquery).Reading the first 1.5 million rows of the file from #8946 (1.26 GB, 96 columns) on macOS:
Peak memory footprint measured via:
The full 2.5 GB table previously needed about 17 GB at its peak, and now needs about 6.8 GB, or 5.1 GB when reading only some of its columns (as described in the issue). The full file parse takes 125 sec on my Mac laptop.
The pure-Python parser
_debug_python_based_parser=Truehas about the same speed for this file, but uses more memory. But the improvement is still good for the half-size file - from 8.9 GB down to 5.2 GB peak memory footprint. So an improvement, but not as much as on the C side with the default parser. It is still holding a copy somewhere. But since it is really not used, not worth spending more time on.Downstream pyvo and astroquery unit tests pass based on this branch. So no downstream changes expected.
Resolves #8946
AI Disclosure
Claude Opus 5.5 was used to explore the existing parser code, find redundant memory usage, demonstrate fixes, verify nothing broke, and write new tests. I have carefully reviewed, modified where needed and tested the changes.
Merge method