feat(loader): add pipeline_tag fallback to task detection#1113
feat(loader): add pipeline_tag fallback to task detection#1113xieofxie wants to merge 3 commits into
Conversation
When architecture-based task detection (Stage 1c) fails because config.architectures contains a generic name like 'Model', fall back to querying the HuggingFace Hub pipeline_tag (Stage 1e) before the last-resort feature-extraction default (Stage 1d). - Add TaskSource.PIPELINE_TAG enum value for provenance tracking - Add Stage 1e in resolve_task() between 1c and 1d: - Checks model_id is not a local path via _is_local_path - Queries HfApi().model_info(model_id).pipeline_tag - Validates against KNOWN_TASKS after normalize_task() - Wrapped in try/except for graceful fallthrough on errors - Add 4 tests covering valid tag, invalid task, API failure, local path Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Addresses CodeQL 'Empty except' finding by documenting that the pass is intentional — network errors, invalid model IDs, and missing pipeline_tag all gracefully fall through to Stage 1d. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
a follow up of #1094 tested via winml export -m audeering/wav2vec2-large-robust-24-ft-age-gender -o temp/wav2vec2.onnx |
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
Thanks for the pipeline_tag fallback — the overall shape is right: it only runs on the last-resort path (opt_task is None), guards against local paths, and falls through gracefully. A few things worth addressing before merge, left inline:
- resolve_task now does blocking network I/O on a path that used to be purely local — no timeout, no caching, and the docstring's stage list wasn't updated.
- The bare
except Exception: passdrops all diagnostics. - The local-path negative test can't actually fail, because the AssertionError it raises is swallowed by that same
except Exception.
…ging
- Extract get_pipeline_tag() helper in hub_utils.py:
- Encapsulates _is_local_path check, HfApi call with 10s timeout,
and logger.debug on failure (matching existing hub_utils conventions)
- Simplify Stage 1e in resolution.py to call get_pipeline_tag() directly
- Update resolve_task docstring to mention pipeline-tag stage
- Rewrite tests to mock get_pipeline_tag at source, verify call args
Addresses review feedback:
- Timeout on HfApi().model_info() (was unbounded)
- logger.debug breadcrumb instead of bare pass
- Centralized Hub access (no more reaching into private _is_local_path)
- Stronger test assertion (mock_tag.assert_called_once_with)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
Reviewed the pipeline_tag fallback — the core idea is sound and the resolve_task dispatch is well covered. A few things inline worth a look: the KNOWN_TASKS gate admits display-only tasks that aren't ONNX-exportable; this adds a Hub network call to the shared resolve_task path (build/eval/inference, not just inspect); the 1e/1d labels read out of execution order; and the local-path test mocks the function under test so it doesn't cover the skip.
One more, not inline: the PR description looks slightly stale — it says the HfApi().model_info(...) call is inlined in resolution.py and lists 2 changed files, but the implementation factors it into a get_pipeline_tag helper in utils/hub_utils.py (3 files). Worth updating so the description matches.
| tag = get_pipeline_tag(model_id) | ||
| if tag: | ||
| normalized_tag = normalize_task(tag) | ||
| if normalized_tag in KNOWN_TASKS: |
There was a problem hiding this comment.
KNOWN_TASKS is the display/validation taxonomy and includes tasks that have no ONNX export path (text-to-image, reinforcement-learning, time-series-forecasting, any-to-any, inpainting, ...). I checked, and sentence-similarity is safe because optimum's map_from_synonym collapses it to feature-extraction inside normalize_task — but those others pass through unchanged. So for a model whose pipeline_tag is one of them and whose Stage 1c failed, we'd now set opt_task to a non-exportable task instead of the old feature-extraction default. For inspect that's arguably more accurate, but on the build/eval path it turns a usable default into a hard failure at export / model-class resolution. Consider gating on the exportable set (e.g. intersect with get_supported_tasks / what TasksManager can resolve) rather than the full KNOWN_TASKS.
| opt_task = None | ||
|
|
||
| # 1e. Hub pipeline_tag fallback | ||
| if opt_task is None and model_id: |
There was a problem hiding this comment.
This puts a Hub round-trip (model_info) on resolve_task, which is shared by build / eval / inference too, not just inspect — those callers reach here whenever --task isn't passed and arch detection fails. Task resolution has been deliberately offline/local up to now, so worth confirming we want the network reach on all of those paths vs. scoping it to inspect. Offline mode degrades fine (the except returns None) and it's guarded to the fallback, so this is more "is this intended" than a bug. Minor: model_info retries transient errors, so worst-case latency can exceed the 10s timeout.
| except ValueError: | ||
| opt_task = None | ||
|
|
||
| # 1e. Hub pipeline_tag fallback |
There was a problem hiding this comment.
Nit: this block is labeled 1e but executes before 1d (last-resort default), so the code reads 1c -> 1e -> 1d. The docstring already lists them in execution order ("TasksManager / pipeline-tag / default"); renaming pipeline-tag to 1d and the default to 1e would make the inline labels match.
| assert r.source == TaskSource.HF_TASK_DEFAULT | ||
|
|
||
|
|
||
| def test_resolve_task_pipeline_tag_skips_local_path() -> None: |
There was a problem hiding this comment.
This test mocks get_pipeline_tag, so it never actually exercises the local-path skip — that logic lives inside get_pipeline_tag, which is patched out here. The assertion only confirms the helper is called with "./local-model" and returns the mocked None, so the name/docstring overclaim what's covered: the real _is_local_path rejection and the except Exception fallthrough have no direct test. Suggest a focused unit test on get_pipeline_tag itself (local path -> None without hitting the network; model_info raising -> None).
| return False, {"type": "local", "path": model_name_or_path} | ||
|
|
||
|
|
||
| _PIPELINE_TAG_TIMEOUT = 10 # seconds |
3 participants
Summary
When architecture-based task detection (Stage 1c) fails because
config.architecturescontains a generic name like'Model'that doesn't map to any real transformers class, task resolution falls through to the last-resortfeature-extractiondefault. However, HuggingFace Hub knows the correct task viapipeline_tag(e.g.audio-classificationforaudeering/wav2vec2-large-robust-24-ft-age-gender).This PR adds a Stage 1e between Stage 1c (TasksManager architecture detection) and Stage 1d (last-resort default) that queries the Hub's
pipeline_tagas a fallback.Changes
src/winml/modelkit/loader/resolution.pyTaskSource.PIPELINE_TAG = "pipeline-tag"enum value for provenance trackingKNOWN_TASKSto imports from.taskresolve_task():opt_task is None(architecture detection failed) andmodel_idexistsmodel_idis not a local path via_is_local_pathfromhub_utilsHfApi().model_info(model_id).pipeline_tagKNOWN_TASKStry/except Exceptionfor graceful fallthrough on network errors or invalid model IDstests/unit/loader/test_detect_task.pyAdded 4 tests: