Skip to content

fix: include downloaded path in persist_downloaded_model_config warning - #5208

Open
kapil971390 wants to merge 1 commit into
opendatalab:masterfrom
kapil971390:fix/persist-model-config-actionable-warning
Open

fix: include downloaded path in persist_downloaded_model_config warning#5208
kapil971390 wants to merge 1 commit into
opendatalab:masterfrom
kapil971390:fix/persist-model-config-actionable-warning

Conversation

@kapil971390

@kapil971390 kapil971390 commented Jul 1, 2026

Copy link
Copy Markdown

Motivation

While reviewing recent commits, I noticed that when persist_downloaded_model_config fails (e.g. disk full, permission error, network issue fetching the config template), the existing warning only logs the exception message:

Failed to persist downloaded pipeline model config to /home/user/mineru.json: [Errno 28] No space left on device

The models themselves were downloaded successfully, but the path is never written to models-dir in the config. On the next run, get_existing_configured_model_root finds no config entry and falls through to _snapshot_download_cached, triggering a full re-download — silently, with no indication of why.

Modification

Extended the warning message in persist_downloaded_model_config to include:

  1. The path where models were successfully downloaded (model_root)
  2. An explanation that MinerU will re-download on the next run without this entry
  3. The exact JSON snippet the user can paste into their config file to avoid re-downloading

Example of the new warning:

Failed to persist downloaded pipeline model config to /home/user/mineru.json: [Errno 28] No space left on device.
Models were downloaded successfully to '/home/user/.cache/huggingface/hub/...' but the path will not be
cached, so MinerU may re-download on the next run. To avoid this, manually add the following to /home/user/mineru.json:
  "models-dir": {"pipeline": "/home/user/.cache/huggingface/hub/..."}

No logic changes — the exception is still caught and the download result is still returned normally.

BC-breaking

No. This is a warning message change only. All existing behaviour is preserved.

Checklist

Before PR:

  • Bug fixes are fully covered by unit tests, the case that causes the bug should be added in the unit tests.
  • The modification is covered by complete unit tests.
  • Pre-commit or other linting tools are used to fix the potential lint issues.
  • The documentation has been modified accordingly, like docstring or example tutorials.

After PR:

  • CLA has been signed and all committers have signed the CLA in this PR.

When download_and_modify_json fails (e.g. disk full, permission error),
the warning now tells the user exactly where the models landed and shows
the JSON snippet they need to add manually to avoid a redundant re-download
on the next run.

Add five unit tests covering success, failure (OSError/PermissionError),
no-raise guarantee, and both pipeline/vlm repo_mode variants.
@dosubot dosubot Bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jul 1, 2026
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor


Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

@kapil971390

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants