Fleets results: return presigned URL redirect instead of buffering content#2203
Open
avilches wants to merge 36 commits into
Open
Fleets results: return presigned URL redirect instead of buffering content#2203avilches wants to merge 36 commits into
avilches wants to merge 36 commits into
Conversation
avilches
changed the title
avilches
changed the title
Contributor
Author
|
The PR shares code from #2186, so it needs to be merged after it (and probably solve conflicts) |
avilches
force-pushed
the
worktree-fleet-results-presigned-url
branch
from
0961a34 to
928da2c
Compare
avilches
commented
Jun 8, 2026
| request=lambda: requests.get( | ||
| url, | ||
| headers=get_headers(token=self.token, instance=self.instance, channel=self.channel), | ||
| params={"with_result": "false"}, |
Contributor
Author
There was a problem hiding this comment.
Default for with_result is true, so now has been changed explicitly to false, so the Gateway skips the COS read and returns result: null. This does not break anything because job() don't use the result.
avilches
commented
Jun 8, 2026
Comment on lines
+416
to
+429
| gateway_url = f"{self.host}/api/{self.version}/jobs/{job_id}/result/" | ||
| response = requests.get( | ||
| gateway_url, | ||
| headers=get_headers(token=self.token, instance=self.instance, channel=self.channel), | ||
| timeout=REQUESTS_TIMEOUT, | ||
| ) | ||
| return json.loads(response_data.get("result", "{}") or "{}", cls=QiskitObjectsDecoder) | ||
| if response.status_code == 204: | ||
| return None | ||
| # Not all redirects go to COS — HTTP→HTTPS redirects stay on the same host. | ||
| # Checking the hostname detects only redirects to an external host (COS/MinIO). | ||
| redirected_to_cos = urlparse(response.url).hostname != urlparse(gateway_url).hostname | ||
| if redirected_to_cos: | ||
| return json.loads(response.text, cls=QiskitObjectsDecoder) if response.ok else None | ||
| return json.loads(response.json().get("result", "{}") or "{}", cls=QiskitObjectsDecoder) |
Contributor
Author
There was a problem hiding this comment.
Use the new /result endpoint following the same approach than /logs and /provider-logs
avilches
commented
Jun 8, 2026
|
|
||
| @dataclass | ||
| class LogsResult: | ||
| class GetLogsResponse: |
Contributor
Author
There was a problem hiding this comment.
LogsResult was a terrible name. Now, GetJobLogsUseCase -> returns GetJobLogsResponse and GetResultUseCase -> returns GetResultResponse
… to check raw_result is None
avilches
commented
Jun 9, 2026
…-url' into worktree-fleet-results-presigned-url
avilches
commented
Jun 9, 2026
avilches
commented
Jun 9, 2026
Tansito
requested changes
Jun 9, 2026
|
|
||
|
|
||
| @swagger_auto_schema( | ||
| method="post", |
Member
There was a problem hiding this comment.
add a swagger auto schema for GET
Tansito
requested changes
Jun 9, 2026
| if result is not None: | ||
| job.result = result | ||
| # with_result is legacy, new clients will use the new GET /:id/results endpoints | ||
| if with_result: |
Member
There was a problem hiding this comment.
We need to ensure then that this with_result is false. I think we need to change it in the serializer if I remember correctly.
Contributor
Author
There was a problem hiding this comment.
Confirmed: the default value was true. I changed to false in this commit
avilches
commented
Jun 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Continuing the approach from #2186 with the Fleet logs. This PR adds
GET /jobs/{id}/result/as a new endpoint for downloiad job results and updates the Python client to use it.New endpoint:
GET /jobs/{id}/result/Serves both Ray and Fleet jobs, but behaves differently by runner:
results.jsonexists in COS and returns 302 redirect to a presigned URL (result ready) or 204 No Content (not ready yet). The gateway never download the COS content.The existing
GET /jobs/{id}/endpoint is unchanged and still supports?with_result=truefor backward compatibility with older clients.Client:
result()GET /jobs/{id}/?with_result=true.GET /jobs/{id}/result/and handles 302 (follows redirect to COS, decodes JSON from the COS response), 204 (returnsNone), and 200 (decodesresponse["result"]as before for Ray jobs).Client:
job()GET /jobs/{id}/without an explicitwith_resultparameter. The server defaulted towith_result=true, so it read the result from COS or the local filesystem on every call.with_result=false. The server skips the storage read entirely and returnsresult: null. This does not break anything:job()only usesidandcompute_profilefrom the response and never exposed the result field to the caller. The result has always been fetched separately viaresult().