Repository navigation
Add Cython implementation for VOTable binary converters - #18454
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.
|
82cd430 to
1dfea18
Compare
|
|
||
| format = "f8" | ||
|
|
||
| def binparse(self, read): |
There was a problem hiding this comment.
Seems a little repetitive across the board. Looks like the only difference is a number. Can this method be inherited but access some self._expected_len (name negotiable) that is set by the subclass?
There was a problem hiding this comment.
Great suggestion, looking back at it now I can see there is a lot of duplication. I've moved things up to the parent Numeric class, let me know if that looks good. (Or if you see any adjustments needing to be made)
There was a problem hiding this comment.
Once we have the benchmark down, should maybe put some ballpark values here.
pllim
added
Performance
Extra CI
|
Hmm looks like @eerovaher proposed an alternative at #18455 . Are you interested to have a look, @stvoutsin ? Thanks, all! |
|
astropy/astropy-benchmarks#141 is merged so theoretically next push would trigger a new benchmark run with the new relevant benchmarks. 🤞 |
ba78237 to
1babe2e
Compare
|
Hmm benchmark job says nothing has "significantly changed". Is this expected? I thought it would say "this and that is much faster" or something. |
Apologies I think i introduced a regression while trying to fix an issue that caused failed tests in the CI. I will also test with increasing the row sizes for the sample data to see how the runtime difference between main and this branch scales. |
|
I've run the astropy-benchmarks locally with 200k, 500k and 1M sample row sizes (https://github.com/astropy/astropy-benchmarks/blob/main/benchmarks/votable.py#L13). Results shown here: u/stvoutsin/binary2-cython vs main branch performance comparison
|
350e752 to
96b3904
Compare
|
Benchmark run looking good. Thanks!
SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY. |
|
Any thoughts on possibly moving this forward? |
|
Hello. Let me ping PyVO people. Thank you for your patience! |
8d62fcd to
cb86c53
Compare
00f2d1c to
fb6574a
Compare
@bsipocz I've removed the bitarray converters as the benchmarks showed that they were slower then the merged-in version. The most recent benchmarks show some improvements to the rest of the tests for what it's worth:
SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY. |
fb6574a to
2d98012
Compare
|
@stvoutsin - could you please rebase this so we get fresh CI status? I'll try to review before feature freeze. [never mind, it looks like we can now do that from the UI] |
2d98012 to
1cdb43d
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
To be specific:
|
1cdb43d to
f06284a
Compare
f06284a to
a970d56
Compare
|
@bsipocz are we merging this in for v8.0.0 ? Looks good to you now? |
bsipocz
left a comment
bsipocz
left a comment
There was a problem hiding this comment.
OK, I think this is good to go. I tried to review the changes, especially where it's not just the addition but the removal of existing python lines; but didn't run any additional round tripping tests, but knowing that @stvoutsin will be a user of these I trust any problems will eventually smoked out.
Minor comment is not a blocker for merging.
| elif byte_val == 0 or byte_val == ord(b' ') or byte_val == ord(b'?'): | ||
| value = False | ||
| is_null = True | ||
| else: | ||
| value = False | ||
| is_null = True |
There was a problem hiding this comment.
why do we need this elif? I mean I see that in the non cython version we have the mapping for these, but here maybe it's not needed?
There was a problem hiding this comment.
Oh yea that looks unnecessary, thanks for spotting. I'll remove in a quick follow-up PR.
There was a problem hiding this comment.
Seems simple enough that I already opened #19664 so we don't forget about this before RC. 😅 Please review, thanks!
|
Thanks @stvoutsin again and also thanks for being patient with this. |
Cool thanks for reviewing, and indeed we do rely on this so we're happy to take up any work needed to support this! |
|
The main concern was to see easier ways to spot that there aren't any regressions/changes in the actual data values. But then we decided that you are the best person to test/benchmark that so here we're 😄 |
|
Please also review #19686 to ignore the C file that this PR generates. Thanks! |
Description
This pull request attempts to improve the performance of VOTable binary parsing by implementing optimized Cython converters.
Initial benchmarks have shown performance improvements of the order:
Problem:
We’re seeing relatively slow performance when parsing large VOTables using the BINARY2 serialization format.
Profiling results from py-spy show a significant portion of time is spent in astropy.io.votable.converters.binparse. Here’s an excerpt of the profiler output:
As shown, the hot path is dominated by the binparse function in converters.py.
Solution:
double,float,int,long,short,unsignedByte,boolean,bit)Compatibility:
Should be 100% backward compatible since the API has not been modified
Testing:
Added comprehensive benchmarks to astropy-benchmarks showing consistent 30-50% performance improvements across different table types and data patterns. astropy/astropy-benchmarks#141
Astropy:main
u/stvoutsin/binary2-cython
For better visualization:
I've also tested parsing outside of the astropy-benchmark PR with a VOTable with 50000 rows and aprox. 1000 columns (~50 million cells).
With this table the parsing took 30 seconds, down from 65 seconds in the version currently on main.
Fixes #18442
Any thoughts on this approach? Are there alternatives that I haven't considered? Perhaps someone with more Cython experience can let me know if I've made any obvious mistakes or if there are better way of doing any of this.