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: Restructuring to enable multiple rows, multiple access points, and unit tests #18

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

zoghbi-a
Copy link
Contributor

This is an attempt to go beyond the simple use cases and consider a realistic case of a user submitting a list of products as either an astropy.Table or pyvo.dal.DALResults.

Both a json column and datalinks are processed to create a list of AccessPoint objects for each row of the data product.

AccessPoint and its subclasses (for aws, gc etc) have a download method that downloads the data

@zoghbi-a
Copy link
Contributor Author

zoghbi-a commented Feb 8, 2023

I have an implementation that integrates this into pyvo. It is in my pyvo cloud-links branch.

@bsipocz
Copy link
Member

bsipocz commented Feb 8, 2023

@zoghbi-a - a lot of it is already massaged into pyvo in astropy/pyvo#369, I'll come back to that PR and chop it up to smaller pieces.

@zoghbi-a
Copy link
Contributor Author

zoghbi-a commented Feb 9, 2023

Thanks @bsipocz,
Yes astropy/pyvo#369 does the json part. The datalinks part that I added is clunky and I plugged it in as proof of concept only. This restructuring allows both (json and datalink methods; plus others if needed, e.g. a column with s3 uris) to be done cleanly both for a single row and for a set of rows. This latter point is important, because for datalinks, it is inefficient to make a datalink call for every row. It is more efficient to make a single call requesting all datalinks. The restructured code allows that.

One other difference is how to deal with services other than SIA, which we want to consider in the future. With what we have, it is not straight forward to expand it to SSA and other services. With the new code, it is a matter of adding a single inheritance to the relevant service or Query class.

@bsipocz
Copy link
Member

bsipocz commented Feb 9, 2023

Yes, I don't mean to have that as it's any more but am cutting it up to pieces.

@zoghbi-a
Copy link
Contributor Author

This will be closed in favor of a other PRs (#21 and #22).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants