Comfy-Org / Comfy-Org/ComfyUI

PDD support unnecessarily breaks the MiniMax H3 `FinalLayer.forward` calling contract

Open
#15,960 1 comment 5 reactions 0 assignees View on GitHub
Potential Bug
Dominant language
Python
Stars
133k
Forks
15.7k
Avg merge
1d 7h
Merged PRs (30d)
158

Description

### Custom Node Testing

- [x] I have tried disabling custom nodes and the issue persists (see [how to disable custom nodes](https://docs.comfy.org/troubleshooting/custom-node-issues#step-1%3A-test-with-all-custom-nodes-disabled) if you need help)

### Expected Behavior

### PDD support unnecessarily breaks the MiniMax H3 `FinalLayer.forward` calling contract

PR #15908 changed MiniMax H3's `FinalLayer.forward` from:

```python
forward(x, t_emb, video_seg, audio_seg)
```

to:

```python
forward(x, t_emb, video_seg, audio_seg, sigma, sample_sigmas, shifts)
```

and the normal H3 forward path now passes those three additional positional arguments unconditionally, including when no PDD LoRA is being used.

This breaks existing custom-node patches/wrappers around `FinalLayer.forward`. For example, H3-Optimizations currently replaces this method to chunk the final projection for lower peak VRAM, and after #15908 that results in:

```text
make_forward..forward() takes 4 positional arguments but 7 were given
```

The PDD feature does need access to the current sigma/schedule when selecting and blending its output heads, but it doesn't appear necessary for that to become a new mandatory calling contract for every H3 `FinalLayer`.

There are several ways this could remain backward compatible, for example:

* optional keyword arguments with defaults;
* passing the PDD context through existing model/transformer state;
* keeping the PDD-specific head selection outside the ordinary `FinalLayer.forward` contract;
* or another implementation-specific helper.

There is already a third-party PDD implementation which obtains the sampling state outside `FinalLayer` and delegates back to the native `FinalLayer.forward`, so PDD itself does not inherently require changing this method's required arguments.

This also seems relevant to the repository's own `AGENTS.md` Interface Contracts guidance:

> “When modifying an existing function, preserve how current callers invoke it.”

Could the PDD implementation be adjusted so the previous four-argument calling convention remains valid?

I'm happy to update H3-Optimizations for the new PDD behavior either way, but avoiding an unnecessary breaking change here would also avoid forcing every custom node that wraps this part of H3 to maintain separate pre/post-#15908 interfaces.

### Actual Behavior

**Exception Message:** make_forward..forward() takes 4 positional arguments but 7 were given

### Steps to Reproduce

.

### Debug Logs

```powershell
.
```

### Other

_No response_

Contributor guide

Open the contributing guide

Research direction

Start with PR #15908 and inspect MiniMax H3's FinalLayer.forward plus the normal H3 forward path. Read the Interface Contracts guidance in AGENTS.md, then trace how PDD obtains sigma and schedule state. Done means preserving the existing four-argument calling convention while keeping PDD behavior working.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
machine-learning
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.