Skip to content
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

WIP: Move astropy.samp to PyVO #239

Closed
wants to merge 9 commits into from
Closed

Conversation

pllim
Copy link
Member

@pllim pllim commented Sep 18, 2020

Fix #155 .

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

TODO

  • Get buy-in and review from PyVO.
  • 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.

@andamian andamian added this to the v1.2 milestone Sep 18, 2020
pllim and others added 5 commits March 25, 2021 14:56
[PATCH] use f-strings

Co-authored-by: ikkamens <[email protected]>
[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 <[email protected]>
[PATCH] typo in docstring

Co-authored-by: E. Madison Bray <[email protected]>
@pllim pllim force-pushed the mv-astropy-samp branch from b301f56 to 2ec74ba Compare March 25, 2021 19:56
@codecov
Copy link

codecov bot commented Mar 25, 2021

Codecov Report

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

Impacted file tree graph

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

Copy link
Member Author

@pllim pllim left a comment

Choose a reason for hiding this comment

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

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

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:
Copy link
Member Author

Choose a reason for hiding this comment

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

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

import sys
import argparse

from astropy import log
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Comment on lines +21 to +23
# 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")
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

from urllib.parse import urlparse
import xmlrpc.client as xmlrpc

from astropy.config.paths import _find_home
Copy link
Member Author

Choose a reason for hiding this comment

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

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

setup.cfg Outdated Show resolved Hide resolved
@@ -72,3 +72,4 @@ exclude_lines =
def _ipython_key_completions_

[entry_points]
samp_hub = pyvo.astropy_samp.hub_script:hub_script
Copy link
Member Author

Choose a reason for hiding this comment

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

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 pllim requested a review from tomdonaldson March 25, 2021 20:43
@pllim
Copy link
Member Author

pllim commented Apr 1, 2021

Note to self:

  1. git format-patch -1 <sha>
  2. Copy the stuff starting from @@ in patch file into a new patch file, one patch for each file affected.
  3. patch -u <filename> -i <new_patch_file>
  4. Check with git diff to make sure.
  5. 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 <[email protected]>
@bsipocz bsipocz modified the milestones: v1.2, v1.3 Dec 17, 2021
@tomdonaldson tomdonaldson modified the milestones: v1.3, v1.4 Feb 19, 2022
@bsipocz bsipocz removed this from the v1.4 milestone Jun 1, 2022
@tomdonaldson
Copy link
Contributor

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

@pllim pllim deleted the mv-astropy-samp branch September 26, 2022 19:08
@pllim
Copy link
Member Author

pllim commented Sep 26, 2022

Thanks for taking this over!

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.

Move astropy.samp to pyvo
4 participants