-
-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Backing out of requesting alt_identifiers every time. #523
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #523 +/- ##
==========================================
- Coverage 80.33% 79.71% -0.62%
==========================================
Files 52 52
Lines 6177 6178 +1
==========================================
- Hits 4962 4925 -37
- Misses 1215 1253 +38 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a changelog entry?
And unless @andamian or @tomdonaldson wants to have a look at this we can merge it.
This is supposed to address bug astropy#522. The code as it was up to here would have needed aggregation of alt_identifiers (which are n:1 over resources), or else we see duplicate capabilities. But at least some registry operators prefer to not hit the rr.alt_identifier table by default as long as it's not clear who will actually look at these alternate identifiers. But we maintain the alt identifiers in describe(); to do that, there's now get_alternate_identifiers method returning these. The downside: describe() now does an uncached network query. Perhaps we want to at least hide failures from there? On the other hand, once we are here we can also call get_contact() here; should we?
2e8a828
to
095b734
Compare
On Thu, Feb 15, 2024 at 06:32:56AM +0000, Brigitta Sipőcz wrote:
@bsipocz commented on this pull request.
Could you please add a changelog entry?
Ah, sorry. I had deluded myself into believing that hadn't been in
released and then failed to see that even then, I'd have to do
something about the changelog.
Force-pushed a commit including the changelog.
|
LGTM, thanks Markus! About adding get_contact in describe. From a user point of view, I'd use get_contact() only in case of issues/bugs with the service, it's not something I'd like to see all the time. |
So... the CI doesn't like my indentation in
-- which, frankly, I consider severely odd. Yes, this might be part of So... can we perhaps haggle about dropping another pep8 check? |
Oh, that may be my bad, I haven't remembered this. |
Let me see what it does when removing the linebreak in format. If it still complains then I'll go ahead and add this check to the ignore list. |
@msdemlei, how about this? I find this one more readable than your version with the nested indents, and apparently, the linter likes it too.
|
On Thu, Feb 15, 2024 at 10:03:52AM -0800, Brigitta Sipőcz wrote:
@msdemlei, how about this? I find this one more readable than your
version with the nested indents, and apparently, the linter likes
it too.
(I personally like f-strings a lot, but also understand if you prefer not to use one in this one).
```
res = get_RegTAP_service().run_sync("""
SELECT alt_identifier
FROM rr.alt_identifier
WHERE ivoid={}""".format(rtcons.make_sql_literal(self.ivoid)))
```
Readability necessarily is a matter of taste. Here, I frankly can't
really see the rationale of the whitespace checker, and I'd slightly
prefer if we could avoid mixing nontrivial SQL and Python on one
line.
But then... I think people can easily figure out what's going on in
either version, and the lines are shorter than 80 characters (which I
do care about; longer lines *really* spoil a lot of things in my
workflows). So, I'm happy with this.
Thanks!
|
that is good to know about, I'll try to remember to be more strict in my codes for long lines. |
Once we get this one and #416 in, I'm happy to tag a bugfix release. What do you think? |
thank you @msdemlei! |
On Thu, Feb 15, 2024 at 11:13:01PM -0800, Brigitta Sipőcz wrote:
Once we get this one and #416
in, I'm happy to tag a bugfix release. What do you think?
I'd really be grateful.
And I apologise again for not having realised that something is amiss
before the release. This brings me back go sketching some
pre-release exercise to try a representative sample of jupyter
notebooks as considered in #304. If someone has a good example of
where folks have a reasonable system where they run notebooks as
integration tests, I might try my hands...
|
I do run notebooks in CI (we both have pyvo focusing navo notebooks as well as other IRSA notebooks), but arguably these don't cover much of pyvo's codebase (what they cover and uncover I tend to open issues for). CDS also has very nice notebooks, and as more archives switch to use the pyvo backend in astroquery I expect a bit more coverage from there, too. As for #304, I still think it's not our (==pyvo maintainers') job to test third-party notebooks as part of the release process, but I do think we can help promote CI practices (or oversee such CIs in our other capacities, like I do with the navo and irsa notebooks) and e.g. help setting up cron jobs that run the notebooks regularly, or before releases. I was chattering about tagging the 1.5 release on the astropy slack, but I don't think it reaches the relevant people. Would e.g. announcing on the ivoa slack be preferable? We could do that ~a week before releasing, at least when doing a non-bugfix release. |
We have a CI for notebooks here: https://github.com/cds-astro/tutorials
The big flaw is that it only makes sure that they run, not that the output did not change. I'm exploring solutions for that. |
we do use |
Thanks! Don't know how I missed that, looks awesome. |
We do have some plans to improve this part of the ecosystem, but it's not happening overnight (but we know eventually there would be a lot of users, at least for the small tutorial/snippet like notebooks (not for the long demo ones that require a lot of exotic dependencies and are expected to run for many hours to complete) |
I'm happy to give a tour about it at the ivoa if it would be useful for more groups. |
Backing out of requesting alt_identifiers every time.
Backing out of requesting alt_identifiers every time.
This is supposed to address bug #522. The code as it was up to here would have needed aggregation of alt_identifiers (which are n:1 over resources), or else we see duplicate capabilities.
But at least some registry operators prefer to not hit the rr.alt_identifier table by default as long as it's not clear who will actually look at these alternate identifiers.
But we maintain the alt identifiers in describe(); to do that, there's now get_alternate_identifiers method returning these.
The downside: describe() now does an uncached network query. Perhaps we want to at least hide failures from there? On the other hand, once we are here we can also call get_contact() here; should we?