Fix helper fn for new processor config format #42085
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
molbap
left a comment
molbap
left a comment
There was a problem hiding this comment.
Thanks, looks fine to me! just a couple questions
|
Oke, now it works. We have to load both (legacy vs non-legacy) configs and then decide which one takes priority because it depends on their content |
molbap
left a comment
molbap
left a comment
There was a problem hiding this comment.
Re-approving after re-reading 😁 thanks for the iteration, not much except proposing to refacto the json opening a bit as many repeated bits
| except json.JSONDecodeError: | ||
| raise OSError(f"It looks like the config file at '{resolved_processor_file}' is not a valid JSON file.") |
There was a problem hiding this comment.
could be another exception type, why not capture and re-raise as part of the message?
There was a problem hiding this comment.
i see it was always raising a clear message for only for incorrectly serialized files, while other types of Errors are raised as is. So I decided to leave it as is, but in general we can just fallback to json's own errors for all cases. I think they are pretty straighforward to understand
There was a problem hiding this comment.
maybe in the future
There was a problem hiding this comment.
yeah agreed, json own error handling is robust
Co-authored-by: Pablo Montalvo <39954772+molbap@users.noreply.github.com>
|
[For maintainers] Suggested jobs to run (before merge) run-slow: auto |
3 participants
What does this PR do?
As per title, these helpers are used in vLLM and don't work if we save a processor after v5. Now all processor's subcomponents are saved in one single place in
processor_config.json, so we need to check it as well when trying to locate the config filefyi @hmellor