WIP: Move astropy.samp to PyVO by pllim · Pull Request #239 · astropy/pyvo

pllim · 2020-09-18T22:06:01Z

Fix #155 .

This is the first step towards astropy/astropy#9763 . cc @astrofrog

TODO

Get buy-in and review from PyVO.
- ~~Need to wait for PyVO to drop Python 3.5 first.~~ MNT: Bump Python minversion #255
- If PyVO applies APE 17, can undo the changes to setup_package.py.
Port over typo fixes from Fix various typos astropy#11967
Incorporate review comments.
Make sure tests pass.
Make sure render doc looks fine.
Add change log? (Can't find a section for unreleased version.)

Notes

I cannot name this new subpackage pyvo.samp because you already have samp.py. I don't know how it overlaps with astropy.samp.
astropy.samp used astropy.__version__, this needs to change now. Needs discussion.
This subpackage uses astropy.config system. I don't think we can change it for the sake of backward compatibility.
This subpackage uses astropy.log for logging. If PyVO has its own logging, it can be replaced.
There is usage of hidden function astropy.config.path._find_home. If you have a better replacement, please let me know.
astropy moved away from a lot of the astropy_helpers setup stuff. I tried to move them back here (e.g., setup_package.py) since you have not implemented APE 17 yet. Need extra pairs of eyes to make sure I didn't mess things up.
Replaced astropy.samp with new import.

Disclaimer: I don't maintain astropy.samp, Tom Robitaille does.

[PATCH] use f-strings Co-authored-by: ikkamens <ikkamens@amazon.com>

[PATCH] BUG: Fix missing server_close method on SAMPHubProxy It's possible this bug has existed for a long time and was just hidden. Another possibility is it appeared at some point during refactoring, but as far as I can tell it's been around a long time. The attribute is still just a ; it does not make sense for it to have a method because it does not itself run or wrap a server. It does however manage a pool of XML-RPC client connections which is deleted when calling . To be on the safe side I added a method to the proxy pool class which closes the client connections. This is called when shutting down the server loop. Unlike the normal class, the used only in tests never runs a TCP server of its own, so calling anywhere in here does not make sense. Co-authored-by: E. Madison Bray <erik.bray@lri.fr>

[PATCH] typo in docstring Co-authored-by: E. Madison Bray <erik.bray@lri.fr>

codecov · 2021-03-25T20:11:32Z

Codecov Report

Merging #239 (4f3fbd7) into master (062710b) will increase coverage by 0.56%.
The diff coverage is 77.11%.

@@            Coverage Diff             @@
##           master     #239      +/-   ##
==========================================
+ Coverage   74.83%   75.39%   +0.56%     
==========================================
  Files          43       55      +12     
  Lines        5035     6678    +1643     
==========================================
+ Hits         3768     5035    +1267     
- Misses       1267     1643     +376

Impacted Files	Coverage Δ
pyvo/astropy_samp/standard_profile.py	`65.43% <65.43%> (ø)`
pyvo/astropy_samp/web_profile.py	`66.32% <66.32%> (ø)`
pyvo/astropy_samp/utils.py	`69.66% <69.66%> (ø)`
pyvo/astropy_samp/lockfile_helpers.py	`73.01% <73.01%> (ø)`
pyvo/astropy_samp/hub.py	`77.51% <77.51%> (ø)`
pyvo/astropy_samp/client.py	`78.18% <78.18%> (ø)`
pyvo/astropy_samp/hub_script.py	`78.33% <78.33%> (ø)`
pyvo/astropy_samp/integrated_client.py	`83.62% <83.62%> (ø)`
pyvo/astropy_samp/hub_proxy.py	`92.00% <92.00%> (ø)`
pyvo/astropy_samp/__init__.py	`100.00% <100.00%> (ø)`
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 062710b...4f3fbd7. Read the comment docs.

pllim

I never used SAMP before, so you should really test this branch out during review.

Can probably ignore the coverage complains. Adding new tests is out of scope of this PR.

Would be nice if we can get this in and if you plan on a release before astropy 4.3, so we can go ahead and deprecate astropy.samp for 4.3 (feature freeze on 2021-04-23).

pllim · 2021-03-25T20:09:28Z

+SAFE_MTYPES = ["samp.app.*", "samp.msg.progress", "table.*", "image.*",
+               "coord.*", "spectrum.*", "bibcode.*", "voresource.*"]
+
+with open(get_pkg_data_filename('data/astropy_icon.png'), 'rb') as f:


If there is some pyvo icon you want to replace this with, let me know.

pllim · 2021-03-25T20:19:55Z

+import sys
+import argparse
+
+from astropy import log


If you want to replace this with your own logger, please give some advice on how to do it properly.

pllim · 2021-03-25T20:21:17Z

+    # TODO: Currently defines astropy version from which this is copied over.
+    # PyVO should define a new versioning scheme for this module.
+    parser = argparse.ArgumentParser(prog="samp_hub 4.2")


Also let me know how you want to track its version. It was using astropy.__version__ but I changed it to 4.2. If I use pyvo.__version__, it will be too confusing because pyvo still at 1.x.

pllim · 2021-03-25T20:29:48Z

+from urllib.parse import urlparse
+import xmlrpc.client as xmlrpc
+
+from astropy.config.paths import _find_home


This uses a "hidden" function in astropy. Please let me know if you have a better alternative.

pllim · 2021-03-25T20:42:27Z

  def _ipython_key_completions_

 [entry_points]
+samp_hub = pyvo.astropy_samp.hub_script:hub_script


I am not sure how confusing this CLI will get since astropy and pyvo will have this same command until we manage to remove the code from astropy in a future release (it needs a deprecation period over there).

pllim · 2021-04-01T21:36:16Z

Note to self:

git format-patch -1 <sha>
Copy the stuff starting from @@ in patch file into a new patch file, one patch for each file affected.
patch -u <filename> -i <new_patch_file>
Check with git diff to make sure.
Commit with a message referencing to the original SHA upstream and give original author co-author credit.

[PATCH] Fixes #11407: Makes two additional fixes to the test SAMPWebClient: * Its server loop (for accepting calls from the Hub) should block until the client has actually registered with the hub (rather than busy waiting) * More importantly, there needs to be a lock associated with whether or not the client is registered. The lock does not need to be held by SAMPWebClient.register because the _server_thread blocks until the _registered_event is set. However, SAMPWebClient.unregister does need to hold the lock to ensure it does not unregister the client when it is about to perform a pullCallbacks request to the hub. Co-authored-by: E. Madison Bray <erik.bray@lri.fr>

tomdonaldson · 2022-09-22T22:35:06Z

Closing this to get a fresh start with many thanks to @pllim for this work and the notes it generated.

pllim · 2022-09-26T19:08:49Z

Thanks for taking this over!

pllim mentioned this pull request Sep 18, 2020

Move samp to PyVO package astropy/astropy#9763

Closed

6 tasks

andamian added this to the v1.2 milestone Sep 18, 2020

andamian added the enhancement label Sep 18, 2020

WIP: Move astropy.samp to PyVO

1b02b29

pllim mentioned this pull request Mar 25, 2021

MNT: Bump Python minversion #255

Merged

pllim and others added 5 commits March 25, 2021 14:56

Apply patch from astropy/astropy@6f9d3ae

b8f37cb

[PATCH] use f-strings Co-authored-by: ikkamens <ikkamens@amazon.com>

Apply patch from astropy/astropy@903393a

fa5ef19

[PATCH] typo in docstring Co-authored-by: E. Madison Bray <erik.bray@lri.fr>

Copy over astropy.samp docs

5de526e

DOC: Replace astropy.samp with pyvo.astropy_samp

2ec74ba

pllim force-pushed the mv-astropy-samp branch from b301f56 to 2ec74ba Compare March 25, 2021 19:56

DOC: Do not link to old module

e2bb271

BUG: Fix URL and entry point

16aaff5

pllim commented Mar 25, 2021

View reviewed changes

pllim requested a review from tomdonaldson March 25, 2021 20:43

bsipocz modified the milestones: v1.2, v1.3 Dec 17, 2021

tomdonaldson modified the milestones: v1.3, v1.4 Feb 19, 2022

bsipocz removed this from the v1.4 milestone Jun 1, 2022

tomdonaldson mentioned this pull request Sep 22, 2022

Move astropy.samp to pyvo #155

Closed

tomdonaldson closed this Sep 22, 2022

pllim deleted the mv-astropy-samp branch September 26, 2022 19:08

MridulS mentioned this pull request Feb 20, 2024

Move astropy.samp to pyvo.astropy_samp #527

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

WIP: Move astropy.samp to PyVO#239

WIP: Move astropy.samp to PyVO#239
pllim wants to merge 9 commits into
astropy:mainfrom
pllim:mv-astropy-samp

pllim commented Sep 18, 2020 •

edited

Loading

Uh oh!

codecov Bot commented Mar 25, 2021 •

edited

Loading

Uh oh!

pllim left a comment

Uh oh!

pllim Mar 25, 2021

Uh oh!

pllim Mar 25, 2021

Uh oh!

pllim Mar 25, 2021

Uh oh!

pllim Mar 25, 2021

Uh oh!

Uh oh!

pllim Mar 25, 2021

Uh oh!

pllim commented Apr 1, 2021

Uh oh!

tomdonaldson commented Sep 22, 2022

Uh oh!

pllim commented Sep 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Uh oh!

Conversation

pllim commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

pllim Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

pllim Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

pllim Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

pllim Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pllim Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

pllim commented Apr 1, 2021

Uh oh!

tomdonaldson commented Sep 22, 2022

Uh oh!

pllim commented Sep 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pllim commented Sep 18, 2020 •

edited

Loading

codecov Bot commented Mar 25, 2021 •

edited

Loading