Open-Sora icon indicating copy to clipboard operation
Open-Sora copied to clipboard

fix a bug that caused to pop all `v2v` conditions during training

Open hadipash opened this issue 8 months ago • 5 comments

This PR fixes condition config mutation in prepare_visual_condition during training. Specifically, because dictionaries are passed by reference, even a single short video would be enough to completely remove v2v_head, v2v_tail, v2v_head_easy, and v2v_tail_easy from the condition config.

hadipash avatar Apr 23 '25 02:04 hadipash

@hadipash Could you please specify any exact bugs this usage causes? Poping a config might be useful if we don't want it to be consumed twice (but I'm not sure if it's the case here)

botbw avatar May 09 '25 05:05 botbw

@botbw It doesn't cause bugs with the current configs (as they use i2v configuration only), but if one were to add v2v configurations, a single short video would cause those configurations to pop permanently in the following lines: https://github.com/hpcaitech/Open-Sora/blob/d0cd5ac50da79e9a9d2285a952d4dcd806e6c8fc/opensora/utils/train.py#L214-L221 The popping above is supposed to be local and applied to a specific batch, rather than modifying the global configuration.

hadipash avatar May 09 '25 06:05 hadipash

@botbw It doesn't cause bugs with the current configs (as they use i2v configuration only), but if one were to add v2v configurations, a single short video would cause those configurations to pop permanently in the following lines:

https://github.com/hpcaitech/Open-Sora/blob/d0cd5ac50da79e9a9d2285a952d4dcd806e6c8fc/opensora/utils/train.py#L214-L221

The popping above is supposed to be local and applied to a specific batch, rather than modifying the global configuration.

@hadipash I see, but I think the caller should be aware of potential modifications when executing this (although surely it should be documented), there is no optimal design (IMO) as python doesn't have any ownership concept.

botbw avatar May 09 '25 08:05 botbw

@botbw Right. Moved the dictionary copying under prepare_visual_condition to avoid confusion, as it's supposed to be internals of the API.

hadipash avatar May 12 '25 06:05 hadipash

@zhengzangw A tiny change for the robustness of open-sourced code.

botbw avatar May 12 '25 07:05 botbw