Support custom file names for multi-session models - #1768
Open
yushuosun wants to merge 1 commit into
Open
Conversation
There was a problem hiding this comment.
馃煛 Changes recommended
It introduces/retains regressions in hub caching/error semantics and reduces critical error-path test coverage that should be preserved alongside the new filename-override behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR restores/extends support for overriding ONNX model file names in multi-session models by allowing model_file_name to be either a string (single-session) or a per-session/per-default-name mapping (multi-session), and threads that option through model/pipeline file resolution.
Changes:
- Add
getSessionsConfiglogic to apply per-session/default-name file-name overrides for multi-session models. - Propagate
model_file_namethrough pipeline file resolution and update registry-facing JSDoc to reflectstring | Record<string,string>. - Update/add tests for override behavior and model file resolution.
File summaries
| File | Description |
|---|---|
| packages/transformers/tests/utils/session_config.test.js | Replaces prior session-config tests with file-name override coverage for single- and multi-session models. |
| packages/transformers/tests/utils/model_registry.test.js | Adds a get_model_files test for seq2seq file-name mappings; removes several error-path tests for get_available_dtypes. |
| packages/transformers/src/utils/model_registry/ModelRegistry.js | Updates JSDoc to document model_file_name as string-or-mapping. |
| packages/transformers/src/utils/model_registry/get_pipeline_files.js | Uses getSessionsConfig so text-only filtering respects runtime session config + file-name overrides. |
| packages/transformers/src/utils/model_registry/get_model_files.js | Updates JSDoc for mapping support (implementation already uses getSessionsConfig). |
| packages/transformers/src/utils/model_registry/get_files.js | Updates JSDoc for mapping support (but contains a `null |
| packages/transformers/src/utils/model_registry/get_available_dtypes.js | Updates JSDoc for mapping support. |
| packages/transformers/src/utils/hub.js | Updates docs and changes caching/error handling paths related to resource loading. |
| packages/transformers/src/pipelines.js | Passes model_file_name through to get_pipeline_files. |
| packages/transformers/src/models/session_config.js | Introduces applyModelFileNames + updates getSessionsConfig to support mapping overrides. |
| packages/transformers/src/models/modeling_utils.js | Switches session resolution to getSessionsConfig for consistent override handling. |
Review details
Suppressed comments (1)
packages/transformers/src/utils/hub.js:330
- This invalid-model-id path is also a missing/inaccessible-file condition (no remote fetch will be attempted). Throwing
ModelFileNotFoundErrorkeeps error handling consistent with other "missing model" cases.
if (!validModelId) {
// Before making any requests to the remote server, we check if the model ID is valid.
// This prevents unnecessary network requests for invalid model IDs.
throw Error(
`Local file missing at "${localPath}" and download aborted due to invalid model ID "${path_or_repo_id}".`,
);
- Files reviewed: 11/11 changed files
- Comments generated: 6
- Review effort level: Lite
馃挕 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
11
to
15
| import { | ||
| handleError, | ||
| isValidUrl, | ||
| pathJoin, | ||
| isValidHfModelId, |
Comment on lines
+198
to
206
| export async function storeCachedResource(path_or_repo_id, filename, cache, cacheKey, response, result, options = {}) { | ||
| // Check again whether request is in cache. If not, we add the response to the cache | ||
| if ((await cache.match(cacheKey)) !== undefined) { | ||
| return; | ||
| } | ||
|
|
||
| if ( | ||
| typeof Cache !== 'undefined' && | ||
| cache instanceof Cache && | ||
| !isValidUrl(toAbsoluteURL(cacheKey, { allowUnresolved: true }), ['http:', 'https:']) | ||
| ) { | ||
| // The browser Cache API only supports http(s) URLs as keys, so do not attempt to cache | ||
| // responses for other schemes (e.g., files bundled within a browser extension). Relative | ||
| // keys are resolved against the page URL first, since a relative key on an extension page | ||
| // still resolves to a chrome-extension:// request. | ||
| return; | ||
| } | ||
|
|
||
| if (!result) { | ||
| // We haven't yet read the response body, so we need to do so now. | ||
| // Ensure progress updates include consistent metadata. |
Comment on lines
+316
to
318
| throw Error( | ||
| `\`local_files_only=true\` or \`env.allowRemoteModels=false\` and file was not found locally at "${localPath}".`, | ||
| ); |
| * @param {import('../dtypes.js').DataType|Record<string, import('../dtypes.js').DataType>} [options.dtype=null] Override dtype (use this if passing dtype to pipeline) | ||
| * @param {import('../devices.js').DeviceType|Record<string, import('../devices.js').DeviceType>} [options.device=null] Override device (use this if passing device to pipeline) | ||
| * @param {string|null} [options.model_file_name=null|null] Override the model file name (excluding .onnx suffix) | ||
| * @param {string|Record<string, string>|null} [options.model_file_name=null|null] Override model file names (excluding .onnx suffix) |
Comment on lines
+186
to
199
| describe("get_model_files", () => { | ||
| it("should support custom file names for seq2seq models", async () => { | ||
| const files = await get_model_files("test/model", { | ||
| config: SEQ2SEQ_CONFIG, | ||
| dtype: "fp32", | ||
| model_file_name: { | ||
| encoder_model: "custom_encoder", | ||
| decoder_model_merged: "custom_decoder", | ||
| }, | ||
| }); | ||
|
|
||
| it("should rethrow access errors from dtype file probes", async () => { | ||
| mockGetFileMetadata.mockRejectedValue(new ModelFileNotFoundError('Forbidden access to file: "https://huggingface.co/test/model/resolve/main/onnx/model.onnx".', { status: 403 })); | ||
|
|
||
| await expect(get_available_dtypes("test/model", { config: ENCODER_ONLY_CONFIG })).rejects.toThrow(ModelFileNotFoundError); | ||
| }); | ||
| expect(files).toEqual(["config.json", "onnx/custom_encoder.onnx", "onnx/custom_decoder.onnx", "generation_config.json"]); | ||
| }); | ||
| }); |
Comment on lines
+37
to
48
| it("keeps string overrides working for single-session models", () => { | ||
| const { sessions } = getSessionsConfig( | ||
| MODEL_TYPES.DecoderOnly, | ||
| {}, | ||
| { | ||
| model_file_name: "custom_model", | ||
| }, | ||
| ); | ||
|
|
||
| expect(sessions).toEqual({ model: "custom_model" }); | ||
| }); | ||
| }); |
Contributor
Author
|
I opened a refreshed PR against current main: #1770 |
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
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.
Replaces the stale PR branch with a fresh base on latest main to unblock mergeability.
This restores support for per-session file-name overrides in multi-session models: