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

Support for TAP table create/delete/upload CADC prototype #274

Merged
merged 2 commits into from
Sep 23, 2022

Conversation

andamian
Copy link
Contributor

@andamian andamian commented Oct 27, 2021

This represents an implementation of a soon-to-be-proposed prototype for TAP1.2 to allow for the create/deletion of tables as well as bulk uploads of table content. The prototype service runs at the CADC and has the following characteristics:

Table creation:
Works with a PUT to the /tables/<tableName> end point. It supports 2 formats for the table schema: VOSITable and VOTable. These formats are identified by their corresponding Content-Type: text/xml and application/x-votable+xml respectively.

Table removal (table drop in DB):
Works with a DELETE to the /tables/<tableName> end point.

Table load:
Works with a sync POST to a special end point /load/<tableName> that is a sibling of the /tables end point. Supported data formats and their corresponding Content-Type are: tsv (text/tab-separated-values), csv (text/csv) and FITSTable as in FITS Binary Table (application/fits). Table must exist on the service.

Index creation:
Works with an async POST to a special end point /table-update that is sibling of the /tables end point. This mechanism supports the creation of single-column indexes only.

@andamian andamian marked this pull request as draft October 27, 2021 18:11
@andamian andamian added this to the v1.2 milestone Oct 27, 2021
@andamian andamian changed the title First version of table create/delete/upload prototype Support for TAP table create/delete/upload CADC prototype Oct 27, 2021
@bsipocz bsipocz modified the milestones: v1.2, v1.3 Dec 17, 2021
@tomdonaldson tomdonaldson modified the milestones: v1.3, v1.4 Feb 19, 2022
@olaurino
Copy link
Contributor

olaurino commented Mar 11, 2022

I have a commit that fixes some unit tests on windows, but this PR needs to be rebased since it doesn't work with recent versions of astropy to begin with (it depends on a wraps decorator that is not importable anymore). @andamian how would you like me to proceed?

@andamian
Copy link
Contributor Author

I have a commit that fixes some unit tests on windows, but this PR needs to be rebased since it doesn't work with recent versions of astropy to begin with (it depends on a wraps decorator that is not importable anymore). @andamian how would you like me to proceed?

The reason this is stalled was the missing support for experimental code. Once that is available, I can proceed with this.

Maybe I should try to rebase it and then pull from your fork for the Windows fixes? Sounds good?

@andamian
Copy link
Contributor Author

@olaurino - rebased

@olaurino
Copy link
Contributor

olaurino commented Mar 11, 2022

https://github.com/olaurino/pyvo/tree/cadc_tbupload I added two commits, one for the windows error, one for the trailing whitespace I hadn't noticed.

@codecov
Copy link

codecov bot commented Mar 11, 2022

Codecov Report

Merging #274 (c3171e6) into main (acced64) will increase coverage by 0.14%.
The diff coverage is 92.95%.

@@            Coverage Diff             @@
##             main     #274      +/-   ##
==========================================
+ Coverage   78.37%   78.51%   +0.14%     
==========================================
  Files          46       47       +1     
  Lines        5498     5548      +50     
==========================================
+ Hits         4309     4356      +47     
- Misses       1189     1192       +3     
Impacted Files Coverage Δ
pyvo/auth/authsession.py 86.66% <50.00%> (-5.65%) ⬇️
pyvo/dal/tap.py 72.84% <92.50%> (+2.22%) ⬆️
pyvo/utils/protofeature.py 100.00% <100.00%> (ø)
pyvo/utils/prototype.py 100.00% <100.00%> (ø)
pyvo/io/uws/tree.py 90.81% <0.00%> (+0.68%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@andamian
Copy link
Contributor Author

@olaurino - your changes have been added and the win tests are passing. Thanks!

pyvo/utils/protfeature.py Outdated Show resolved Hide resolved
@andamian andamian marked this pull request as ready for review September 8, 2022 16:39
@andamian
Copy link
Contributor Author

andamian commented Sep 8, 2022

@tomdonaldson @olaurino @bsipocz - this is ready for final review. The only think that I'm not sure about is documentation. The proposed update to the spec is already documented in IVOA Twiki page (link provided with the feature) but should the existence of the PyVO prototype be documented somewhere? If so where - tap documentation or a dedicated prototypes section?

@bsipocz
Copy link
Member

bsipocz commented Sep 8, 2022

I strongly believe there should be documentation in PyVO, independently of twiki as the library is a standalone one and not an ivoa thing.

If so where - tap documentation or a dedicated prototypes section?

I think it should be at both places, cross-linking. I'm thinking the main one should be at TAP, as ideally moving from prototype to standard would only involve of removing an e.g. "Prototypes" heading in the docs. And then have a separate prototypes listing what would collect all the prototypes (basically just a bulleted list with the references to the relevant docs sections?) from all over the package, as an overview.

@andamian
Copy link
Contributor Author

andamian commented Sep 8, 2022

Documentation added.

Copy link
Contributor

@tomdonaldson tomdonaldson left a comment

Choose a reason for hiding this comment

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

This generally looks good to me, with a couple comments.

The documentation location seems fine to me. The documentation content seems good also but maybe another note or two would help answer my workflow question.

pyvo/dal/tap.py Show resolved Hide resolved
pyvo/dal/tap.py Outdated Show resolved Hide resolved
@bsipocz
Copy link
Member

bsipocz commented Sep 21, 2022

Please rebase this before merging to make sure the remote and doctests are run and passed for the changes

@andamian
Copy link
Contributor Author

I believe this is now ready as well.

Copy link
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.

One minor doc formatting issue, and a more generic suggestion for the new API.

docs/dal/index.rst Outdated Show resolved Hide resolved
pyvo/dal/tap.py Show resolved Hide resolved
@tomdonaldson tomdonaldson merged commit d053cad into astropy:main Sep 23, 2022
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.

4 participants