search

astropy / pyvo

An Astropy affiliated package providing access to remote data and services of the Virtual Observatory (VO) using Python.
https://pyvo.readthedocs.io/en/latest
BSD 3-Clause "New" or "Revised" License
74 stars 50 forks source link

Now using DISTINCT in ivoid-selecting registry subqueries. #572

Closed msdemlei closed 1 week ago

msdemlei commented 1 week ago

This is not only more logical (let's filter out duplicates as early as possible)but experimentally also is enough to keep the planner going off to follies of the type encountered in bug #571.

msdemlei commented 1 week ago

I think no-changelog-entry is roughly right, although I have added the new PR number to the changelog entry about subqueries (that doesn't seem to placate the CI, though).

Anyway, the current test failure appears to be unrelated to the change at hand (though I'd say we should do something about the sia2 tests in general, as they generally take ages). So, I think this is ready to go.

ManonMarchand commented 1 week ago

I was unsure about the changelog. But I guess it can be merged with the failure.

I don't have time to review today but I'll look tomorrow.

These two successive PRs make me wonder how we could detect that a change that was done makes everything slower. Would it be wise to benchmark?

msdemlei commented 1 week ago

On Tue, Jul 02, 2024 at 05:46:43AM -0700, Manon Marchand wrote:

These two successive PRs make me wonder how we could detect that a change that was done makes everything slower. Would it be wise to benchmark?

I'm afraid this is one of the many things where we can't sensibly defend with unit tests or something like them.

The problem here was, at its core, a specific fluke on one specific service. Ok, it's the default one we use, but still: if we try to even diagnose performance regressions, we will have useless test suites because (a) they'll take forever to finish and (b) they'll only pass once every blue moon because the more services you hit the more certain it becomes that at least one of them will be down or misbehaving at any time.

I'm still all for a wider array of tests, perhaps also human-executed tests (I'd be in for checking a notebook or two), to be executed before we do a release. This would have caught this problem, but frankly, that would mainly have been sheer luck, too.

bsipocz commented 1 week ago

I was unsure about the changelog. But I guess it can be merged with the failure.

adding the PR number is certainly the preferred way for the changelog for this. Or calling it "affect-dev", and when that label present the checker expects no changelog entry.

The reason for the CI job to fail was the missing milestone though.

bsipocz commented 1 week ago

(though I'd say we should do something about the sia2 tests in general, as they generally take ages)

That's temporary and is due to cadc experiencing some server side issues.

andamian commented 1 week ago

I think that the extra resources required by DISTINCT (temporary table to sort and remove duplicates) are saved with UNION ALL so overall the impact on the database is small.