Add missing match on onnx/model.onnx download
#472
Conversation
Narsil
left a comment
Narsil
left a comment
There was a problem hiding this comment.
LGTM.
I trust that it's fine if you checked it. Worse case we patch.
I'm surprised at the logic flow though.
Prevents the logging message "Model ONNX weights downloaded in ..." from showing whenever those are either not present or not downloaded
|
You're right indeed, there's something that I missed because the |
|
Should be fixed already, thanks for the catch up!
|
| let p = api.get("onnx/model.onnx").await?; | ||
| model_files.push(p.parent().unwrap().to_path_buf()) | ||
|
|
||
| match api.get("onnx/model.onnx").await { | ||
| Ok(p) => model_files.push(p.parent().unwrap().to_path_buf()), | ||
| Err(err) => tracing::warn!("Could not download `onnx/model.onnx`: {err}"), | ||
| }; |
There was a problem hiding this comment.
Isn't this virtually the same? How is the error handled in this fn?
| download_onnx(api_repo) | ||
| let model_files = download_onnx(api_repo) | ||
| .await | ||
| .map_err(|err| BackendError::WeightsNotFound(err.to_string())); |
There was a problem hiding this comment.
Didn't you only need to add a ? after the map_err?
There was a problem hiding this comment.
No because it still succeeds as far as I remember the download_onnx function was not producing errors but just warnings so empty ONNX files was not considered an error and so on it was printing that the files where there while those really weren't + this doesn't need to fail as it needs to fallback to the Safetensors backend if the ONNX files are not there.
Anyway I could've missed something, so please let me know!
3 participants
What does this PR do?
This PR adds a missing
matchstatement within thedownload_onnxfunction as it was missing for theonnx/model.onnxdownload, leading to an unexpected logging message claiming that the ONNX files were downloaded successfully, while those were not indeed (see attachment at huggingface/Google-Cloud-Containers#132 (comment)).This PR fixes a non-breaking issue, as even if the logging messages were misleading the behaviour for rolling back to
safetensorsif ONNX weights were not available is working as expected.Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
@OlivierDehaene