Move `astropy.samp` to `pyvo.astropy_samp` by fjebaker · Pull Request #729 · astropy/pyvo

fjebaker · 2026-02-06T20:20:32Z

Addresses #155 (mirrored at astropy/astropy#9763).

(should it be called pyvo.astropy_samp? In my opinion it would be cleaner to merge with the existing SAMP things so it's all under pyvo.samp?)

Not much had changed in the astropy/main version of the SAMP implementation since #527, so I applied the diffs on top as a fixup commit.

The only additional change required was a test case that I don't understand why it wasn't failing before? From my understanding of the exception subclass rules, SAMPProxyError could never be caught when a xmlrpc.client.Fault was being thrown.

I had the tests for SAMP all passing (some non-SAMP ones were failing though) and then rebased onto the main branch of pyvo. I'm now hitting this error when I try to use pytest:

  File "/home/fergus/.local/share/hatch/env/virtual/pyvo/jgqKZZnG/pyvo/lib/python3.14/site-packages/pyvo/astropy_samp/constants.py", line 36, in <module>
    with open(get_pkg_data_filename("data/astropy_icon.png"), "rb") as f:
              ~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/fergus/.local/share/hatch/env/virtual/pyvo/jgqKZZnG/pyvo/lib/python3.14/site-packages/astropy/utils/data.py", line 770, in get_pkg_data_filename
    return download_file(
        conf.dataurl + data_name,
    ...<3 lines>...
        sources=[conf.dataurl + data_name, conf.dataurl_mirror + data_name],
    )
  File "/home/fergus/.local/share/hatch/env/virtual/pyvo/jgqKZZnG/pyvo/lib/python3.14/site-packages/astropy/utils/data.py", line 1586, in download_file
    raise urllib.error.URLError(
        f"Unable to open any source! Exceptions were {errors}"
    ) from errors[sources[0]]
urllib.error.URLError: <urlopen error Unable to open any source! Exceptions were {'http://data.astropy.org/data/astropy_icon.png': <HTTPError 404: 'Not Found'>, 'http://www.astropy.org/astropy-data/data/astropy_icon.png': <HTTPError 404: 'Not Found'>}>

I don't really know how to resolve it. If I curl the URLs its trying to reach it's 200 fine? Importing pyvo and using SAMP works too.

Help with that much appreciated!

fjebaker · 2026-02-06T20:32:28Z

Oh and one more thing: this obviously erases the original authors and makes tools like git-blame not particularly useful. Is that permisseable here or should I attempt to cherry pick the commits over from the astropy repository to preserve authorship and blames?

Edit: I think GitHub would notify them all if I were to do that, so I'm reluctant to, to avoid spam.

codecov · 2026-02-06T20:33:32Z

Codecov Report

❌ Patch coverage is 74.92519% with 419 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.62%. Comparing base (0d97469) to head (c1308d5).
⚠️ Report is 69 commits behind head on main.

Files with missing lines	Patch %	Lines
pyvo/samp/hub.py	77.54%	168 Missing ⚠️
pyvo/samp/hub_script.py	14.28%	54 Missing ⚠️
pyvo/samp/client.py	78.18%	48 Missing ⚠️
pyvo/samp/web_profile.py	65.00%	35 Missing ⚠️
pyvo/samp/lockfile_helpers.py	72.80%	34 Missing ⚠️
pyvo/samp/standard_profile.py	65.85%	28 Missing ⚠️
pyvo/samp/utils.py	75.45%	27 Missing ⚠️
pyvo/samp/integrated_client.py	83.76%	19 Missing ⚠️
pyvo/samp/hub_proxy.py	92.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #729      +/-   ##
==========================================
- Coverage   84.12%   82.62%   -1.50%     
==========================================
  Files          79       91      +12     
  Lines        8617    10287    +1670     
==========================================
+ Hits         7249     8500    +1251     
- Misses       1368     1787     +419

☔ 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.

bsipocz · 2026-02-06T20:35:43Z

(should it be called pyvo.astropy_samp? In my opinion it would be cleaner to merge with the existing SAMP things so it's all under pyvo.samp?)

The main thing that got the move stalled for a few times in the past that this codebase and the existing pyvo.samp should be massaged together and we should only have one (with some deprecations if necessary). And that step needed someone who knows samp well enough to make the calls beyond just the mechanical moving of the code.

bsipocz · 2026-02-06T20:36:26Z

@pllim - please add Fergus to the astropy org, so I don't need to approve each of the CI runs here.

bsipocz · 2026-02-06T20:38:19Z

I don't really know how to resolve it. If I curl the URLs its trying to reach it's 200 fine? Importing pyvo and using SAMP works too.

Try marking that test with remote-data.

pllim · 2026-02-06T20:39:15Z

I invited PR author to https://github.com/orgs/astropy/teams/astropy-contributors team, though I am not sure if that affects the CI approval. I think you have to keep approving until they have a PR in...

bsipocz · 2026-02-06T20:43:35Z

I think you have to keep approving until they have a PR in...

I think it should be fine once he is in the team.

fjebaker · 2026-02-06T21:04:10Z

@bsipocz it's not happening inside a test, it's during setup:

https://github.com/fjebaker/pyvo/blob/0a29ea17231ec51e57a411b5e4745812e5307e1c/pyvo/astropy_samp/constants.py#L36-L37

It's strange, I can't reproduce it locally now, it magically started working fine after trying different package arguments and I can get all tests passing on my machine, but the CI has the same problem as before...

bsipocz · 2026-02-06T21:10:23Z

Locally it will work fine as long as you have the datafile. Let me just push a one line to the config, which should fix it.

bsipocz · 2026-02-06T21:14:23Z

A few gotchas/notes:

it's highly recommended to work from a feature branch on your fork, especially for large PRs. (I mean next time)
running pytest directly usually doesn't pick up issues with packaging; but you almost always reproduce the issues you see in CI if running the tox commands. E.g. tox -e test

bsipocz · 2026-02-06T21:16:49Z

Ignore: codecov status; and python3.9 status -- there is already a PR to bump those versions, once it's merged we can rebase this and hopefully the failure is going away, and if not we'll fix it at that point.

fjebaker · 2026-02-06T21:20:36Z

Ah excellent! Thank you for that, it would have taken me a while to think to look there.

And thanks for the notes! Out of curiosity, why is it recommended to work on a feature branch on a fork? Isn't the fork's main in some sense a feature branch, or is this because of CI rule matching?

In the meantime, I merged everything together into a pyvo.samp module. The existing samp.py only contains a few extra helper functions for sending results to SAMP clients, so there was not much work involved.

Am I alright to push that here as well?

bsipocz · 2026-02-06T21:24:36Z

And thanks for the notes! Out of curiosity, why is it recommended to work on a feature branch on a fork? Isn't the fork's main in some sense a feature branch, or is this because of CI rule matching?

Because you may want to work something else, too while this is waiting for review :)
(yes, you're right that your main is like a feature branch so it doesn't really matter. And this all will become a thing when you have gazillions of branches and start working on other people's too. E.g. for me pushing to other people's main is just feels not right, especially when it's a force push 😅 )

bsipocz · 2026-02-06T21:25:30Z

In the meantime, I merged everything together into a pyvo.samp module. The existing samp.py only contains a few extra helper functions for sending results to SAMP clients, so there was not much work involved.

OK, that sounds all right, push it in here. I'll leave the actual review to the others as I don't really have relevant samp experience.

fjebaker · 2026-02-06T21:31:17Z

E.g. for me pushing to other people's main is just feels not right, especially when it's a force push 😅

That's absolutely fair enough, will bear that in mind for the future!

I've pushed the last of my changes then for now and will await review. Thanks again for your help and for wrangling the CI :)

bsipocz · 2026-02-06T22:24:21Z

The sphinx warnings are real, but we can wrangle the docs once the review is done and if opinions about the public API are expressed.

bsipocz

Some very high level comments, I would say the most important part will be to think about what the public API should look like.

bsipocz · 2026-02-06T22:37:00Z

we don;t need this file, I wonder why was it still around for astropy

@astrofrog probably should have deleted it in astropy/astropy#8413 but looks like he deleted the content but not the file itself. Probably an oversight.

bsipocz · 2026-02-06T22:38:18Z

+    n_retries = _config.ConfigItem(
+        10, "How many times to retry communications when they fail"
+    )
+


I suppose we may want to have some package level public API with an __all__; at least this should be a discussion item during review of what and how the public API should look like.

bsipocz · 2026-02-06T22:39:07Z

+    else:
+        try:
+            urlopen("http://google.com", timeout=1.0)
+        except Exception:


It would be probably useful to clean up all these blanket exceptions (here and everywhere below); maybe it's more as a part of a follow-up though

bsipocz · 2026-02-06T22:39:25Z

+from .errors import SAMPProxyError
+
+
+def internet_on():


do we need this at all?

It seems to be used in a couple of places: https://github.com/search?q=repo%3Aastropy%2Fastropy%20internet_on&type=code

bsipocz · 2026-02-06T22:40:27Z

+
+from .standard_profile import SAMPSimpleXMLRPCRequestHandler, ThreadingXMLRPCServer
+
+__all__ = []


None of these are public API?

Nope. Not since astropy/astropy#1907 but doesn't mean you cannot make them public now.

msdemlei · 2026-02-07T08:11:42Z

On Fri, Feb 06, 2026 at 01:31:41PM -0800, Fergus Baker wrote: fjebaker left a comment (astropy/pyvo#729) I've pushed the last of my changes then for now and will await review. Thanks again for your help and for wrangling the CI :)

I've paged through a summary diff towards 2f31cc1, and while it would certainly be useful to again think about much of sampy's code, my take would be that it's old enough to count as ground truth until proven buggy. I'd hence be happy to take this as a baseline (merged into pyvo, as far as I am concerned) and then start working from that for API and robustness improvements and in particular documentation; against what's currently documented in astropy.samp, I think I'd prefer showing people the context manager (without it, they're quite likely to accumulate hung clients in the Hub), and perhaps also the convenience functions for sending tables and images. Fergus: If there's non-technical changes against what was in astropy.samp already in this PR, could you briefly say where they are and where we should take a closer look?

fjebaker · 2026-02-07T13:29:28Z

Fergus: If there's non-technical changes against what was in astropy.samp already in this PR, could you briefly say where they are and where we should take a closer look?

@msdemlei I have made no changes to the implementation or interfaces in this PR -- all of the commits are simply moving code around and appeasing the style checker, packaging, and adjusting one test case to catch the actual exception being thrown (*).

My plan was to help move the existing SAMP code, unchanged, from astropy to pyvo, so that I would have a clear place where I can start opening PRs to fix common pitfalls. As Markus mentions, the documentation is also not always the most clarifying (e.g. on the difference between the different bind functions). Low hanging fruit such as that was going to be where I start -- essentially fixing the bits I, and likely others, struggle with when using the package.

I agree that decisions on what should the public API be ought to be deferred to later PRs. Nothing has been changed from the current experience, so a user would not notice anything other than an eventual s/astropy.samp/pyvo.samp/g if this were merged. Deferring the API discussions to other PRs would also lead to easier review cycles in my opinion, as this PR is a nasty +5066,-1, and GitHub doesn't make that easy or pretty to look at.

(*) for the record, this PR was prepared by taking #527 and the main branch of astropy and using diff to resolve any changes and correct the import statements as necessary. If you diff astropy main and this branch now, that's also all you would see.

bsipocz · 2026-02-08T02:11:40Z

I'm ok with this plan above. The plan is to release 1.9 before ivoa, so I think we have plenty of time to sort out any critical issues that may pop up.

msdemlei

Given #729 (comment), I'm all in favour of merging this.

bsipocz · 2026-02-09T15:20:55Z

@fjebaker - could you please add a sentence or two to the changelog entry, and also rebase? That would take care of two of the failing CI jobs.

fjebaker · 2026-02-10T17:52:56Z

For the CI failures:

Read the Docs: I missed updating the docstrings from astropy.samp to pyvo.samp, which I will now do.
Python 3.10, the SAMP code copied over has references to from astropy.tests.helper import CI in order to skip some tests. @bsipocz would you know what I should do here?
Codecov: I didn't really want to add new code here so I think that will just have to be left failing...

bsipocz · 2026-02-10T17:58:05Z

Read the Docs: I missed updating the docstrings from astropy.samp to pyvo.samp, which I will now do.

Thanks!

Python 3.10, the SAMP code copied over has references to from astropy.tests.helper import CI in order to skip some tests. @bsipocz would you know what I should do here?

I'll have a look at this, maybe directly push some changes if the workaround will feel to be too hacky to explain

Codecov: I didn't really want to add new code here so I think that will just have to be left failing...

Do ignore codecov, it has no significance for this type of PR (we only care about it if a PR directly decreases coverage, here everything is just ported so our new baseline will have to change).

bsipocz · 2026-02-10T18:18:31Z

+
+from astropy.samp import SAMPHubServer, SAMPIntegratedClient, conf
+from astropy.samp.web_profile import CLIENT_ACCESS_POLICY, CROSS_DOMAIN
+from astropy.tests.helper import CI


Just swap this out with the line, here and in the other file, too.

CI = os.environ.get("CI", "false") == "true"

The Python 3.10 CI fails to resolve the necessary versions of astropy to import the CI helper. As suggested by Brigitta, these are replaced with queries to the environment variables.

These particular test seems to hang forever on the MacOS CI runner, likely due to some race condition or other.

fjebaker · 2026-02-20T22:31:19Z

I tried discovering which tests are causing MacOS to fail but it's a bit tedious since I don't have a Mac and can't reproduce the failure outside of the GitHub CI. So, instead I've just skipped basically all the SAMP tests on MacOS. It's not ideal but I think if it needs to be addressed it can be done so in a subsequent PR.

bsipocz · 2026-02-21T02:40:41Z

 from pyvo.samp.hub_proxy import SAMPHubProxy
 from pyvo.samp.integrated_client import SAMPIntegratedClient

+IS_MACOS = sys.platform == "darwin"


Locally these don't hang, so I went ahead and change that they are only skipped in CI.

bsipocz

Thank you @fjebaker, this is awesome!

Would you mind going ahead and make a deprecating CI for astropy? Or let us know if not and the either me or @pllim will make one.

bsipocz · 2026-02-21T02:43:27Z

I tried discovering which tests are causing MacOS to fail but it's a bit tedious since I don't have a Mac and can't reproduce the failure outside of the GitHub CI. So, instead I've just skipped basically all the SAMP tests on MacOS. It's not ideal but I think if it needs to be addressed it can be done so in a subsequent PR.

That sounds all good. The issue is that the tests passing well for me locally, on osx. So I'm just as puzzled. Would you mind opening a reminder issue for turning back on these tests?

fjebaker · 2026-02-21T07:25:39Z

@bsipocz thanks for looking at this so quick! Yes, I can put together a deprecating PR (?) for astropy. I had a quick look at how astropy does that and I gather I should decorate all of the main SAMP classes and methods with @deprecated(...)?

pllim · 2026-02-23T19:33:16Z

Thanks! So is the next step to release pyvo with this feature and then we can deprecate astropy.samp?

bsipocz · 2026-02-23T19:46:25Z

Thanks! So is the next step to release pyvo with this feature and then we can deprecate astropy.samp?

This will be released in v1.9, end of May - early June. I don't see dates for v8.0 yet, do you have them by any chance? We can either coordinate to get it out in 8.0, or schedule the deprecation for v8.1 to be merged once 8.0 is out.

bsipocz · 2026-02-23T19:47:57Z

@bsipocz thanks for looking at this so quick! Yes, I can put together a deprecating PR (?) for astropy. I had a quick look at how astropy does that and I gather I should decorate all of the main SAMP classes and methods with @deprecated(...)?

It's whole module level deprecation, we may instead want to just raise the warning at the time of import (e.g. I do similar things in astroquery for astroquery.irsa; there maybe similar precedents on the sub-submodule level in astropy, too).

pllim · 2026-02-23T19:53:48Z

Yeah I have to remember how we did that for vo_conesearch back in the days.

astrofrog · 2026-02-23T19:54:16Z

A single module level warning seems reasonable

pllim · 2026-02-23T19:56:41Z

I don't see dates for v8.0 yet

@astrofrog , can you please fill in https://github.com/astropy/astropy/wiki/Release-Calendar ? 🙏

I am guessing it would be Apr-May 2026, which is just before the release here. So maybe pushing back to astropy v8.1 is more realistic. Which means we cannot merge the astropy.samp deprecation PR until after v8.0.x is branched.

bsipocz · 2026-02-23T19:59:52Z

OK, then my suggestion is that either you @pllim or I open the astropy PR as it's just scaffolding and admin wrangling through the different versions (and we should not do that to Fergus just for the sake of it, there isn't much interesting happening there but potentially a lot of coordination and kicking the testing infrastructure until it's happy.)

pllim · 2026-02-23T20:01:56Z

Sure! But we should not do it until we can sort out the release timelines. No point doing it now if we have to wait till v8.1

bsipocz · 2026-02-23T20:13:42Z

Sure! But we should not do it until we can sort out the release timelines. No point doing it now if we have to wait till v8.1

Except that we don't forget about it 😅 ; there is certainly precedent to parking PRs like that.

pllim · 2026-03-06T23:10:18Z

OK, here it is: astropy/astropy#19373

bsipocz added this to the v1.9 milestone Feb 6, 2026

bsipocz added component: samp enhancement labels Feb 6, 2026

fjebaker force-pushed the main branch from 3be131f to 0a29ea1 Compare February 6, 2026 21:02

bsipocz reviewed Feb 6, 2026

View reviewed changes

msdemlei approved these changes Feb 8, 2026

View reviewed changes

fjebaker force-pushed the main branch from fbe3775 to 4e6c695 Compare February 10, 2026 17:45

bsipocz reviewed Feb 10, 2026

View reviewed changes

fix: remove astropy.tests.helper CI

6a72f2e

The Python 3.10 CI fails to resolve the necessary versions of astropy to import the CI helper. As suggested by Brigitta, these are replaced with queries to the environment variables.

fjebaker force-pushed the main branch from 960ec7e to 5b2aa12 Compare February 20, 2026 21:57

test: skip a samp tests on macos

92cc8d0

These particular test seems to hang forever on the MacOS CI runner, likely due to some race condition or other.

fjebaker force-pushed the main branch from 5b2aa12 to 92cc8d0 Compare February 20, 2026 22:27

CI: only skip OSX tests in CI, locally they don't hang

c1308d5

bsipocz reviewed Feb 21, 2026

View reviewed changes

bsipocz approved these changes Feb 21, 2026

View reviewed changes

bsipocz merged commit 79276ed into astropy:main Feb 21, 2026
10 of 13 checks passed

fjebaker mentioned this pull request Feb 21, 2026

CI: MacOS SAMP tests hang indefinitely #731

Open

pllim mentioned this pull request Feb 23, 2026

Move samp to PyVO package astropy/astropy#9763

Closed

6 tasks

This comment was marked as resolved.

Sign in to view

pllim mentioned this pull request Mar 6, 2026

API: Deprecate astropy.samp astropy/astropy#19373

Merged

3 tasks

This was referenced Mar 11, 2026

SAMPHubServer should raise an exception if another hub is already running #737

Open

web_profile_server is not properly shutting down in astropy.vo.samp #738

Open

This was referenced Jun 5, 2026

Enable context-aware warnings for the freethreaded CI run astropy/astropy#19861

Merged

Remove SAMP maintenance from Astropy Team astropy/astropy.github.com#716

Merged

Fixes for SAMP migration from astropy to pyvo #762

Merged


		from .standard_profile import SAMPSimpleXMLRPCRequestHandler, ThreadingXMLRPCServer

		__all__ = []

Uh oh!

Uh oh!

Conversation

fjebaker commented Feb 6, 2026

Uh oh!

fjebaker commented Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bsipocz commented Feb 6, 2026

Uh oh!

bsipocz commented Feb 6, 2026

Uh oh!

bsipocz commented Feb 6, 2026

Uh oh!

pllim commented Feb 6, 2026

Uh oh!

bsipocz commented Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjebaker commented Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsipocz commented Feb 6, 2026

Uh oh!

bsipocz commented Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsipocz commented Feb 6, 2026

Uh oh!

fjebaker commented Feb 6, 2026

Uh oh!

bsipocz commented Feb 6, 2026

Uh oh!

bsipocz commented Feb 6, 2026

Uh oh!

fjebaker commented Feb 6, 2026

Uh oh!

bsipocz commented Feb 6, 2026

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msdemlei commented Feb 7, 2026 via email

Uh oh!

fjebaker commented Feb 7, 2026

Uh oh!

bsipocz commented Feb 8, 2026

Uh oh!

msdemlei left a comment

Choose a reason for hiding this comment

Uh oh!

bsipocz commented Feb 9, 2026

Uh oh!

fjebaker commented Feb 10, 2026

Uh oh!

bsipocz commented Feb 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjebaker commented Feb 20, 2026

fjebaker commented Feb 6, 2026 •

edited

Loading

codecov Bot commented Feb 6, 2026 •

edited

Loading

bsipocz commented Feb 6, 2026 •

edited

Loading

fjebaker commented Feb 6, 2026 •

edited

Loading

bsipocz commented Feb 6, 2026 •

edited

Loading

bsipocz commented Feb 23, 2026 •

edited

Loading