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

Make TAP job deletion optional defaulting to True #640

Merged
merged 4 commits into from
Jan 16, 2025

Conversation

stvoutsin
Copy link
Contributor

@stvoutsin stvoutsin commented Jan 16, 2025

Motivation for change

We have a requirement for the Rubin Observatory TAP services to maintain and provide history of queries run through the TAP service, for which we plan on utilizing the UWS jobs list. However, since many of our users will be querying our services via pyvo, query jobs will be deleted by default after being run as it currently stands.

PR Summary

This PR adds a delete parameter to control whether TAP jobs are automatically deleted after completion.
By default, jobs are still deleted (to maintain backward compatibility), but users can now set delete=False
to keep jobs.

Changes

  • Added delete parameter to run_async method in TAPService
  • Added delete parameter to AsyncTAPJob context manager
  • Updated docstrings to document new parameter

Example usage

# Default behavior - job is deleted after completion
result = service.run_async(query)

# Keep job after completion
result = service.run_async(query, delete=False)

# Context manager with job retention
with AsyncTAPJob(url, delete=False) as job:
    results = job.run().wait().fetch_result()

@bsipocz bsipocz added this to the v1.7 milestone Jan 16, 2025
pyvo/dal/tap.py Outdated
if self._delete_on_exit:
try:
self.delete()
except Exception:
Copy link
Member

Choose a reason for hiding this comment

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

This may point beyond the PR, but while we're at it, maybe use a more specific exception here rather than a blanket one?

Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 44.44444% with 5 lines in your changes missing coverage. Please review.

Project coverage is 82.30%. Comparing base (d1fa2bb) to head (f765128).
Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
pyvo/dal/tap.py 44.44% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #640      +/-   ##
==========================================
- Coverage   82.30%   82.30%   -0.01%     
==========================================
  Files          72       72              
  Lines        7427     7430       +3     
==========================================
+ Hits         6113     6115       +2     
- Misses       1314     1315       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

This needs a changelog entry otherwise looks good to me.

(Test failures in the remote job are not related)

Copy link
Contributor

@andamian andamian left a comment

Choose a reason for hiding this comment

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

It looks good to me. Just needs a CHANGE log entry and make sure tests pass.

@stvoutsin
Copy link
Contributor Author

Thanks for the review. I've added a changelog entry, I also noticed the code-coverage message, happy to add some tests as well if you think they would be useful here.

@bsipocz
Copy link
Member

bsipocz commented Jan 16, 2025

I'm not sure how to meaningfully cover this with tests, so I would say this is ready to go, pending a small fix to the changelog.

Thanks @stvoutsin!

(ps Adrian: the test failures are unrelated)

CHANGES.rst Outdated Show resolved Hide resolved
@bsipocz bsipocz merged commit 323fae4 into astropy:main Jan 16, 2025
10 of 13 checks passed
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.

3 participants