refactor: decouple FilesetFileSystem from Starlette test internals and improve upload/download paths - #629
Merged
Merged
Conversation
ironcommit
changed the title
Contributor
📝 WalkthroughWalkthroughAdds an httpx-compatible adapter around FastAPI/Starlette’s ChangesSDK test client compatibility
Sequence Diagram(s)sequenceDiagram
participant NeMoPlatform
participant SDKTestClientAdapter
participant TestClient
participant FilesystemTransportDetection
NeMoPlatform->>SDKTestClientAdapter: send(httpx.Request)
SDKTestClientAdapter->>TestClient: request(method, URL, headers, body)
TestClient-->>SDKTestClientAdapter: response
SDKTestClientAdapter-->>NeMoPlatform: httpx.Response
FilesystemTransportDetection->>SDKTestClientAdapter: read app
FilesystemTransportDetection->>FilesystemTransportDetection: create ASGITransport(app)
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/nmp_testing/src/nmp/testing/client.py`:
- Around line 19-20: The imports in the client adapter are relying on httpx
private internals, so update the module to use only public httpx APIs and type
definitions. In the client.py module, replace the direct imports of
UseClientDefault and AuthTypes from internal httpx modules with their public
equivalents from httpx’s exposed typing/API surface, and adjust any annotations
in the surrounding client code that reference those types so the adapter no
longer depends on internal symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ba76e98f-f680-4fa8-9dde-e4590143a9d8
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
packages/nmp_testing/pyproject.tomlpackages/nmp_testing/src/nmp/testing/client.py
Contributor
|
ironcommit
force-pushed
the
starlette-depr
branch
2 times, most recently
from
July 9, 2026 22:44
301fb04 to
869080a
Compare
mckornfield
reviewed
Jul 9, 2026
mckornfield
approved these changes
Jul 9, 2026
ironcommit
force-pushed
the
starlette-depr
branch
from
July 10, 2026 03:02
869080a to
3c28af1
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/nmp_testing/src/nmp/testing/client.py (1)
84-94: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDon't coerce headers to
dict.dict(request.headers)anddict(response.headers)collapse repeated headers (Set-Cookiebecomes comma-joined). Pass thehttpx.Headersobjects through unchanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nmp_testing/src/nmp/testing/client.py` around lines 84 - 94, Preserve repeated headers by passing the existing httpx.Headers objects unchanged in the request and response handling within the test client method: remove dict() coercion from headers=dict(request.headers) and headers=dict(response.headers), while keeping the rest of the request and httpx.Response construction intact.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/nmp_testing/src/nmp/testing/client.py`:
- Around line 84-94: Preserve repeated headers by passing the existing
httpx.Headers objects unchanged in the request and response handling within the
test client method: remove dict() coercion from headers=dict(request.headers)
and headers=dict(response.headers), while keeping the rest of the request and
httpx.Response construction intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 653d74fe-91f9-476f-81c4-f32ce121278f
⛔ Files ignored due to path filters (3)
sdk/python/nemo-platform/src/nemo_platform/filesets/filesystem/filesystem.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/integration/conftest.pyis excluded by!sdk/**uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
packages/filesets/src/filesets/filesystem/filesystem.pypackages/nemo_platform_ext/tests/cli/integration/conftest.pypackages/nmp_common/tests/sdk_factory/test_sdk.pypackages/nmp_testing/pyproject.tomlpackages/nmp_testing/src/nmp/testing/__init__.pypackages/nmp_testing/src/nmp/testing/client.pyservices/core/auth/tests/integration/conftest.pyservices/core/jobs/tests/integration/test_task_auth_runtime.pyservices/core/secrets/tests/conftest.pyservices/guardrails/tests/integration/test_guardrails.pyservices/hello-world/tests/integration/test_hello_world.pyservices/intake/tests/integration/test_intake.py
✅ Files skipped from review due to trivial changes (1)
- packages/nmp_testing/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/filesets/src/filesets/filesystem/filesystem.py
- services/core/jobs/tests/integration/test_task_auth_runtime.py
- services/intake/tests/integration/test_intake.py
- packages/nemo_platform_ext/tests/cli/integration/conftest.py
- packages/nmp_common/tests/sdk_factory/test_sdk.py
- services/guardrails/tests/integration/test_guardrails.py
- services/hello-world/tests/integration/test_hello_world.py
- packages/nmp_testing/src/nmp/testing/init.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@services/core/files/tests/integration/conftest.py`:
- Line 203: Remove the `default_client_factory` import from
`huggingface_hub.utils._http` and refactor the fixture using only public
`huggingface_hub` APIs, avoiding any attempt to restore the client factory
through internal helpers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ac68f631-f2ac-4c39-84b9-947f16f4f1ee
📒 Files selected for processing (1)
services/core/files/tests/integration/conftest.py
ironcommit
force-pushed
the
starlette-depr
branch
from
July 10, 2026 17:01
916a68a to
604dbf3
Compare
Signed-off-by: Ryan S <267728323+ironcommit@users.noreply.github.com>
Signed-off-by: Ryan S <267728323+ironcommit@users.noreply.github.com>
ironcommit
force-pushed
the
starlette-depr
branch
from
July 10, 2026 20:45
d1109e2 to
9f7e6c4
Compare
ironcommit
changed the title
This was referenced Jul 13, 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
2 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
Remove Starlette-specific hacks from
FilesetFileSystemand the SDK test infrastructure, replacing them with clean interfaces that properly bridge theTestClient ↔ httpx.Clientgap.Changes
FilesetFileSystem (
packages/filesets,sdk/python)fsspec.asyn._run_coros_in_chunksmonkey-patch — the customrun_coros_in_chunksis still used, but no longer replaces fsspec's global helper_detect_async_transport— this Starlette-aware hack extractedASGITransportfrom aTestClientat runtime; the async client is now injected explicitly via a newasync_clientparameter onFilesetFileSystem.__init___putoverride — properly handles local→remote path expansion (glob, recursive, directory creation) instead of relying on the base-class implementation that doesn't work with fileset refsmodeparameter to_pipe_fileand_put_filesignaturesout: dict[str, FileInfo],ListFilesQueryParamsfor query dicts_getfor mixed list/string argumentsSDK test infrastructure (
packages/nmp_testing)SDKTestClientAdapter— anhttpx.Clientsubclass that delegates.send()to Starlette'sTestClient.request(), preserving request headers and body_install_asgi_files_resource— wires the ASGI-backedAsyncFilesClientinto the sync SDK's.filesresource (and patches.copy/.with_optionsso cloned SDKs keep the same transport)httpx2>=2.0.0dependency tonmp_testingSDKTestClientAdapterandClientContextfromnmp.testingConsumers updated
data_designer_nemo— usesdk.files.fsspecinstead of manually constructingFilesetFileSystemviaclient_from_platformnemo_platform_plugin(file_manager) — same simplificationfilesets/resources.py— accept optionalasync_files_client, useDEFAULT_CALLBACKinstead of conditional kwargs, useCacheStatusenum valuesfiles,auth,secrets,guardrails,hello_world,intake, and CLI tests — updated to useSDKTestClientAdapterandClientContextdocker.io/library/alpine:3.20)Why
The previous approach detected Starlette's
TestClientat runtime insideFilesetFileSystemto extract anASGITransport— a fragile coupling that broke when the SDK's HTTP client interface changed. The new approach keeps test transport concerns in the test infrastructure where they belong, and givesFilesetFileSystema cleanasync_clientinjection point.Summary by CodeRabbit
New Features
Bug Fixes