-
Notifications
You must be signed in to change notification settings - Fork 13
Add tracking of submitted transforms #634
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
base: master
Are you sure you want to change the base?
Add tracking of submitted transforms #634
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #634 +/- ##
==========================================
- Coverage 96.97% 96.68% -0.29%
==========================================
Files 29 29
Lines 1948 1960 +12
==========================================
+ Hits 1889 1895 +6
- Misses 59 65 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is what it looks like right now:
Cached Queries
┏━━━━━━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━┳━━━━━━━━━━━┓
┃ Title ┃ Codegen ┃ Transform ID ┃ Run Date ┃ Files ┃ Format ┃
┡━━━━━━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━╇━━━━━━━━━━━┩
│ ttbar_trijet │ atlasr25 │ 14b9d860-06de-47d1-bb52-34b73451ca7b │ Sat, 2025-07-12 09:32 │ 1 │ root-file │
│ object_counts │ atlasr25 │ 169d78f0-b56a-4444-a070-eb130dacab61 │ Sat, 2025-07-12 19:18 │ 1 │ root-file │
│ ttbar_trijet │ atlasr25 │ c10294a6-ae9a-4c07-b58f-31500ee40280 │ Sun, 2025-07-13 02:01 │ 1 │ root-file │
│ ttbar_trijet │ atlasr25 │ 7230cfd5-f445-4049-9b0d-2a874700859f │ Sun, 2025-07-13 02:08 │ 1 │ root-file │
│ ttbar_trijet │ atlasr25 │ 5605d79e-1110-4784-b5a3-157a4aeaa248 │ Sun, 2025-07-13 02:16 │ 1 │ root-file │
│ object_counts │ atlasr25 │ 9fb515d0-f4ba-43db-8302-052bfb31c840 │ Tue, 2025-07-15 10:02 │ 100 │ root-file │
│ object_counts │ atlasr25 │ 8cae73e8-1b59-4528-a6cb-0f647aad641c │ Tue, 2025-07-15 14:31 │ 2 │ root-file │
│ object_counts │ atlasr25 │ 413692bd-70f4-496f-ba51-4f1500f7e742 │ Sun, 2025-07-20 11:59 │ 3 │ root-file │
│ object_counts │ atlasr25 │ 3a13ea62-1850-40a3-8509-93ee33854d68 │ Mon, 2025-07-21 15:05 │ 5300 │ root-file │
│ object_counts │ atlasr25 │ 08c0fca8-9163-41b1-b90b-60b912c61186 │ Tue, 2025-07-22 14:37 │ 1 │ root-file │
│ │ │ e8c18537-65d5-42f0-b68e-3f54471b0ad6 │ Pending │ Pending │ │
└───────────────┴──────────┴──────────────────────────────────────┴───────────────────────┴─────────┴───────────┘
Currently, in the db entry, more information is not available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces tracking of submitted transforms in the cache system and improves the cache list command to display pending submissions. The changes consolidate the transform submission recording process and provide better visibility into pending requests.
- Consolidates submitted transform metadata recording with a new
cache_submitted_transform
method - Adds functionality to retrieve and display submitted (pending) queries in the CLI
- Updates documentation to clarify that the cache list command shows both completed and pending submissions
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
servicex/query_cache.py | Adds cache_submitted_transform method and submitted_queries method for better tracking |
servicex/query_core.py | Updates transform submission to use new consolidated cache API |
servicex/app/cache.py | Enhances cache list command to display pending submissions alongside completed ones |
tests/test_query_cache.py | Updates tests to use new cache API and adds test for submitted queries functionality |
tests/test_dataset.py | Updates mock method calls to reflect new cache API |
docs/command_line.rst | Documents that cache list shows pending submissions |
Comments suppressed due to low confidence (1)
servicex/app/cache.py:75
- [nitpick] The variable name 'r' is ambiguous and inconsistent with the loop above which uses the same name. Consider using 'pending_query' or 'pending_record' for clarity.
for r in pending:
"result_format": transform.result_format, | ||
"request_id": request_id, | ||
"status": "SUBMITTED", | ||
"submit_time": datetime.now(timezone.utc).isoformat(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider accepting submit_time as a parameter to make the method more testable and flexible, rather than always using the current time.
"submit_time": datetime.now(timezone.utc).isoformat(), | |
"submit_time": submit_time, |
Copilot uses AI. Check for mistakes.
…ps://github.com/ssl-hep/ServiceX_frontend into codex/extend-query-cache-with-submitted-queries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, please take a look at the comments, but on the whole I like the motivation for the changes
@@ -62,6 +62,7 @@ def list(): | |||
table.add_column("Files") | |||
table.add_column("Format") | |||
runs = cache.cached_queries() | |||
pending = cache.submitted_queries() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please change "pending" to "submitted"
r.get("codegen", ""), | ||
r.get("request_id", ""), | ||
"Pending", | ||
"Pending", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In particular as to what is shown to the users
@@ -148,6 +149,24 @@ def update_transform_request_id(self, hash_value: str, request_id: str) -> None: | |||
transform.hash == hash_value, | |||
) | |||
|
|||
def cache_submitted_transform( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer not to have a submitted-only code path
@@ -203,6 +222,17 @@ def cached_queries(self) -> List[TransformedResults]: | |||
] | |||
return result | |||
|
|||
def submitted_queries(self) -> List[dict]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be generalized to return queries in a specified state (not just submitted)?
Clean up of how we record submitted items, as well as add them to the
cache list
command.Summary
cache_submitted_transform
submitted_queries
cache list
shows submitted queriesTesting
pytest -q
Comments
In its current form there is a potential UX issue - the
Pending
queries can complete, but the local cache won't be updated. This might mean this dump is incorrect (e.g. lies to the user).https://chatgpt.com/codex/tasks/task_e_68791d01353483208e71471198f5c3ca