Make a context manager for cast_bias_weight and use it.#14750
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChangesA new Sequence Diagram(s)Not applicable — the change is a mechanical refactor of internal weight-casting call patterns without introducing new external interaction flows. Related PRs: None identified from the provided context. Suggested labels: refactor, comfy/ops Suggested reviewers: Reviewers familiar with Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@rattus128 can you take a look? |
| device = bias_a.device | ||
| os.wait_stream(comfy.model_management.current_stream(device)) | ||
|
|
||
| class CastBiasWeightContext: |
There was a problem hiding this comment.
This could also be done with a context manager decorator, though it would require adding an import for contextlib. Something like:
@contextlib.contextmanager
def cast_bias_weight_context(*args, **kwargs):
slf = args[0] if len(args) else None
state = (None, None) if slf is None else cast_bias_weight(*args, **kwargs)
del args, kwargs
if len(state) < 3 or state[2] is None:
state = [state]
yield state.pop()
return
try:
yield state[:2]
finally:
uncast_bias_weight(slf, *state)Not sure if that's better. Using the list and popping is so the context manager can be sure it's not holding any references in the case it doesn't need to uncast.
This pull adds a context manager for
cast_bias_weightto ensure thatuncast_bias_weightis always called when necessary, even if an exception occurs. It possibly also fixes a couple potential issues:fp8_linearaccesses the weight after uncasting it.Linearmodule in mixed precision ops converts the dtype of the weight, but it callsuncast_bias_weighton the converted tensor rather than the original. I'm not sure what happens when you try to uncast something that wasn't cast but I'm guessing it's a potential problem.The changes are mostly straightforward but there are a lot of code paths and I haven't tested all of them. I have checked and triple checked the patch, ran it by a few LLMs and as far as I know there aren't any current problems.
I tried to maintain the existing code style as much as possible.