Fix begin and end validation used in ControlNets.

[JPPhoto]

Summary

Setting a ControlNet beginning and ending to the same value results in a pydantic error.

Related Issues / Discussions

https://discord.com/channels/1020123559063990373/1149506274971631688/1333866963256086589

QA Instructions

Validation now works as expected when the beginning and ending are the same value.

Checklist

• The PR has a short but descriptive title, suitable for a changelog
• Tests added / updated (if applicable)
• Documentation added / updated (if applicable)
• Updated What's New copy (if doing a release after this PR)

[psychedelicious]

Does anything happen when they are the same value (e.g. a single step w/ the control)?

[JPPhoto]

Does anything happen when they are the same value (e.g. a single step w/ the control)?

No, if they are the same value then it appears to do nothing. I think that's better behavior than an exception as it lets you leave a ControlNet in a workflow but set values (algorithmically) to 0 to disable it.

[hipsterusername]

This seems like a workaround for a mute function.

[JPPhoto]

This seems like a workaround for a mute function.

Sorry, I misunderstood what you were asking. If you set start and end to the same value in main, you get an exception rather than the expected no-op.

[psychedelicious]

Sorry, I misunderstood what you were asking. If you set start and end to the same value in main, you get an exception rather than the expected no-op.

I think you thought @hipsterusername 's reply was from me - you did answer my question initially. But I agree that this change seems like a workaround for a mute feature more than a fix.

If we make this a no-op, why not make begin > end a no-op also? Have to draw the line somewhere and it might as well be in the technically correct spot, which means begin == end is invalid.

[JPPhoto]

Sorry, I misunderstood what you were asking. If you set start and end to the same value in main, you get an exception rather than the expected no-op.

I think you thought @hipsterusername 's reply was from me - you did answer my question initially. But I agree that this change seems like a workaround for a mute feature more than a fix.

If we make this a no-op, why not make begin > end a no-op also? Have to draw the line somewhere and it might as well be in the technically correct spot, which means begin == end is invalid.

So this means that it's impossible to make a workflow where you set start and end points for the control independently of generation, i.e. I want the control to be for overall steps 0 to 0.2 and generation to be split into two separate prompts from 0 to 0.3 and 0.3 to 1. The math works fine if I set the control to go for longer than 0.3 but not if I want to cut it off early; I have to modify my workflow every time I want to change cutoffs.

See the following for how I implement it after the patch is in place (duration being the length of the control and split being when to split generation steps):

[psychedelicious]

Though I don't fully understand what you are doing, I do understand that the current behaviour makes it difficult.

I think this may be an XY problem, where changing the constraints is a way to address a narrow use-case (i.e. lets you do what you want to do without changing any other nodes), but doesn't address a broader need. I wonder if there's a way to handle the math stuff such that we don't need to compromise the technical accuracy of the fields.

[hipsterusername]

Definitely a way using float math nodes to do a max or min

[JPPhoto]

You can't use the same value for control begin steps percent and end steps percent, or the exception gets thrown and the workflow fails.

Even the most basic case of having the node as part of a workflow and (you hope) disabled by setting both begin and end to 0 causes an exception to be thrown. To me this seems like a problem in the code logic, especially given the error that I didn't change that says only less than is problematic:

raise ValueError("Begin step percent must be less than or equal to end step percent")

In other words, I made the code match the exception that's thrown rather than changing the exception to match the code.

I don't think there's a way to work around the exception since all node paths get evaluated and there's no conditional execution logic. I thought the change I made would be harmless and the easiest and least controversial path - guess not!

[psychedelicious]

Thanks for the explanation, I see how it's difficult to do what you want without this change (or a mute feature). I'd prefer to not change the constraints but I also don't think there is any harm in changing them. I can't imagine it would actually break anything.

I'll just ask for @RyanJDick's opinion on the change.

PS: A future field type might be a number tuple that renders as a range slider. Like with two thumbs. Would be pretty useful.

[RyanJDick]

I think I understand the motivation. My main concern would be that people set start == end and expect the controlnet to be disabled, but it still might run for 2 steps.

In a few places, we have logic that looks something like the following. This will always run the controlnet for at least 1 step, and will often run it for 2 steps (due to floor/ceil).

            first_control_step = math.floor(control_datum.begin_step_percent * total_step_count)
            last_control_step = math.ceil(control_datum.end_step_percent * total_step_count)
            if step_index >= first_control_step and step_index <= last_control_step:
                ...

I can understand why the original author chose to make the range inclusive on both ends - it makes it more natural when you want to run the controlnet for all steps. But, it makes things awkward when you are trying to do more fine-grained intermediate start/stop control. To do this 'right', I think we'd probably want to handle it like we do for FLUX denoising start/end with epsilon-close equality checks (

InvokeAI/invokeai/backend/flux/sampling_utils.py

Lines 62 to 97 in 7daff46

	def _find_last_index_ge_val(timesteps: list[float], val: float, eps: float = 1e-6) -> int:
	"""Find the last index in timesteps that is >= val.
	We use epsilon-close equality to avoid potential floating point errors.
	"""
	idx = len(list(filter(lambda t: t >= (val - eps), timesteps))) - 1
	assert idx >= 0
	return idx
	def clip_timestep_schedule(timesteps: list[float], denoising_start: float, denoising_end: float) -> list[float]:
	"""Clip the timestep schedule to the denoising range.
	Args:
	timesteps (list[float]): The original timestep schedule: [1.0, ..., 0.0].
	denoising_start (float): A value in [0, 1] specifying the start of the denoising process. E.g. a value of 0.2
	would mean that the denoising process start at the last timestep in the schedule >= 0.8.
	denoising_end (float): A value in [0, 1] specifying the end of the denoising process. E.g. a value of 0.8 would
	mean that the denoising process end at the last timestep in the schedule >= 0.2.
	Returns:
	list[float]: The clipped timestep schedule.
	"""
	assert 0.0 <= denoising_start <= 1.0
	assert 0.0 <= denoising_end <= 1.0
	assert denoising_start <= denoising_end
	t_start_val = 1.0 - denoising_start
	t_end_val = 1.0 - denoising_end
	t_start_idx = _find_last_index_ge_val(timesteps, t_start_val)
	t_end_idx = _find_last_index_ge_val(timesteps, t_end_val)
	clipped_timesteps = timesteps[t_start_idx : t_end_idx + 1]
	return clipped_timesteps

).

I see a few paths forward. Want to hear other opinions based on this additional context:

1. Make the proposed change in the PR. It'll make the described workflow possible, but probably won't behave in the way that users expect (i.e. will run 1-2 steps when users expect it to be disabled)
2. Refactor all start/end logic to use the 'correct' implementation. This impacts a handful of features (controlnet, IP-Adapter, T2I) across various base models.
3. Update the error text to match the existing logic. Do it 'right' for future base models.