ENH: adding minimalist BaseVOQuery baseclass #2836

bsipocz · 2023-09-20T06:41:55Z

Inheriting a lot of unused methods, properties, etc from BaseQuery makes little sense for modules that rely fully on pyvo rather than making their own GET/POST queries.

This PR also ensures that the session set in the class is being passed onto TAPServcie rather than using a new one from pyvo. cadc is basically doing this already, the PR also adds the same approach to alma, nexsci and irsa.

To merge this, I would like to see approvals or thumbs-ups from the affected modules: alma and nexsci

cc @andamian @rickynilsson

codecov · 2023-09-20T06:45:00Z

Codecov Report

Merging #2836 (46d9ed5) into main (6eefe6d) will increase coverage by 0.02%.
The diff coverage is 86.20%.

@@            Coverage Diff             @@
##             main    #2836      +/-   ##
==========================================
+ Coverage   66.49%   66.51%   +0.02%     
==========================================
  Files         235      235              
  Lines       18081    18096      +15     
==========================================
+ Hits        12023    12037      +14     
- Misses       6058     6059       +1

Files	Coverage Δ
astroquery/cadc/core.py	`81.11% <100.00%> (ø)`
astroquery/alma/core.py	`55.57% <66.66%> (ø)`
astroquery/ipac/irsa/core.py	`75.00% <66.66%> (ø)`
...roquery/ipac/nexsci/nasa_exoplanet_archive/core.py	`57.55% <66.66%> (ø)`
astroquery/query.py	`64.23% <94.44%> (+1.68%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

andamian · 2023-09-20T17:37:16Z

Excellent idea. Glad to see the third VO usage in here. The BaseVOQuery class could eventually include auth functionality but that is still being worked on in the IVOA.

@bsipocz - there are a few self._request calls in the cadc.core and alma.core which will stop working. They could probably be replaced with direct requests calls.

bsipocz · 2023-09-20T17:51:35Z

Excellent idea. Glad to see the third VO usage in here. The BaseVOQuery class could eventually include auth functionality but that is still being worked on in the IVOA.

Yes, I'm all for adding all other useful VO related functionality to this base class, but first really just wanted to cut out the clutter as e.g. having all the cache-related things there is more confusing than useful.

bsipocz · 2023-09-20T17:57:41Z

As for alma and cadc and also for the exoplanet_archive: I've seen that they do some non-VO self._request usage, which is all fine as I kept those classes to be subclasses of both the bases, so with this PR both this works and the only functional change for them is the User-Agent has changed and now hopefully consistently sending something like
astroquery/0.4.7.dev8868 pyVO/1.5.dev192+g75bcb73 Python/3.10.8 (Darwin) python-requests/2.28.1

bsipocz · 2023-09-20T18:04:10Z

@andamian - So, as I see, the main question here is whether you see any problems with passing the session from the instances onto pyvo rather than defaulting it using a new one.

andamian · 2023-09-22T22:42:19Z

I would have to review both alma and cadc, more specifically their login methods. At least in cadc they seem to bypass the base session opting for PyVO new authsessions instead. This is where the astroquery header can go missing. That needs to be fixed.
alma on the other hand seems to be using self._session but that it's through self._request (core.py: 292) so it's not authenticating the right session.
Maybe we need to offer login and logout functionality in the BaseVOQuery or at least setter and getters to the session that we can augment with the right agent. The idea would be that the users (or packages such as alma and cadc) create auth sessions and register them with the base class.
A&A is a big subject in IVOA bright now but a clear solution is not yet available so we can only speculate at this point.

bsipocz · 2023-09-22T22:50:55Z

alma on the other hand seems to be using self._session but that it's through self._request (core.py: 292) so it's not authenticating the right session.

But that's the same self._session it inherits from the baseclass, so all should be good, isn't it? (it's just not being passed fwd to pyvo)

andamian · 2023-09-22T23:00:25Z

alma on the other hand seems to be using self._session but that it's through self._request (core.py: 292) so it's not authenticating the right session.

But that's the same self._session it inherits from the baseclass, so all should be good, isn't it? (it's just not being passed fwd to pyvo)

Is it? The session auth is set in self._request() method which belongs to the BaseQuery which has its own _session. So in this case alma is authenticating on BaseQuery._session but using BaseVOQuery._session for PyVO calls, no?

bsipocz · 2023-09-22T23:13:25Z

BaseQuery._session but using BaseVOQuery._session for PyVO calls, no

Indeed, I'll need to double-check the inheritance hierarchy then, and add a test for it. The intention was to use BaseVOQuery._session's only when there is no other one around, it definitely shouldn't override the authenticated ones.

bsipocz

Ahh, I cannot request changes on my own PR 😭

bsipocz · 2023-09-22T23:20:56Z

astroquery/query.py

+    """
+    def __init__(self):
+        super().__init__()
+        self._session = requests.Session()


OK, I suppose putting this into a conditional should be sufficient to address the issue @andamian pointed out.

But it all needs thorough testing.

andamian · 2023-09-22T23:33:17Z

Maybe CadcClass and AlmaClass should only inherit from BaseVOQuery if they are not supposed to find anything else useful in BaseQuery?

It's confusing to have all these sessions flying around- which one is auth, which ones was created externally, etc? So it's nice to have one set in the base class and shared by the implementations. But is that practical? Can one session be shared in a multithread environment (multiple downloads for ex)? It's not very clear whether requests.Session is thread safe psf/requests#2766.

It would be useful I think to add a factory create_session method to the BaseVOQuery for external users to get astroquery prepped sessions. And we'll have to remember to use it all the time in the implementation.

…r VO modules

bsipocz · 2023-09-26T03:15:18Z

Made this a little bit more robust, and now I believe it's good to go.

As for the bigger picture question:

t's confusing to have all these sessions flying around- which one is auth, which ones was created externally, etc? So it's nice to have one set in the base class and shared by the implementations. But is that practical? Can one session be shared in a multithread environment (multiple downloads for ex)?

I don't have a good answer for this, but the current inconsistency feels more ad-hoc than on purpose, and definitely not consistent over astroquery or even within pyvo, so if we switch over from having one shared version we need to do it purposefully and as consistently as possible (and preferably we should leave some doc trail about it ;) )

andamian · 2023-09-26T22:57:26Z

astroquery/query.py

+        else:
+            user_agents = [f"astroquery/{version.version} pyVO/{pyvo.__version__} "
+                           f"Python/{platform.python_version()} ({platform.system()})"] + user_agents
+


This is not potentially repeating the Python version?

Potentially yes, but it's already the case in the current main, I just made sure that we don't do this if we (==astroquery or pyvo) already modified the user-agent. The one corner case that I haven't covered if the session is somehow made in pyvo, and thus only have the pyvo version listed, but I don't think that will ever be the case here.

PyVO might be used to create auth sessions if that is going to be standardized eventually. It already has authsession.AuthSession although that is a bit outdated.

If I see correctly, then authsession.AuthSession doesn't modify the headers. Though yes, I suppose the above logic should better be fixed in case it would modify it.

The latest commit should cover the case when PyVO already modified the user-agent.

Some other tools might have already added the python and platform specifics, too, but I think we should just roll with that and add our own, too, and thus duplicate if that's the case.

bsipocz added the query astroquery internals label Sep 20, 2023

bsipocz added this to the v0.4.7 milestone Sep 20, 2023

keflavich approved these changes Sep 20, 2023

View reviewed changes

bsipocz mentioned this pull request Sep 22, 2023

BUG: SIA2Service.search() method has separate session astropy/pyvo#487

Closed

bsipocz commented Sep 22, 2023

View reviewed changes

bsipocz added 5 commits September 25, 2023 20:10

ENH: Adding BaseVOQuery baseclass to serve as a minimal base class fo…

2455808

…r VO modules

MAINT: passing on self._session to pyvo

2120834

DOC: adding changelog

df1f755

Adding BaseVOQuery to the public API

7a4a7bf

Ensuring we don't override existing sessions

1482981

bsipocz force-pushed the ENH_basevoquery branch from f13444f to 1482981 Compare September 26, 2023 03:10

andamian reviewed Sep 26, 2023

View reviewed changes

FIX: Cover case when pyVO already modified the user-agent

46d9ed5

andamian approved these changes Sep 27, 2023

View reviewed changes

bsipocz merged commit abeabdc into astropy:main Sep 27, 2023

bsipocz deleted the ENH_basevoquery branch December 8, 2023 18:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: adding minimalist BaseVOQuery baseclass #2836

ENH: adding minimalist BaseVOQuery baseclass #2836

bsipocz commented Sep 20, 2023

codecov bot commented Sep 20, 2023 •

edited

Loading

andamian commented Sep 20, 2023

bsipocz commented Sep 20, 2023

bsipocz commented Sep 20, 2023

bsipocz commented Sep 20, 2023

andamian commented Sep 22, 2023

bsipocz commented Sep 22, 2023

andamian commented Sep 22, 2023

bsipocz commented Sep 22, 2023

bsipocz left a comment

bsipocz Sep 22, 2023

andamian commented Sep 22, 2023

bsipocz commented Sep 26, 2023

andamian Sep 26, 2023

bsipocz Sep 26, 2023

andamian Sep 26, 2023

bsipocz Sep 26, 2023

bsipocz Sep 26, 2023

ENH: adding minimalist BaseVOQuery baseclass #2836

ENH: adding minimalist BaseVOQuery baseclass #2836

Conversation

bsipocz commented Sep 20, 2023

codecov bot commented Sep 20, 2023 • edited Loading

Codecov Report

andamian commented Sep 20, 2023

bsipocz commented Sep 20, 2023

bsipocz commented Sep 20, 2023

bsipocz commented Sep 20, 2023

andamian commented Sep 22, 2023

bsipocz commented Sep 22, 2023

andamian commented Sep 22, 2023

bsipocz commented Sep 22, 2023

bsipocz left a comment

Choose a reason for hiding this comment

bsipocz Sep 22, 2023

Choose a reason for hiding this comment

andamian commented Sep 22, 2023

bsipocz commented Sep 26, 2023

andamian Sep 26, 2023

Choose a reason for hiding this comment

bsipocz Sep 26, 2023

Choose a reason for hiding this comment

andamian Sep 26, 2023

Choose a reason for hiding this comment

bsipocz Sep 26, 2023

Choose a reason for hiding this comment

bsipocz Sep 26, 2023

Choose a reason for hiding this comment

codecov bot commented Sep 20, 2023 •

edited

Loading