Migrate `JobFilesAPIController` to FastAPI (excluding TUS uploads) by kysrpex · Pull Request #20235 · galaxyproject/galaxy

kysrpex · 2025-05-15T14:14:25Z

As part of the development of an integration of Galaxy with ARC (Advanced Resource Connector) as a Pulsar job runner, tweaks to the JobFilesAPIController are needed. None of such tweaks are included in this PR, but it makes sense to implement them building upon a FastAPI endpoint rather than a legacy WSGI one; and the first step to do that is migrating the controller to FastAPI.

FastAPIJobFiles is the new, FastAPI version of JobFilesAPIController. The endpoints that have been migrated should exhibit exactly the same behavior as the old ones from FastAPIJobFiles.

Endpoints dedicated to TUS uploads work in tandem with the WSGI middleware TusMiddleware from the tuswsgi package. WSGI middlewares and endpoints are injected into the FastAPI app after FastAPI routes as a single sub-application wsgi_handler using app.mount("/", wsgi_handler), meaning that requests are passed to the wsgi_handler sub-application (and thus to TusMiddleware) only if there was no FastAPI endpoint defined to handle them. Therefore, they cannot be migrated to FastAPI unless TusMiddleware is also migrated to ASGI. I am postponing that migration, because the ARC integration needs to be delivered soonish.

I also included three new tests for existing functionality: writing from uploads done with the nginx_upload_module, from TUS uploads and using the parameter __file.

How to test the changes?

(Select all options that apply)

I've included appropriate automated tests.
This is a refactoring of components with existing test coverage.
Instructions for manual testing are as follows:
1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

kysrpex · 2025-05-15T14:18:41Z

Pulsar uses these endpoints, so before merging this, it is critical that it is passes all tests from test/integration/test_job_files.py and test_job_files_tus.py (./run_tests.sh -integration test/integration/test_job_files.py --, ./run_tests.sh -integration test/integration/test_job_files_tus.py --).

kysrpex · 2025-05-15T14:28:40Z

Locally, test/integration/test_job_files_tus.py::test_tools[simple_constructs] and test/integration/test_job_files_tus.py::test_tools[composite_output_tests] are failing because the tools produce different outputs from what's expected.

@jmchilton I could use some help from you (I see that you wrote test/integration/test_job_files_tus.py). Do you think the failures are related to the changes from this PR? Do you have any clue of what's failing before I look further into it? I would be quite grateful if you could have a look after the CI finishes running the tests.

lib/galaxy/webapps/galaxy/api/job_files.py

+
+    @router.post(
+        "/api/jobs/{job_id}/files",
+        summary="Populate an output file.",


lib/galaxy/webapps/galaxy/api/job_files.py

+        responses={
+            200: {"description": "An okay message.", "content": {"application/json": {"example": {"message": "ok"}}}},
+        },
+    )


lib/galaxy/webapps/galaxy/api/job_files.py

-            if os.path.exists(path) and (path.endswith("tool_stdout") or path.endswith("tool_stderr")):
-                with open(path, "ab") as destination:
-                    shutil.copyfileobj(open(input_file.name, "rb"), destination)
+        if os.path.exists(path) and (path.endswith("tool_stdout") or path.endswith("tool_stderr")):


lib/galaxy/webapps/galaxy/api/job_files.py

-                with open(path, "ab") as destination:
-                    shutil.copyfileobj(open(input_file.name, "rb"), destination)
+        if os.path.exists(path) and (path.endswith("tool_stdout") or path.endswith("tool_stderr")):
+            with open(path, "ab") as destination:


lib/galaxy/webapps/galaxy/api/job_files.py

+        if os.path.exists(path) and (path.endswith("tool_stdout") or path.endswith("tool_stderr")):
+            with open(path, "ab") as destination:
+                if input_file_path:
+                    with open(input_file_path, "rb") as input_file_handle:


lib/galaxy/webapps/galaxy/api/job_files.py

+            # (https://docs.python.org/3/library/tempfile.html#tempfile.SpooledTemporaryFile), so now there is not even
+            # a path where uploaded files can be accessed on disk
+            if input_file_path:
+                shutil.move(input_file_path, path)


lib/galaxy/webapps/galaxy/api/job_files.py

+            # (https://docs.python.org/3/library/tempfile.html#tempfile.SpooledTemporaryFile), so now there is not even
+            # a path where uploaded files can be accessed on disk
+            if input_file_path:
+                shutil.move(input_file_path, path)


lib/galaxy/webapps/galaxy/api/job_files.py

-                # tempfile has moved and Python wants to delete it.
-                pass
-        return {"message": "ok"}
+                with open(path, "wb") as destination:


kysrpex · 2025-05-15T14:43:40Z

About the CodeQL issues, although there is self.__check_job_can_write_to_path(trans, job, path), which does some permission checks (I did not check if it completely gets rid of the risk), I am migrating the endpoint, not improving it. Those issues were there already and unfortunately I cannot spend time at the moment in fixing them.

One solution is to add an exception. Another to delay the merge to see if there is some time remaining to fix them after the ARC integration has been worked through.

lib/galaxy/webapps/galaxy/api/job_files.py

+
+        job = self.__authorize_job_access(trans, job_id, path=path, job_key=job_key)
+
+        if not os.path.exists(path):


lib/galaxy/webapps/galaxy/api/job_files.py

-            if os.path.exists(path) and (path.endswith("tool_stdout") or path.endswith("tool_stderr")):
-                with open(path, "ab") as destination:
-                    shutil.copyfileobj(open(input_file.name, "rb"), destination)
+        if os.path.exists(path) and (path.endswith("tool_stdout") or path.endswith("tool_stderr")):


lib/galaxy/webapps/galaxy/api/job_files.py

-                with open(path, "ab") as destination:
-                    shutil.copyfileobj(open(input_file.name, "rb"), destination)
+        if os.path.exists(path) and (path.endswith("tool_stdout") or path.endswith("tool_stderr")):
+            with open(path, "ab") as destination:


lib/galaxy/webapps/galaxy/api/job_files.py

+        if os.path.exists(path) and (path.endswith("tool_stdout") or path.endswith("tool_stderr")):
+            with open(path, "ab") as destination:
+                if input_file_path:
+                    with open(input_file_path, "rb") as input_file_handle:


lib/galaxy/webapps/galaxy/api/job_files.py

+            # (https://docs.python.org/3/library/tempfile.html#tempfile.SpooledTemporaryFile), so now there is not even
+            # a path where uploaded files can be accessed on disk
+            if input_file_path:
+                shutil.move(input_file_path, path)


lib/galaxy/webapps/galaxy/api/job_files.py

+            # (https://docs.python.org/3/library/tempfile.html#tempfile.SpooledTemporaryFile), so now there is not even
+            # a path where uploaded files can be accessed on disk
+            if input_file_path:
+                shutil.move(input_file_path, path)


lib/galaxy/webapps/galaxy/api/job_files.py

-
-        See more discussion of checking upload access, but we shouldn't need the
-        API key and session stuff the user upload tusd server should be configured with.
+                with open(path, "wb") as destination:


kysrpex · 2025-05-20T13:33:54Z

Locally, test/integration/test_job_files_tus.py::test_tools[simple_constructs] and test/integration/test_job_files_tus.py::test_tools[composite_output_tests] are failing because the tools produce different outputs from what's expected.

@jmchilton I could use some help from you (I see that you wrote test/integration/test_job_files_tus.py). Do you think the failures are related to the changes from this PR? Do you have any clue of what's failing before I look further into it? I would be quite grateful if you could have a look after the CI finishes running the tests.

It was because the endpoint /api/jobs/{job_id}/files needs to accept path and and job_key both as query and form parameter for POST requests because most Galaxy tests submit the parameters as form parameters but Pulsar submits them as query parameters. It now does so (see d17002d). There are now no more relevant test failures.

kysrpex · 2025-05-27T10:15:06Z

@maikenp FYI

`FastAPIJobFiles` is the new, FastAPI version of `JobFilesAPIController`. The endpoints that have been migrated should exhibit exactly the same behavior as the old ones from `FastAPIJobFiles`. Something to keep in mind is that while FastAPI has some extra built-in features that the legacy WSGI system did not have, such as answering HEAD requests, those do not work because of the way legacy WSGI endpoints are injected into the FastAPI app (using `app.mount("/", wsgi_handler)`), meaning that for example, HEAD requests are passed to the `wsgi_handler` sub-application. Endpoints dedicated to TUS uploads work in tandem with the WSGI middleware `TusMiddleware` from the `tuswsgi` package. As explained above, WSGI middlewares and endpoints are injected into the FastAPI app after FastAPI routes as a single sub-application `wsgi_handler` using `app.mount("/", wsgi_handler)`, meaning that requests are passed to the `wsgi_handler` sub-application (and thus to `TusMiddleware`) only if there was no FastAPI endpoint defined to handle them. Therefore, they cannot be migrated to FastAPI unless `TusMiddleware` is also migrated to ASGI.

Work around a bug in FastAPI (fastapi/fastapi#13175) that assigns the same operation id to both request methods GET and HEAD of the endpoint `/api/jobs/{job_id}/files` when using the `@router.api_route()` decorator with `methods=["GET", "HEAD"]` as keyword argument.

…T requests to `/api/jobs/{job_id}/files` Pulsar formats the `path` and `job_key` parameters as query parameters when submitting POST requests to `/api/jobs/{job_id}/files`. However, many Galaxy tests format them as form parameters. The only way to keep the endpoint working as it should (as it worked before the migration to FastAPI) is to accept both query and form parameters.

… requests to `/api/jobs/{job_id}/files` FastAPI will not use the parameter aliases of form parameters in the OpenAPI docs, but the name of their Python variables. Therefore, the API docs show `path_form` and `job_key_form`. Rename them so that the API docs show the correct parameter names.

Change docstrings of `job_key_query_or_form()` and `path_query_or_form()` to reference the correct HTTP error code (400).

There was a small merge conflict with 7f89e0e, make sure changes are compliant with said commit.

kysrpex · 2025-09-09T12:44:40Z

@mvdbeek Any chance someone can review the PR? It is a pre-requisite to merge the ARC integration (#20598). I can offer direct assistance explaining whatever is needed, either via GitHub or a meeting.

mvdbeek

here's the (incomplete) review i never submitted, maybe you can take a look at this and I'll come back to this next week

mvdbeek · 2025-07-29T15:45:02Z

lib/galaxy/webapps/galaxy/api/job_files.py


-        :rtype:     binary
-        :returns:   contents of file
+    # FastAPI answers HEAD requests automatically for GET endpoints. However, because of the way legacy WSGI endpoints


Where's that info coming from ? This has been discussed but was never implemented (last attempt fastapi/fastapi#13509).

If you return FileResponse, then it's true fastapi/fastapi#1773 (comment). I incorrectly assumed it was generally true, because I just tried it with this endpoint.

I changed the comments so that they are precise 8def3a7. Thanks for letting me know about this.

mvdbeek · 2025-07-29T15:50:24Z

lib/galaxy/webapps/galaxy/api/job_files.py

+    @router.get(
+        # simplify me (remove `_args` and `_kwargs` defined using the walrus operator) when ALL endpoints have been
+        # migrated to FastAPI, this is a workaround for FastAPI bug https://github.com/fastapi/fastapi/issues/13175
+        *(_args := ["/api/jobs/{job_id}/files"]),
+        **(
+            _kwargs := dict(
+                summary="Get a file required to staging a job.",
+                responses={
+                    200: {
+                        "description": "Contents of file.",
+                        "content": {"application/json": None, "application/octet-stream": {"example": None}},
+                    },
+                    400: {
+                        "description": (
+                            "File not found, path does not refer to a file, or input dataset(s) for job have been purged."
+                        )
+                    },
+                },
+            )
+        ),
+    )
+    @router.head(*_args, **_kwargs)  # type: ignore[name-defined]
+    # remove `@router.head(...)` when ALL endpoints have been migrated to FastAPI


Can you follow the ordinary way of setting up fastapi endpoints, i.e. no argument unpacking and I don't think the explicit responses and summary keyword are needed either.

This is the result of adding the keywords.

...

I don't think there harm in neatly documenting the endpoint, especially because the cause of HTTP 400 errors is hard to guess.

Regarding argument unpacking, refer to this answer.

mvdbeek · 2025-07-29T15:51:12Z

lib/galaxy/webapps/galaxy/api/job_files.py

+router = Router(
+    # keep the endpoint in the undocumented section of the API docs `/api/docs`, as all endpoints from `FastAPIJobFiles`
+    # are certainly not meant to represent part of Galaxy's stable, user facing API
+    tags=["undocumented"]


That's meant for the stuff that hasn't been ported. There's no harm documenting these and making them available for client libraries as long as these are marked as internal.

Assigned "jobs" tag (c5c7d34).

mvdbeek · 2025-07-29T16:00:37Z

lib/galaxy/webapps/galaxy/api/job_files.py

+    #
+    @router.get(
+        # simplify me (remove `_args` and `_kwargs` defined using the walrus operator) when ALL endpoints have been
+        # migrated to FastAPI, this is a workaround for FastAPI bug https://github.com/fastapi/fastapi/issues/13175


That's maybe a little excessive, you can also pass in an explicit operation_id if you did this to avoid the warning.

The point of having both decorators @router.get() and @router.head() is indeed avoiding the warning.

If I have only one decorator @router.api_route(), then passing an explicit operation_id does not solve anything (both GET and HEAD get the same operation id).

If I use both decorators, then I never have to pass an explicit operation_id, because FastAPI will generate two different ones for me.

Regarding argument unpacking, the idea is that the same method index() has to be reused both for GET and HEAD. I am forced to use the two-decorator approach (to avoid the warning) and by definition, both the GET and HEAD endpoints must be defined in exactly the same way. I use argument unpacking to avoid repeating the arguments of the first decorator in the second decorator.

I realize however, that including both GET and HEAD in the API schema is redundant. Thus, I added include_in_schema=False to the @router.head() decorator (bed78be).

Sorry, I don't think my comment is addressed. Please prefer a bit of repetitive code over disabling type checks.

Since I set include_in_schema=False, @router.head() can now skip the kwargs and there would then be little repetition (only the path). Seems ok if it's just that what's repeated.

Addressed in b53fa35.

mvdbeek · 2025-07-29T16:02:07Z

lib/galaxy/webapps/galaxy/api/job_files.py

+
+    This method does not force the client to provide the parameter, it could simply not submit the parameter in either
+    format. To force the client to provide the parameter, coerce the output of the method to a string, i.e.
+    `job_key: str = Depends(job_key_query_or_form)` so that FastAPI responds with status code 500 when the parameter is


That should be a validation error, not an internal server error.

That's correct, fixed in 62d3499·

kysrpex · 2025-09-11T08:05:00Z

here's the (incomplete) review i never submitted, maybe you can take a look at this and I'll come back to this next week

Thank you, complete or not, that's some steam to get the engine running.

Including both GET and HEAD in the API schema for the same endpoint is redundant.

…ally It is only true for endpoints returning `FileResponse`, see fastapi/fastapi#1773 (comment).

Group job files API endpoints together with jobs API endpoints.

kysrpex · 2025-09-11T09:50:49Z

here's the (incomplete) review i never submitted, maybe you can take a look at this and I'll come back to this next week

Thank you, complete or not, that's some steam to get the engine running.

I addressed the comments, let's continue next week. Thanks again for moving things forward.

mvdbeek

This is a pretty important and performance sensitive endpoint. Have you tested all of the variants? Can you run a performance comparison (for speed and memory consumption) between the old and new endpoints ?

kysrpex · 2025-09-22T13:25:57Z

This is a pretty important and performance sensitive endpoint. Have you tested all of the variants? Can you run a a performance comparison (for speed and memory consumption) between the old and new endpoints ?

Thanks for coming back to this again.

Have you tested all of the variants?

Regarding test coverage I think we are safe. I added a couple tests to cover the code paths that were originally not covered. Luckily, besides the tests from test_job_files.py, there also exist tests that caught a nasty problem that would've been overlooked (see this comment).

Can you run a a performance comparison (for speed and memory consumption) between the old and new endpoints ?

You're right this is important. It can be the the next step to get this merged 👍. There's also this potential performance issue that's not simple to solve at all.

kysrpex · 2025-09-22T13:33:15Z

lib/galaxy/webapps/galaxy/api/job_files.py

+                with open(path, "wb") as destination:
+                    shutil.copyfileobj(cast(IO[bytes], input_file), destination)


This is the potential performance mentioned in this comment. For readability I just copied the new code below (the comment above the code explains the problem).

# prior to migrating to FastAPI, this operation was done more efficiently for all cases using # `shutil.move(input_file_path, path)`, but FastAPI stores the uploaded file as # `tempfile.SpooledTemporaryFile` # (https://docs.python.org/3/library/tempfile.html#tempfile.SpooledTemporaryFile), so now there is not even # a path where uploaded files can be accessed on disk if input_file_path: shutil.move(input_file_path, path) else: with open(path, "wb") as destination: shutil.copyfileobj(cast(IO[bytes], input_file), destination)

It's tricky because the limitation is in FastAPI's design.

you should be able to call rollover on the file and move it then ?

Thanksss, that's just what I was looking for!

Hmm there is a problem with this. Besides storing the data in memory until the file is rolled over to disk, tempfile.SpooledTemporaryFile operates exactly as tempfile.TemporaryFile. So even if rollover() is called, there is no path for that file (and there is nothing to call shutil.move() on).

Under Unix, the directory entry for the file is either not created at all or is removed immediately after the file is created. Other platforms do not support this; your code should not rely on a temporary file created using this function having or not having a visible name in the file system.

Definitely writing the file to disk is twice suboptiomal... 🤔

The point of having both decorators `@router.get()` and `@router.head()` is working around FastAPI issue fastapi/fastapi#13175. If there is only one decorator `@router.api_route()`, then both GET and HEAD get the same operation id. If both decorators are used, then FastAPI generates two different operation ids. The idea behind argument unpacking is that the same method `index()` has to be reused both for GET and HEAD. The two-decorator approach is needed to avoid the duplicate operation id warning and by definition, both the GET and HEAD endpoints must be defined in exactly the same way. Argument unpacking allows to avoid repeating the arguments of the first decorator in the second decorator. However, including both GET and HEAD in the API schema is redundant. Commit bed78be adds `include_in_schema=False` to the `@router.head()`. That implies that there is now not so much benefit in the argument unpacking trick, because the kwargs are no longer needed and there would then be little repetition (only the path). Thus, declare `@router.head()` explicitly.

github-actions bot added area/testing area/API area/testing/integration labels May 15, 2025

github-actions bot added this to the 25.1 milestone May 15, 2025

kysrpex added kind/refactoring cleanup or refactoring of existing code, no functional changes area/jobs labels May 15, 2025

kysrpex self-assigned this May 15, 2025

github-advanced-security bot found potential problems May 15, 2025

View reviewed changes

kysrpex force-pushed the job_files_fastapi_migration branch 2 times, most recently from 1a8b10b to d17002d Compare May 20, 2025 10:04

github-advanced-security bot found potential problems May 20, 2025

View reviewed changes

This was referenced May 26, 2025

Improve compliance of FastAPIJobFiles with the HTTP spec (files endpoint) #20351

Draft

Add PUT request support at /api/jobs/{job_id}/files to FastAPIJobFiles #20353

Draft

mvdbeek self-requested a review July 1, 2025 14:59

kysrpex mentioned this pull request Jul 3, 2025

Pulsar-based ARC Job Runner #20598

Open

4 tasks

kysrpex added 4 commits July 29, 2025 17:05

mvdbeek force-pushed the job_files_fastapi_migration branch from 071d2d5 to 7a11519 Compare July 29, 2025 15:06

kysrpex added 3 commits September 9, 2025 11:20

Fix HTTP error code typo

62d3499

Change docstrings of `job_key_query_or_form()` and `path_query_or_form()` to reference the correct HTTP error code (400).

Merge branch 'dev' into job_files_fastapi_migration

c8392df

Cleanup changes using pyupgrade

0c22d44

There was a small merge conflict with 7f89e0e, make sure changes are compliant with said commit.

mvdbeek reviewed Sep 10, 2025

View reviewed changes

kysrpex added 2 commits September 11, 2025 11:35

Hide HEAD method for /api/jobs/{job_id}/files in API docs

bed78be

Including both GET and HEAD in the API schema for the same endpoint is redundant.

Correct comment claiming that FastAPI answers HEAD requests automatic…

8def3a7

…ally It is only true for endpoints returning `FileResponse`, see fastapi/fastapi#1773 (comment).

kysrpex force-pushed the job_files_fastapi_migration branch from 49ac9c3 to 8def3a7 Compare September 11, 2025 09:36

Assign "jobs" tag to FastAPIJobFiles router

c5c7d34

Group job files API endpoints together with jobs API endpoints.

mvdbeek reviewed Sep 22, 2025

View reviewed changes

kysrpex commented Sep 22, 2025

View reviewed changes

ahmedhamidawan modified the milestones: 25.1, 26.0 Sep 23, 2025

ahmedhamidawan assigned mvdbeek Sep 23, 2025

ahmedhamidawan modified the milestones: 25.1, 26.0 Oct 7, 2025

kysrpex mentioned this pull request Oct 30, 2025

Replace tuswsgi with tuspyserver #21201

Merged

4 tasks

guerler modified the milestones: 26.0, 26.1 Jan 27, 2026


		job = self.__authorize_job_access(trans, job_id, path=path, job_key=job_key)

		if not os.path.exists(path):

		with open(path, "wb") as destination:
		shutil.copyfileobj(cast(IO[bytes], input_file), destination)

Conversation

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

Uh oh!

How to test the changes?

License

Uh oh!

kysrpex commented May 15, 2025

Uh oh!

kysrpex commented May 15, 2025

Uh oh!

Check failure

Uh oh!

Check failure

Uh oh!

Uh oh!

Check failure

Uh oh!

Check failure

Uh oh!

Check failure

Uh oh!

Uh oh!

Check failure

Uh oh!

Uh oh!

Check failure

Uh oh!

Check failure

Uh oh!

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

Uh oh!

Uh oh!

Check failure

Check failure

Check failure

Check failure

Check failure

Check failure

Check failure

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

Uh oh!

Uh oh!

kysrpex commented May 27, 2025

Uh oh!

kysrpex commented Sep 9, 2025

Uh oh!

mvdbeek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kysrpex Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kysrpex Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kysrpex Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mvdbeek Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kysrpex Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kysrpex Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

kysrpex commented May 15, 2025 •

edited

Loading

kysrpex commented May 15, 2025 •

edited

Loading

kysrpex commented May 20, 2025 •

edited

Loading

kysrpex Sep 11, 2025 •

edited

Loading

kysrpex Sep 11, 2025 •

edited

Loading

kysrpex Sep 11, 2025 •

edited

Loading

mvdbeek Sep 22, 2025 •

edited

Loading

kysrpex Sep 22, 2025 •

edited

Loading

kysrpex Sep 22, 2025 •

edited

Loading

kysrpex commented Sep 11, 2025 •

edited

Loading

mvdbeek left a comment •

edited

Loading

kysrpex commented Sep 22, 2025 •

edited

Loading

kysrpex Sep 22, 2025 •

edited

Loading