Skip to content

ESA: abstract EsaTap using PyVO, HST and ISLA refactored, EMDS module and EinsteinProbe module#3511

Merged
bsipocz merged 31 commits intoastropy:mainfrom
esdc-esac-esa-int:ESA_isla-emds-epsa_improvements
Mar 31, 2026
Merged

ESA: abstract EsaTap using PyVO, HST and ISLA refactored, EMDS module and EinsteinProbe module#3511
bsipocz merged 31 commits intoastropy:mainfrom
esdc-esac-esa-int:ESA_isla-emds-epsa_improvements

Conversation

@jespinosaar
Copy link
Copy Markdown
Contributor

Dear Astroquery team,

We come from PR 3501: #3501

As part of the migration to PyVO, we have generated a more general class called EsaTap, extending PyVO capabilities with custom code (authentication, parameters in the request, methods to query tables with specific filters...). We will be using this class to extend ESA modules, so the common code will remain under EsaTap and the mission-specific methods will be defined in their specific modules. As a reference, we have already applied this approach to eHST and ISLA.

In addition to this, we have generated a new module called ESDC Multi-Mission Data Services (EMDS). Then, inside it, we have generated the module for Einstein Probe module, that depends on the EMDS one.

Please let us know if this is ok with you.

Happy to receive your feedback and implement any change you require!

Kind regards,
@jespinosaar and @pdmElisa

cc @esdc-esac-esa-int

@jespinosaar
Copy link
Copy Markdown
Contributor Author

Great, so after some iterations it seems that the only pending things are unrelated errors (it seems there are several deprecation warnings there...). So, we are ready for the review, @bsipocz .

And thanks @pdmElisa for your contributions!

@jespinosaar
Copy link
Copy Markdown
Contributor Author

Thanks for the updates @pdmElisa !

@jespinosaar
Copy link
Copy Markdown
Contributor Author

It seems that the workflows are waiting for the approval of a maintainer. I cannot see any button to launch them, @bsipocz . Can you please execute them to verify that the changes are ok?

@jespinosaar
Copy link
Copy Markdown
Contributor Author

Hi @bsipocz ! We are in the process of creating additional PRs from other ESA modules soon, based on the code included in this branch. Sorry for the rush, but it would be great if we could iterate on it and try to merge it as soon as possible.

FYI, these future PRs will include a new module, and we will request a formal release by last week of March / first week of April.

@pdmElisa and myself will fix quickly the changes you request.

@bsipocz
Copy link
Copy Markdown
Member

bsipocz commented Feb 14, 2026

@pllim - could you please add @pdmElisa to the astropy contributors team so we can avoid the need of manual workflow approval?

@bsipocz
Copy link
Copy Markdown
Member

bsipocz commented Feb 14, 2026

@jespinosaar - I'll try to get back to this one next week. The diff looks enormous, so it hasn't got to the top of the quick-things-to-review list yet.

@bsipocz
Copy link
Copy Markdown
Member

bsipocz commented Feb 14, 2026

(But I also noticed looking at the cron test jobs that the current integral module is very badly broken, so that definitely bumped up the priority on this one)

@jespinosaar
Copy link
Copy Markdown
Contributor Author

jespinosaar commented Feb 16, 2026

Hi @bsipocz!
Many thanks for your support!
Some of the ESA Archives are not available right now, this is why you will see some remote tests failing (e.g. ISLA, HST or XMM). The rest of them are up and running. We are working to make them available again as soon as possible. If it is ok with you, let's have a look at the content of this pull request in the meantime, as we can validate EMDS and Einstein-Probe module. Many thanks!

@jespinosaar jespinosaar force-pushed the ESA_isla-emds-epsa_improvements branch from 213f26d to 83ffebe Compare February 25, 2026 14:45
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 25, 2026

Codecov Report

❌ Patch coverage is 81.09756% with 62 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.10%. Comparing base (e49f51a) to head (dbf41fd).
⚠️ Report is 98 commits behind head on main.

Files with missing lines Patch % Lines
astroquery/esa/utils/utils.py 73.68% 40 Missing ⚠️
astroquery/esa/emds/core.py 77.02% 17 Missing ⚠️
astroquery/esa/emds/einsteinprobe/core.py 92.72% 4 Missing ⚠️
astroquery/esa/integral/core.py 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3511      +/-   ##
==========================================
+ Coverage   72.66%   73.10%   +0.43%     
==========================================
  Files         219      224       +5     
  Lines       20478    20839     +361     
==========================================
+ Hits        14880    15234     +354     
- Misses       5598     5605       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jespinosaar
Copy link
Copy Markdown
Contributor Author

Dear @bsipocz ,

I have rebased this PR and added some changes to increase code-coverage and ensure sphinx is working as expected. In addition to this, the Archives are available again, so you can now execute all the remote tests.

As we already commented, it would be good if we could review this soon, as we will be using this code to generate other modules as well. We will be waiting for your feedback.

Many thanks for your patience! 😄

@jespinosaar
Copy link
Copy Markdown
Contributor Author

jespinosaar commented Mar 3, 2026

Dear @bsipocz ,

We would like to merge now another module from a different mission. This should be available at the end of this month (presumably with a released version, if not we can work with a pre-released). The problem here is that we are basing this development on what we have prepared for this PR. How should we proceed? Do you prefer to have this merged and then merge the new module in a different PR? Or should I include that code in this PR? Thanks in advance and sorry for insisting!

@jespinosaar
Copy link
Copy Markdown
Contributor Author

Hi @bsipocz , @adamginsburg ,
As I commented before, we need this PR to be merged in other to create a new one, containing another module. And we wanted to have it merged by the end of this month. But please, let us know if this cannot be done soon, due to the complexity of this PR and your procedures, so we can plan an alternative.

@jespinosaar
Copy link
Copy Markdown
Contributor Author

Hi @bsipocz , @adamginsburg ,
Sorry for insisting several times, but this PR has been opened for more than one month. I just need to know whether it would feasible to have this PR merged soon and so the additional one for the other mission I have already commented (as reference, it would be something similar to the EinsteinProbe module, and it is already developed on our side). Could you please let me know? Even a brief update would be very helpful. Thank you in advance.

Kind regards,
jespinosaar

Copy link
Copy Markdown
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before starting a detailed review I noticed that there are lots of test failures. Could you please have a look at them? Here is a very brief summary; I haven't yet look into the tracebacks.

pytest astroquery docs -P esa.emds,esa.hubble,esa.integral,esa.utils,esa.xmm_newton -R
================================================= test session starts ==================================================
platform darwin -- Python 3.12.1, pytest-9.0.2, pluggy-1.6.0
cachedir: .tox/test-online/.pytest_cache

Running tests with astroquery version 0.4.12.dev356+gf09e2fb37_testrun.
Running tests in astroquery docs.

Date: 2026-03-17T11:28:39

Platform: macOS-12.5.1-arm64-arm-64bit

Executable: /Users/bsipocz/munka/devel/astroquery/.tox/test-online/bin/python

Full Python Version: 
3.12.1 (main, Dec 22 2023, 11:59:08) [Clang 14.0.0 (clang-1400.0.29.202)]

encodings: sys: utf-8, locale: UTF-8, filesystem: utf-8
byteorder: little
float info: dig: 15, mant_dig: 15

Package versions: 
Numpy: 2.4.3
Matplotlib: 3.10.8
Astropy: 7.2.0
regions: not available
pyVO: 1.8.1
mocpy: not available
astropy-healpix: not available
vamdclib: not available

Using Astropy options: remote_data: any.

rootdir: /Users/bsipocz/munka/devel/astroquery
configfile: pyproject.toml
plugins: astropy-0.11.0, mock-3.15.1, remotedata-0.4.1, timeout-2.4.0, custom-exit-code-0.3.0, filter-subpackage-0.2.0, astropy-header-0.2.2, hypothesis-6.151.9, doctestplus-1.7.1, arraydiff-0.6.1, dependency-0.6.1, rerunfailures-16.1, cov-7.0.0
timeout: 300.0s
timeout method: signal
timeout func_only: False
collected 240 items                                                                                                    

../../.tox/test-online/lib/python3.12/site-packages/astroquery/esa/emds/einsteinprobe/test/test_einsteinprobe_remote.py . [  0%]
...FFFFFF                                                                                                        [  4%]
../../.tox/test-online/lib/python3.12/site-packages/astroquery/esa/emds/einsteinprobe/test/test_einsteinprobe_tap.py . [  4%]
....................                                                                                             [ 12%]
../../.tox/test-online/lib/python3.12/site-packages/astroquery/esa/emds/tests/test_emds_remote.py .....FFFFFFE   [ 17%]
../../.tox/test-online/lib/python3.12/site-packages/astroquery/esa/emds/tests/test_emds_tap.py ................. [ 24%]
..                                                                                                               [ 25%]
../../.tox/test-online/lib/python3.12/site-packages/astroquery/esa/hubble/tests/test_esa_hubble.py ............. [ 30%]
.................................                                                                                [ 44%]
../../.tox/test-online/lib/python3.12/site-packages/astroquery/esa/hubble/tests/test_esa_hubble_remote.py ...... [ 47%]
.........                                                                                                        [ 50%]
../../.tox/test-online/lib/python3.12/site-packages/astroquery/esa/integral/tests/test_isla_remote.py .....F..F. [ 55%]
.F                                                                                                               [ 55%]
../../.tox/test-online/lib/python3.12/site-packages/astroquery/esa/integral/tests/test_isla_tap.py ............. [ 61%]
.............E..........                                                                                         [ 71%]
../../.tox/test-online/lib/python3.12/site-packages/astroquery/esa/utils/tests/test_utils.py ................... [ 79%]
...........                                                                                                      [ 83%]
../../.tox/test-online/lib/python3.12/site-packages/astroquery/esa/xmm_newton/tests/test_xmm_newton.py ......... [ 87%]
.................                                                                                                [ 94%]
../../.tox/test-online/lib/python3.12/site-packages/astroquery/esa/xmm_newton/tests/test_xmm_newton_remote.py F. [ 95%]
......                                                                                                           [ 97%]
../../docs/esa/emds/einsteinprobe/einsteinprobe.rst F                                                            [ 98%]
../../docs/esa/emds/emds.rst F                                                                                   [ 98%]
../../docs/esa/hubble/hubble.rst F                                                                               [ 99%]
../../docs/esa/integral/integral.rst F                                                                           [ 99%]
../../docs/esa/xmm_newton/xmm_newton.rst F                                                                       [100%]

======================================================== ERRORS ========================================================

@jespinosaar
Copy link
Copy Markdown
Contributor Author

jespinosaar commented Mar 18, 2026

Hi @bsipocz , and many thanks for looking into this and your support.

I have modified the tests and the documentation, and now everything seems to be working as expected.

pytest --pyargs astroquery docs -P esa.emds,esa.hubble,esa.integral,esa.utils,esa.xmm_newton -R

========================================================================================================== test session starts ==========================================================================================================
platform darwin -- Python 3.11.14, pytest-9.0.2, pluggy-1.6.0

Running tests with astroquery version 0.4.12.dev358+g741ec3ca3.d20260318_testrun.
Running tests in astroquery docs.

Date: 2026-03-18T14:41:14

Platform: macOS-26.2-arm64-arm-64bit

Full Python Version: 
3.11.14 (main, Oct 21 2025, 18:27:30) [Clang 20.1.8 ]

encodings: sys: utf-8, locale: UTF-8, filesystem: utf-8
byteorder: little
float info: dig: 15, mant_dig: 15

Package versions: 
Numpy: 2.4.3
Matplotlib: 3.10.8
Astropy: 7.2.0
regions: not available
pyVO: 1.8.1
mocpy: not available
astropy-healpix: not available
vamdclib: not available

Using Astropy options: remote_data: any.

rootdir: /Users/git/astroquery
configfile: pyproject.toml
plugins: astropy-0.11.0, mock-3.15.1, remotedata-0.4.1, timeout-2.4.0, filter-subpackage-0.2.0, astropy-header-0.2.2, hypothesis-6.151.9, doctestplus-1.7.1, arraydiff-0.6.1, requests-mock-1.12.1, cov-7.0.0
timeout: 300.0s
timeout method: signal
timeout func_only: False
collected 240 items                                                                                                                                                                                                                     

esa/emds/einsteinprobe/test/test_einsteinprobe_remote.py ..........                                                                                                                                                               [  4%]
esa/emds/einsteinprobe/test/test_einsteinprobe_tap.py .....................                                                                                                                                                       [ 12%]
esa/emds/tests/test_emds_remote.py ...........                                                                                                                                                                                    [ 17%]
esa/emds/tests/test_emds_tap.py ...................                                                                                                                                                                               [ 25%]
esa/hubble/tests/test_esa_hubble.py ..............................................                                                                                                                                                [ 44%]
esa/hubble/tests/test_esa_hubble_remote.py ...............                                                                                                                                                                        [ 50%]
esa/integral/tests/test_isla_remote.py ............                                                                                                                                                                               [ 55%]
esa/integral/tests/test_isla_tap.py .....................................                                                                                                                                                         [ 71%]
esa/utils/tests/test_utils.py ..............................                                                                                                                                                                      [ 83%]
esa/xmm_newton/tests/test_xmm_newton.py ..........................                                                                                                                                                                [ 94%]
esa/xmm_newton/tests/test_xmm_newton_remote.py ........                                                                                                                                                                           [ 97%]
docs/esa/emds/einsteinprobe/einsteinprobe.rst .                                                                                                                                                                                   [ 98%]
docs/esa/emds/emds.rst .                                                                                                                                                                                                          [ 98%]
docs/esa/hubble/hubble.rst .                                                                                                                                                                                                      [ 99%]
docs/esa/integral/integral.rst .                                                                                                                                                                                                  [ 99%]
docs/esa/xmm_newton/xmm_newton.rst .                                                                                                                                                                                              [100%]

==================================================================================================== 240 passed in 401.42s (0:06:41) ====================================================================================================

Could you please test it again? And let me know if you find more issues. Many thanks!

Copy link
Copy Markdown
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @jespinosaar of the continued patience with this.

I have a number of review comments, most of them about API consistency and namespaces., and a couple of minor nitpicking items that doesn't need to be addressed.

The only blockers is to iterate on the names/namespaces -- you don't have to follow my suggestions but I would love to see your thinking on them.

super().__init__(auth_session=auth_session, tap_url=tap_url)
self.conf = conf

def get_products(self, obs_id=None, *, columns=None, custom_filters=None,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some API consistency could help here I think. We do have get_product() elsewhere (e.g. euclid and jwst modules), and agruments are called observation_id or product_id or artifact_id. Could we have some renaming to have a bit more consistency?

Also, in the other similar methods all optional arguments are mandatory kwargs, so please move the * one place earlier

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method aims to retrieve a list of products, this is why it is called get_products. If you want to keep the consistency, I can change it to get_product, but I think this may be less intuitive. I would like to know your opinion on this.

The * has been moved.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, let's keep the plural then.

But I think the parameter renames should still be considered for better consistency.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have renamed obs_id to observation_id

Comment on lines +76 to +79
elif isinstance(columns, list):
for r in required:
if r not in columns:
columns.append(r)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bit overoptimisation, but what about making columns (internally) a set, and then you can just add required instead of looping over?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have applied your suggestion

# Build an obs_id filter if provided
obs_filter = None
if obs_id:
obs_filter = "obs_id = '{0}'".format(obs_id)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
obs_filter = "obs_id = '{0}'".format(obs_id)
obs_filter = f"obs_id = '{obs_id}'"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I applied your suggestion

REQUEST_PARAMETERS = {}
"""

def __init__(self, auth_session=None, tap_url=None):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def __init__(self, auth_session=None, tap_url=None):
def __init__(self, *, auth_session=None, tap_url=None):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The asterisk has been added


Returns
-------
A pyvo.dal.TAPService object connected to {ESA_ARCHIVE_NAME} TAP
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
A pyvo.dal.TAPService object connected to {ESA_ARCHIVE_NAME} TAP
A `~pyvo.dal.TAPService` object connected to {ESA_ARCHIVE_NAME} TAP

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added your suggestion


def __create_sql_criteria(self, filters):
"""
Create the SQL clause associated to the query_criteria filters
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this vanilla SQL or ADQL? if the latter, please rename

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is an ADQL query to the TAP, I have renamed it

"""
# For exact searches in not integer values, check the number is
# within a threshold of 1e-5
threshold = 1e-5
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could/should this threshold be configurable?

(I really just wonder, even if it could be, no need to address or change in this PR but instead leave it for a possible future enhancement)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have modified it as a parameter for the class, so it can be configured when instantiating it.

@jespinosaar
Copy link
Copy Markdown
Contributor Author

I think I have addressed all your comments, @bsipocz . Please let me know if further changes need to be integrated.

With respect to the importance of the EMDS, I think it is important to keep it as a separate module, as this will be the entry point for different archives, providing a multi-mission approach. We had the same discussion internally, about einsteinprobe being inside emds or as an extra module inside "esa" folder. We finally added Einstein Probe inside this module, to show that this mission is part of this infrastructure.

If, at some point, a mission with a existing module under esa folder is migrated to EMDS, we will discuss with you on the best approach. Maybe we can keep it separately for existing modules. But, for sure, we will keep you informed.

@bsipocz
Copy link
Copy Markdown
Member

bsipocz commented Mar 26, 2026

With respect to the importance of the EMDS, I think it is important to keep it as a separate module, as this will be the entry point for different archives, providing a multi-mission approach. We had the same discussion internally, about einsteinprobe being inside emds or as an extra module inside "esa" folder. We finally added Einstein Probe inside this module, to show that this mission is part of this infrastructure.

OK, that sounds reasonable, we definitely need to keep emds as is; but I am still not fully sold on not having an astroquery.esa.einsteinprobe; I mean the end user doesn't have to know the internal structure. But let's not block the merge with this, we can still reshuffle namespaces before the actual release is tagged. (Or can do what we did with the solarsystem modules, expose them at two different namespaces).

@jespinosaar
Copy link
Copy Markdown
Contributor Author

With respect to the importance of the EMDS, I think it is important to keep it as a separate module, as this will be the entry point for different archives, providing a multi-mission approach. We had the same discussion internally, about einsteinprobe being inside emds or as an extra module inside "esa" folder. We finally added Einstein Probe inside this module, to show that this mission is part of this infrastructure.

OK, that sounds reasonable, we definitely need to keep emds as is; but I am still not fully sold on not having an astroquery.esa.einsteinprobe; I mean the end user doesn't have to know the internal structure. But let's not block the merge with this, we can still reshuffle namespaces before the actual release is tagged. (Or can do what we did with the solarsystem modules, expose them at two different namespaces).

Ok, maybe we can have a longer discussion later, yes, and decide the best approach. Many thanks! In the meantime, I think all the comments are now addressed

@jespinosaar
Copy link
Copy Markdown
Contributor Author

Just added a minor improvement, to verify if a radius has been defined as a quantity. Thanks @bsipocz !

@jespinosaar jespinosaar requested a review from bsipocz March 30, 2026 08:58
Copy link
Copy Markdown
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much @jespinosaar for your patience with this. Let's merge with the aim of a tagged release by the end of the month, or middle May the latest. So any PRs that are follow-ups would be best suited to land before the release.

I have noticed that some of the remote tests seems to run into issues, but they are not deterministic and sometimes pass for the second test run. It maybe something worth looking into in a follow-up.

@bsipocz bsipocz merged commit 2a06262 into astropy:main Mar 31, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants