feat(file-service): complete the model backend for sharing and the hub - #7937
feat(file-service): complete the model backend for sharing and the hub#7937tanishqgandhi1908 wants to merge 116 commits into
Conversation
… tests Review fixes on apache#5911: use dataset owner email in retrieveLatestDatasetVersion, replace brittle Option.get/.head with headOption, strip datasets prefix on selection-modal reopen, clarify FileResolver docs; add FileResolver/DatasetFileNode and frontend path-helper tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The datasets logical-path prefix strips four leading segments (datasets/owner/dataset/version); update the cover-image test's input path to include the prefix so the extracted relative path is the file name, not an empty string. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Now that models are coming as a separate resource/table, the leading resource-type segment is what selects the backing table, so an unprefixed path can no longer be routed unambiguously. Make the "datasets" prefix required instead of a tolerated fallback, and migrate existing data. - FileResolver: a dataset path must start with the "datasets" segment; unprefixed paths are no longer treated as dataset paths. - pytexera DatasetFileDocument: same rule, mirroring the backend. - FileListerSourceOpExec: parse the now-prefixed datasetVersionPath (skip the "datasets" segment); extracted into a testable helper. - Migration (sql/updates/29.sql): prepend "datasets/" to legacy paths stored in workflow.content and workflow_version.content, covering both the fileName (scan sources) and datasetVersionPath (file lister) operator properties. Only values whose first two segments match an existing (user.email, dataset.name) are rewritten, so local paths and URLs are left untouched; email format is irrelevant (owner may be a username without "@"). Uses create_missing=false and is idempotent. - Example workflows: use datasets-prefixed paths. Tests: FileResolverSpec and WorkflowExecutionsResourceSpec updated; test_dataset_file_document.py updated; new FileListerSourceOpExecSpec. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…l-path-prefix # Conflicts: # frontend/src/app/common/type/datasetVersionFileTree.spec.ts
…format python test - dataset-selection-modal.component.spec.ts: expect the datasets-prefixed selectedPath in version (non-file) mode, matching the emitted path. - test_dataset_file_document.py: apply ruff format (wrap an over-length line) so `ruff format --check` passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The cover-image endpoints resolve a dataset path built as
{owner}/{name}/{coverImage}. With the prefix now required by FileResolver,
these must carry the datasets/ segment; add it in the set-cover,
get-cover redirect, and cover-url handlers so cover images resolve again.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…storage # Conflicts: # sql/changelog.xml
…rage # Conflicts: # common/workflow-core/src/test/scala/org/apache/texera/amber/core/storage/DocumentFactorySpec.scala
…l-path-prefix # Conflicts: # file-service/src/main/scala/org/apache/texera/service/type/dataset/DatasetFileNode.scala # file-service/src/test/scala/org/apache/texera/service/type/dataset/DatasetFileNodeSpec.scala # sql/changelog.xml # sql/updates/29.sql
…' into feat/model-file-storage # Conflicts: # common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/FileResolver.scala # common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/ResourceType.scala # common/workflow-core/src/test/scala/org/apache/texera/amber/storage/FileResolverSpec.scala # sql/changelog.xml # sql/updates/30.sql
…l-path-prefix # Conflicts: # sql/changelog.xml # sql/updates/30.sql
…' into feat/model-file-storage # Conflicts: # common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/ResourceType.scala # sql/changelog.xml # sql/texera_ddl.sql # sql/updates/31.sql
…l-path-prefix # Conflicts: # sql/changelog.xml # sql/updates/31.sql
…oad-engine # Conflicts: # file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala
…ix' into refactor/shared-upload-engine
…upload # Conflicts: # file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala # file-service/src/main/scala/org/apache/texera/service/resource/ModelResource.scala # sql/changelog.xml # sql/texera_ddl.sql
…nto feat/model-version-upload
Promoting the colliding leaf to a directory removed the object it stood for from the tree and from calculateTotalSize: for "model" (3 bytes) plus "model/weights.bin" (5 bytes) the reported size fell from 8 to 5 and "model" was no longer downloadable. That traded a duplicated row for lost data, on datasets as much as models. Register directories only, as before, and note why. The two objects surface as separate children with the same name, which is untidy but keeps every committed object listed, sized and downloadable. The frontend tolerates it: the tree component sets displayField "name" with no idField, so nodes carry generated ids and selection keys off node data, not name. The test now pins the integrity property instead of the tidiness one. The positional isFile fix is unaffected and still covered.
…lision # Conflicts: # file-service/src/test/scala/org/apache/texera/service/resource/DatasetResourceSpec.scala
…ix' into refactor/shared-upload-engine
…nto feat/model-version-upload
# Conflicts: # file-service/src/test/scala/org/apache/texera/service/resource/ModelResourcePermissionsSpec.scala
# Conflicts: # file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala # file-service/src/main/scala/org/apache/texera/service/resource/ResourceUploadService.scala
Adds the model endpoints a model page still lacked, so the sharing and hub
UI can be built as frontend-only work.
Cover images: POST /{mid}/update/cover, GET /{mid}/cover, GET /{mid}/cover-url.
The column already existed. The resource-agnostic halves move into a shared
CoverImageUtils and DatasetResource is refactored onto it (-73 lines), because
the image extension allowlist is a security control against stored XSS via a
presigned URL, and a duplicated allowlist drifts.
Anonymous version browsing: /{mid}/publicVersion/list and
/{mid}/publicVersion/{mvid}/rootFileNodes. Without these a logged-out visitor
on a public model page gets metadata and then an error.
Framework/format editing: POST /update/framework and POST /update/format, plus
"other" in both whitelists. The labels were create-only, so a mislabelled model
had to be recreated.
Routing: /api/model and /api/access/model in nginx, the k8s gateway and the
frontend dev proxy. nginx matches prefixes byte-wise, so the location needs a
trailing slash or it swallows the LLM /api/models route; the Gateway API matches
whole segments and does not have that problem.
Datasets gain a cover-path length check they did not have. dataset.cover_image
is varchar(246) via sql/updates/18.sql, so an over-long path was a jOOQ-wrapped
500 and is now a 400.
Automated Reviewer SuggestionsBased on the
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7937 +/- ##
============================================
+ Coverage 93.21% 93.23% +0.01%
- Complexity 4664 4677 +13
============================================
Files 1175 1176 +1
Lines 47646 47691 +45
Branches 5319 5329 +10
============================================
+ Hits 44413 44464 +51
+ Misses 1752 1749 -3
+ Partials 1481 1478 -3
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 405 | 0.247 | 24,499/32,441/32,441 us | 🔴 -10.2% / 🔴 +130.6% |
| 🔴 | bs=100 sw=10 sl=64 | 934 | 0.57 | 105,597/134,516/134,516 us | 🔴 +14.6% / 🔴 +39.4% |
| ⚪ | bs=1000 sw=10 sl=64 | 1,102 | 0.672 | 909,838/947,645/947,645 us | ⚪ within ±5% / ⚪ within ±5% |
Baseline details
Latest main 6b8027e from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 405 tuples/sec | 451 tuples/sec | 863.81 tuples/sec | -10.2% | -53.1% |
| bs=10 sw=10 sl=64 | MB/s | 0.247 MB/s | 0.275 MB/s | 0.527 MB/s | -10.2% | -53.2% |
| bs=10 sw=10 sl=64 | p50 | 24,499 us | 22,571 us | 11,488 us | +8.5% | +113.3% |
| bs=10 sw=10 sl=64 | p95 | 32,441 us | 31,391 us | 14,067 us | +3.3% | +130.6% |
| bs=10 sw=10 sl=64 | p99 | 32,441 us | 31,391 us | 17,591 us | +3.3% | +84.4% |
| bs=100 sw=10 sl=64 | throughput | 934 tuples/sec | 980 tuples/sec | 1,127 tuples/sec | -4.7% | -17.1% |
| bs=100 sw=10 sl=64 | MB/s | 0.57 MB/s | 0.598 MB/s | 0.688 MB/s | -4.7% | -17.1% |
| bs=100 sw=10 sl=64 | p50 | 105,597 us | 103,084 us | 89,893 us | +2.4% | +17.5% |
| bs=100 sw=10 sl=64 | p95 | 134,516 us | 117,393 us | 96,506 us | +14.6% | +39.4% |
| bs=100 sw=10 sl=64 | p99 | 134,516 us | 117,393 us | 104,915 us | +14.6% | +28.2% |
| bs=1000 sw=10 sl=64 | throughput | 1,102 tuples/sec | 1,106 tuples/sec | 1,156 tuples/sec | -0.4% | -4.6% |
| bs=1000 sw=10 sl=64 | MB/s | 0.672 MB/s | 0.675 MB/s | 0.705 MB/s | -0.4% | -4.7% |
| bs=1000 sw=10 sl=64 | p50 | 909,838 us | 900,978 us | 886,448 us | +1.0% | +2.6% |
| bs=1000 sw=10 sl=64 | p95 | 947,645 us | 986,539 us | 930,093 us | -3.9% | +1.9% |
| bs=1000 sw=10 sl=64 | p99 | 947,645 us | 986,539 us | 963,104 us | -3.9% | -1.6% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,494.29,200,128000,405,0.247,24499.13,32440.86,32440.86
1,100,10,64,20,2140.81,2000,1280000,934,0.570,105597.10,134516.07,134516.07
2,1000,10,64,20,18154.26,20000,12800000,1102,0.672,909837.68,947645.21,947645.21…d-public-apis # Conflicts: # file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala # file-service/src/main/scala/org/apache/texera/service/resource/ModelResource.scala # file-service/src/main/scala/org/apache/texera/service/resource/ResourceUploadService.scala # file-service/src/test/scala/org/apache/texera/service/resource/ModelApiForUiSpec.scala # file-service/src/test/scala/org/apache/texera/service/resource/ModelDownloadResourceSpec.scala # file-service/src/test/scala/org/apache/texera/service/resource/ModelResourcePermissionsSpec.scala # file-service/src/test/scala/org/apache/texera/service/resource/ModelUploadResourceSpec.scala # sql/updates/41.sql
|
/request-review @aicam |
aicam
left a comment
There was a problem hiding this comment.
A couple of things I hit while reading the new cover-image and label-update endpoints, both of which would surface to end users once a model UI is built on this. Nothing blocking.
…date Review feedback on apache#7937. An unresolvable cover path escaped as an IOException from FileResolver, which file-service registers no mapper for, so it surfaced as an opaque 500. Two guards: validatePathOrThrow now requires <version>/<file>, since a bare file name builds a 4-segment logical path parsePrefixedPath cannot parse; and openCover returns an Option, so the write path answers 400 and the reads treat an unresolvable cover as absent rather than failing every card render. The dataset half had the same gap before this branch and is fixed by the same code. The framework and format update endpoints validated more strictly than createModel: a null rendered as "Unsupported framework 'null'", an untrimmed value was accepted at creation and rejected on update, and format had no route back to the unset state create can produce. Both paths now share normalizeLabel.
What changes were proposed in this PR?
#7922 gave models the file API the management UI needs. This PR adds the last backend pieces, so model sharing and the hub can be built as frontend-only work.
Cover images —
POST /{mid}/update/cover,GET /{mid}/cover(307 redirect),GET /{mid}/cover-url(JSON, since<img src>cannot attach the Authorization header on a private model). Thecover_imagecolumn already existed.The resource-agnostic halves move into a shared
CoverImageUtilsandDatasetResourceis refactored onto it, net −55 lines there. The image extension allowlist is a security control, not a convention — a cover is handed to the browser as a presigned URL, so an active document (.svg,.html) would be a stored-XSS vector, and a duplicated allowlist drifts.Anonymous version browsing —
/{mid}/publicVersion/listand/{mid}/publicVersion/{mvid}/rootFileNodes. Without these a logged-out visitor on a public model page gets metadata and then an error.Framework/format editing —
POST /update/framework,POST /update/format, plusotherin both whitelists. The labels were create-only, so a mislabelled model had to be recreated.Routing —
/api/modeland/api/access/modelin nginx, the k8s gateway and the frontend dev proxy. Nothing reaches the resource in a real deployment without them. The nginx location needs a trailing slash: nginx matches prefixes byte-wise, so a bare/api/modelwould swallow the LLM/api/modelsroute. The Gateway API matches whole path segments and does not have that problem.One behavior change on the dataset side: covers now get a path-length check.
dataset.cover_imageisvarchar(246)viasql/updates/18.sql, so an over-long path was a jOOQ-wrapped 500 and is now a 400.Any related issues, documentation, discussions?
mainhas been merged in, so the diff is now this PR's work alone: 10 files, +972/−90. No migration, no schema change.How was this PR tested?
New
CoverImageUtilsSpec(18 cases) pins the allowlist,.svg/.htmlrejection, path traversal, absolute paths and both column-width limits. NewModelHubApiSpec(21 cases) drives the covers end to end through LakeFS/MinIO and covers anonymous browsing, including the negative cases: private model anonymous and no-grant, unknown model and version id, a model unpublished after the fact, and agreement between the public and authenticated file-tree endpoints.ModelApiForUiSpecgains the framework/format update cases.ModelResourcePermissionsSpecneeded the four new@PermitAllendpoint names — the guardrail caught them, as intended.DatasetResourceSpec's 152 tests pass unmodified after the refactor.284 tests across the eight affected suites, all passing after the merge of
main:Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5)