Modularize qwen-format vision processors - #47573
Conversation
|
|
||
| do_resize = True | ||
| resample = PILImageResampling.BICUBIC | ||
| class Cosmos3EdgeImageProcessor(Glm4vImageProcessor): |
There was a problem hiding this comment.
basically, it is same just with different default values, why not copy in that case?
| def _standardize_kwargs(self, **kwargs) -> dict: | ||
| """ | ||
| Update kwargs that need further processing before being validated | ||
| Can be overridden by subclasses to customize the processing of kwargs. | ||
| """ | ||
| kwargs = super()._standardize_kwargs(**kwargs) | ||
| size = kwargs.get("size", self.size) | ||
| if not size.shortest_edge or not size.longest_edge: | ||
| raise ValueError("size must contain 'shortest_edge' and 'longest_edge' keys.") | ||
| return kwargs | ||
|
|
There was a problem hiding this comment.
i don't think we need to check it in here, it belong to self.validate but I decided to move inside self.resize, that makes it easier to re-use in modular
| # Add/overwrite all needed function nodes and dependencies | ||
| # Add/overwrite all needed function nodes and dependencies, but save fn names from modular file before | ||
| self.modular_defined_global_objects.update(functions.keys()) | ||
| self.functions.update(functions) |
There was a problem hiding this comment.
Cases when we want to copy video processing from e.g. Qwen2VL but the model has no image processing. Video processor has an import line from .image_processing_model import smart_resize and currently it's impossible to override it with custom smart_resize
There was a problem hiding this comment.
Tbh, we should never have cross imports in the first place so I'd rather we copy the smart resize functions to vid processor for example
This looks like a workaround we actually don't want to have as it hides the issue with cross imports
|
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. |
There was a problem hiding this comment.
Super nice to get more modular usage out of these 馃 I have a few comments, mostly the modular change is making me hesitate to approve for now
Also we are breaking! We do change kwargs orders etc for the price of uniform implementation so we do need a 馃毃
| patch_size: int, | ||
| temporal_patch_size: int, | ||
| merge_size: int, | ||
| disable_grouping: bool | None, |
There was a problem hiding this comment.
haha, kinda though can we say it's breaking for a private method? 馃槃 They will have defaults propagated iirc when we get to this point
There was a problem hiding this comment.
fair point :D yea its not too breaking
| # Add/overwrite all needed function nodes and dependencies | ||
| # Add/overwrite all needed function nodes and dependencies, but save fn names from modular file before | ||
| self.modular_defined_global_objects.update(functions.keys()) | ||
| self.functions.update(functions) |
There was a problem hiding this comment.
Tbh, we should never have cross imports in the first place so I'd rather we copy the smart resize functions to vid processor for example
This looks like a workaround we actually don't want to have as it hides the issue with cross imports
|
not ready yet, will fix and run slow CI |
|
run-slow: cosmos3_edge, ernie4_5_vl_moe, glm46v, glm4v, glm_image, glmga, hunyuan_vl, kimi_k25, minimax_m3_vl, paddleocr_vl, qwen2_vl, qwen3_vl, video_llama_3 |
|
This comment contains models: ["models/cosmos3_edge", "models/ernie4_5_vl_moe", "models/glm46v", "models/glm4v", "models/glm_image", "models/glmga", "models/hunyuan_vl", "models/kimi_k25", "models/minimax_m3_vl", "models/paddleocr_vl", "models/qwen2_vl", "models/qwen3_vl", "models/video_llama_3"] |
CI ResultsCommit Info
The test failure analysis could not be completed. Please check the workflow run for details. |
|
looks fine, same tests failing as main |
|
@bot /repo |
|
Repo. Consistency bot fixed some files and pushed the changes. |
vasqu
left a comment
There was a problem hiding this comment.
LGTM now thanks 馃 just a few nits but nothing major, one idea for potential mlinter
|
[For maintainers] Suggested jobs to run (before merge) run-slow: cosmos3_edge, ernie4_5_vl_moe, glm46v, glm4v, glm_image, glmga, hunyuan_vl, janus, kimi_k25, minimax_m3_vl, paddleocr_vl, qwen2_vl, qwen3_vl, tvp, video_llama_3, vitmatte |
CI recapDashboard: View test results in Grafana |
What does this PR do?
For easier modular inheritance when we only want to change the resize-fn or the patch layout + easier to read the code