requireFullTime -> timeMinimumInMinutes by rasmi · Pull Request #1059 · PAIR-code/deliberate-lab

rasmi · 2026-03-17T19:30:18Z

Rename requireFullTime (boolean) to timeMinimumInMinutes (number | null), allowing experimenters to set a specific minimum time independent of the max time limit.
Add enforcement of timeLimitInMinutes in private chat (previously only enforced in group chat).
Add server-side validation of minimum time requirement in chat.utils.ts and chat.endpoints.ts.
Add migration script to convert existing requireFullTime: true stages to equivalent timeMinimumInMinutes values.
Force integer values for time inputs in editors.
Add server-side validation between parameters

(in lieu of #1058).

mkbehr · 2026-03-20T20:11:42Z

frontend/src/components/stages/private_chat_editor.ts

      if (!this.stage) return;
+      const val = Number((e.target as HTMLInputElement).value);
+      const max = this.stage.timeLimitInMinutes;
+      const clamped = max !== null ? Math.min(val, max) : val;


If I'm reading this right, the form and value representation will both accept Numbers (including floats?), but the actual time comparison only compares based on an integer number of minutes. If someone enters in a time limit of 2.5 minutes, will we happily claim to have accepted it, and then actually do a limit of 3 minutes in practice?

Normally I'd suggest storing this as seconds internally, partially to make that clearer and partially because it's more common to store times that way IME (so less surprising). But we do already store the time limit in minutes.

Added a TODO for the seconds switch (and enforced setting ints in the UI in the meantime)

mkbehr · 2026-03-20T20:14:59Z

frontend/src/components/stages/private_chat_editor.ts

-                  Elapsed time from first message to conversation close (in
-                  minutes)
-                </label>
+                <label for="timeLimit"> Maximum time (in minutes) </label>


This wording makes it less clear when we start measuring time. If I were reading this in a vacuum, I'd assume it meant maximum time since the stage became available.

Maybe something like "Maximum time in minutes (starting at first message)"?

mkbehr · 2026-03-20T20:20:19Z

frontend/src/components/stages/private_chat_participant_view.ts

+      elapsedMinutes >= this.stage.timeLimitInMinutes;
+
+    // Check if conversation has ended
+    const isConversationOver = maxTurnsReached || maxTimeReached;


The PR description only mentions changing the minimum time config, but this looks like it's changing something around how the end of the conversation is determined. Does it, and if so can you mention in the description?

mkbehr · 2026-03-20T20:22:19Z

frontend/src/components/stages/private_chat_participant_view.ts

+      (discussionStartTimestamp !== null &&
+        elapsedMinutes >= this.stage.timeMinimumInMinutes);
+
+    const isNextDisabled = !minTurnsMet || !minTimeMet;


Maybe a preexisting issue, but what happens if max time elapses before min turns? Will chat get stuck with no way to advance or send new messages?

@cjqian and I discussed and we decided to enforce min requirements over max requirements (i.e. must hit min turns regardless of max time, and same for min_time / max_turns). Noted in the editor UI as well.

mkbehr · 2026-03-20T20:23:24Z

frontend/src/components/stages/private_chat_participant_view.ts

+    return html`
+      <div class="description">
+        You must stay on this chat for at least
+        ${this.stage?.timeMinimumInMinutes} minutes.


Would it be easy to give an indication of how much time is left here?

mkbehr · 2026-03-20T20:28:07Z

utils/src/stages/chat_stage.validation.ts

        timeLimitInMinutes: Type.Union([Type.Number(), Type.Null()]),
-        requireFullTime: Type.Union([Type.Boolean(), Type.Null()]),
+        timeMinimumInMinutes: Type.Optional(
+          Type.Union([Type.Number(), Type.Null()]),


I see we have client-side validation that minimum is between 0 and maximum, but nothing server-side. Can we add that? Ideally we'd have both validation checking that here, and something to make sure the API rejects attempts to set it.

Added server-side stage validation for cross-parameter checks!

mkbehr · 2026-03-20T20:30:08Z

scripts/deliberate_lab/types.py

    progress: StageProgressConfig
    timeLimitInMinutes: float | None = None
-    requireFullTime: bool | None = None
+    timeMinimumInMinutes: float | None = None


Do we have any API client tests that we can use to exercise this?

What do you mean? GH Actions fails if the typebox schemas and Python types are ever out of sync (the Python file is generated from the Typebox schemas using npm run update-schemas).

Also -- API requests are validated through Typebox schema (and added stage validation at your suggestion!)

What I mean is, do we have any tests that send an API request using the parameter, and confirm that it does what we expect? I'm imagining a scenario where we accidentaly set up the API handling to ignore the parameter and just set it to default - everything would pass type validation but the feature wouldn't work. Would we catch that in tests?

(If the answer is "no, and a proper test framework for that would be too much work for this feature," that's fine, I don't want to hold this up until the infra is perfect or anything.)

mkbehr · 2026-03-20T20:30:30Z

utils/src/stages/chat_stage.ts

    discussions: config.discussions ?? [],
    timeLimitInMinutes: config.timeLimitInMinutes ?? null,
-    requireFullTime: config.requireFullTime ?? false,
+    timeMinimumInMinutes: config.timeMinimumInMinutes ?? null,


This shouldn't need a separate null option; we can use 0 for no minimum, in the internal representation and probably the API. That way we don't have to wonder whether there's a difference between a 0 and a null.

We allow a lot of options to remain unset across the codebase, I think it's nice to not have to specify certain optional things and have the codebase handle them appropriately. WDYT?

From a UI perspective, definitely agree that we shouldn't have to specify optional things. From a backend perspective, my claim is that the appropriate way to handle it is to default the minimum to 0, instead of a separate null. Otherwise we have separate representations for "user didn't specify a minimum" and "user specified no minimum". That makes the code harder to reason about, and makes it possible for us to accidentally make them act differently.

What if we make it so that it has to be non-zero if set? I think "user didn't specify a minimum" is a useful state to have, especially for optional features.

…ds).

rasmi added 3 commits March 17, 2026 15:28

requireFullTime -> timeMinimumInMinutes

8d95a0c

Tidy up.

accc857

Tidy up.

b9c7353

rasmi requested a review from cjqian March 17, 2026 19:42

Tidy up

eff0c87

rasmi mentioned this pull request Mar 18, 2026

Added minimum-time feature to Private / Group discussions, so minimum AND maximum durations can be enforced. #1058

Open

cjqian requested a review from mkbehr March 20, 2026 15:13

mkbehr requested changes Mar 20, 2026

View reviewed changes

rasmi added 5 commits March 27, 2026 10:46

Show dynamic min time remaining

11d17d1

Force integer minutes in UI editor (add todo about migrating to secon…

51d32b8

…ds).

Enforce minimums (messages, minutes) over maximums.

867f9e6

Add cross-parameter stage validation.

6458840

Clean up lint warnings / unused variables

382cfbe

rasmi force-pushed the min-time-clean branch 2 times, most recently from edb1b82 to 382cfbe Compare March 27, 2026 17:29

Conversation

rasmi commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rasmi Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rasmi commented Mar 17, 2026 •

edited

Loading

rasmi Mar 27, 2026 •

edited

Loading