remove s3 bucket polling when waiting for transformation results #587

MattShirley · 2025-05-06T20:51:36Z

Client side work for ssl-hep/ServiceX#1049

for more information, see https://pre-commit.ci

codecov · 2025-05-14T19:43:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.50%. Comparing base (eb0ff9c) to head (7ddfc60).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #587      +/-   ##
==========================================
+ Coverage   96.35%   96.50%   +0.14%     
==========================================
  Files          29       29              
  Lines        1892     1973      +81     
==========================================
+ Hits         1823     1904      +81     
  Misses         69       69

Flag	Coverage Δ
unittests	`96.50% <100.00%> (+0.14%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…/ServiceX_frontend into remove-s3-bucket-polling

for more information, see https://pre-commit.ci

…/ServiceX_frontend into remove-s3-bucket-polling

for more information, see https://pre-commit.ci

Copilot

Pull Request Overview

Client-side removal of Minio bucket polling in favor of ServiceX’s direct results API, with corresponding adapter support and test updates.

Added get_transformation_results method to the ServiceX adapter and wired it into download logic.
Updated core query logic (download_files) to pass a begin_at timestamp and call the new API instead of polling Minio.
Refactored tests to mock servicex.get_transformation_results and added unit tests covering its success and error responses.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/test_servicex_dataset.py	Replaced `mock_minio.list_bucket` with `servicex.get_transformation_results` mocks and reset calls
tests/test_servicex_adapter.py	Imported `datetime` and added async tests for `get_transformation_results` handling 200/403/404/500 statuses
tests/test_dataset.py	Passed new `begin_at` argument to `download_files` and mocked the ServiceX results API
servicex/servicex_adapter.py	Implemented `get_transformation_results` method with status‐code checks
servicex/query_core.py	Updated `transform_complete` and `download_files` to accept and forward `begin_at` and call the new API

Comments suppressed due to low confidence (1)

tests/test_servicex_dataset.py:209

The string literal for request_id has mismatched quotes and includes an extra trailing double quote; this will likely cause the test to fail. Use consistent quoting, for example "123-456-789".

servicex.submit_transform.return_value = {"request_id": '123-456-789"'}

servicex/servicex_adapter.py

ponyisi · 2025-05-20T21:38:42Z

servicex/query_core.py

                    for file in files:
-                        if file.filename not in files_seen:
+                        file_path = file.get("file-path", "").replace("/", ":")


This relies on being kept in line with the transformer sidecar, and I think it may fail in certain cases (like the parquet output format is selected). Should we add a new field in the transform_result table that stores the object_name as uploaded to S3 (as determined by https://github.com/ssl-hep/ServiceX/blob/1991e6b2ea00dcbd8cdb9b9ed32fd44049f0dea3/transformer_sidecar/src/transformer_sidecar/transformer.py#L349 etc.) ?

BenGalewsky · 2025-05-21T18:00:10Z

servicex/query_core.py

@@ -557,15 +566,22 @@ async def get_signed_url(
            if self.minio:
                # if self.minio exists, self.current_status will too
                if self.current_status.files_completed > len(files_seen):
-                    files = await self.minio.list_bucket()
+                    new_begin_at = datetime.datetime.now(tz=datetime.timezone.utc)


This is relying on synchronization of clocks between the client and the server. Wouldn't it be better to set new_begin_at to be the latest result timestamp we see in the transform_results?

…/ServiceX_frontend into remove-s3-bucket-polling

for more information, see https://pre-commit.ci

…into remove-s3-bucket-polling

…/ServiceX_frontend into remove-s3-bucket-polling

…into remove-s3-bucket-polling

for more information, see https://pre-commit.ci

BenGalewsky · 2025-06-04T17:32:58Z

servicex/query_core.py

@@ -551,21 +557,27 @@ async def get_signed_url(
            if progress:
                progress.advance(task_id=download_progress, task_type="Download")

+        later_than: datetime.datetime | None = None


Why not initialize later_than to datetime.min to avoid all of the extra if later_than is None or constructs?

…into remove-s3-bucket-polling

MattShirley · 2025-06-12T19:22:53Z

Added the feature flag and updated tests. All work here is done and ready for final review.

BenGalewsky

I assume this has been tested with a serviceX deployment that doesn't yet have the capabilities property in the info?

MattShirley · 2025-06-17T18:39:47Z

I assume this has been tested with a serviceX deployment that doesn't yet have the capabilities property in the info?

Yes, I've tested this against ServiceX's develop branch locally (without the feature flag enabled). There's also parameterized testing added in this PR to test both when it is and isn't present.

ponyisi

Looks pretty good, see the few comments.

servicex/query_core.py

servicex/servicex_adapter.py

ponyisi · 2025-06-18T21:24:15Z

servicex/servicex_adapter.py

+            return self._servicex_info
+
+    async def get_servicex_capabilities(self) -> List[str]:
+        return (await self.get_servicex_info()).capabilities


Add a test for this line?

servicex/query_core.py

pyproject.toml

MattShirley and others added 4 commits May 6, 2025 15:50

remove s3 bucket polling when waiting for transformation results

90b6173

[pre-commit.ci] auto fixes from pre-commit.com hooks

ee7b02c

for more information, see https://pre-commit.ci

add begin_at, update tests

5b04365

[pre-commit.ci] auto fixes from pre-commit.com hooks

85b5346

for more information, see https://pre-commit.ci

MattShirley and others added 9 commits May 14, 2025 14:54

use python3.9 compliant method to get UTC time

b80d72d

Merge branch 'remove-s3-bucket-polling' of https://github.com/ssl-hep…

ccfe38c

…/ServiceX_frontend into remove-s3-bucket-polling

fix breaking tests

36cb4aa

add additional test coverage support

eb9a937

[pre-commit.ci] auto fixes from pre-commit.com hooks

bf805bb

for more information, see https://pre-commit.ci

flake8 compliance

9492997

Merge branch 'remove-s3-bucket-polling' of https://github.com/ssl-hep…

90a0a6b

…/ServiceX_frontend into remove-s3-bucket-polling

code coverage improvement

ab9d0cf

[pre-commit.ci] auto fixes from pre-commit.com hooks

3644ee1

for more information, see https://pre-commit.ci

MattShirley requested review from Copilot, ponyisi, BenGalewsky and kyungeonchoi May 20, 2025 21:29

Copilot AI reviewed May 20, 2025

View reviewed changes

servicex/servicex_adapter.py Outdated Show resolved Hide resolved

ponyisi reviewed May 20, 2025

View reviewed changes

BenGalewsky reviewed May 21, 2025

View reviewed changes

MattShirley and others added 9 commits May 22, 2025 09:55

add ServiceXFile class and refactor tests to match previous

9b506c5

Merge branch 'remove-s3-bucket-polling' of https://github.com/ssl-hep…

932a7e9

…/ServiceX_frontend into remove-s3-bucket-polling

[pre-commit.ci] auto fixes from pre-commit.com hooks

6d18ff3

for more information, see https://pre-commit.ci

Merge branch 'master' of https://github.com/ssl-hep/ServiceX_frontend …

04ae90c

…into remove-s3-bucket-polling

Merge branch 'remove-s3-bucket-polling' of https://github.com/ssl-hep…

c1da28f

…/ServiceX_frontend into remove-s3-bucket-polling

Merge branch 'master' of https://github.com/ssl-hep/ServiceX_frontend …

ffd87ad

…into remove-s3-bucket-polling

resolve breaking tests

98392dd

[pre-commit.ci] auto fixes from pre-commit.com hooks

8e952eb

for more information, see https://pre-commit.ci

flake8

232be85

BenGalewsky reviewed Jun 4, 2025

View reviewed changes

MattShirley added 10 commits June 4, 2025 13:27

simplify typing

ee0423f

add feature branching

f5bf546

add testing

5bbe40f

remove duplicated test

201531a

Merge branch 'master' of https://github.com/ssl-hep/ServiceX_frontend …

268592f

…into remove-s3-bucket-polling

fix breaking test after merging master in

7db79e1

try to fix breaking test on server

6e20fe6

improve code coverage

270bd8b

temporarily remove failing test in server (but not locally)

fc29f59

attempt to fix breaking server test

bf49d24

MattShirley requested review from ponyisi and BenGalewsky June 12, 2025 19:23

BenGalewsky reviewed Jun 16, 2025

View reviewed changes

MattShirley added 2 commits June 17, 2025 13:20

pull master and fix tests

b49edc7

fix broken test

aa3f2fb

ponyisi reviewed Jun 18, 2025

View reviewed changes

MattShirley added 6 commits June 20, 2025 07:49

update models

9785fc6

update tests

72c7bca

resolve merge conflicts

d218879

update pyproject

b0eee6c

get s3 file name directly from API

925c358

add warning notification and additional testing

7ddfc60

ponyisi reviewed Jun 25, 2025

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

ponyisi approved these changes Jun 25, 2025

View reviewed changes

remove pyproject changes

23a0f13

ponyisi merged commit d9511c9 into master Jun 25, 2025
35 checks passed

ponyisi deleted the remove-s3-bucket-polling branch June 25, 2025 23:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

remove s3 bucket polling when waiting for transformation results #587

remove s3 bucket polling when waiting for transformation results #587

Uh oh!

MattShirley commented May 6, 2025

Uh oh!

codecov bot commented May 14, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

ponyisi May 20, 2025

Uh oh!

BenGalewsky May 21, 2025

Uh oh!

BenGalewsky Jun 4, 2025

Uh oh!

MattShirley commented Jun 12, 2025

Uh oh!

BenGalewsky left a comment

Uh oh!

MattShirley commented Jun 17, 2025

Uh oh!

ponyisi left a comment

Uh oh!

Uh oh!

Uh oh!

ponyisi Jun 18, 2025

Uh oh!

MattShirley Jun 25, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

remove s3 bucket polling when waiting for transformation results #587

remove s3 bucket polling when waiting for transformation results #587

Uh oh!

Conversation

MattShirley commented May 6, 2025

Uh oh!

codecov bot commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

ponyisi May 20, 2025

Choose a reason for hiding this comment

Uh oh!

BenGalewsky May 21, 2025

Choose a reason for hiding this comment

Uh oh!

BenGalewsky Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

MattShirley commented Jun 12, 2025

Uh oh!

BenGalewsky left a comment

Choose a reason for hiding this comment

Uh oh!

MattShirley commented Jun 17, 2025

Uh oh!

ponyisi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ponyisi Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

MattShirley Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented May 14, 2025 •

edited

Loading