Skip to content

User input refactor and option bug fixes#620

Open
dale-wahl wants to merge 21 commits into
paramters_updatefrom
user_input_review
Open

User input refactor and option bug fixes#620
dale-wahl wants to merge 21 commits into
paramters_updatefrom
user_input_review

Conversation

@dale-wahl

@dale-wahl dale-wahl commented Jul 16, 2026

Copy link
Copy Markdown
Member

What happened

Working on validate_query, we rely on UserInput.parse_all to do many checks related to options (e.g. via coerce_type or min and max). I found that sillently_correct--which all callers set to False--actually still corrected certain values silently. QueryParametersException was not raised

  • a value that could not be read as a number fell back to the default
  • an out-of-range number was trimmed to fit
  • unparseable date was swallowed by a return inside a finally and became None

Then, I found that own option declarations had drifted in a few ways:

  • defaults that are not one of the option's own choices
  • a coerce_type that is a string instead of a type (nothing would actually coerce
  • misspelled setting keys were silently ignored

None of these were announced and ended up as whatever our default was or the first option in case of choices in the browser (those could bite in presets and next chains).

And then the one that took longest to see, because it hides in plain sight. A default is the form's starting value — the form pre-fills every one of them before the user ever looks at it (value="{{ default }}", selected="selected", checked="checked"). But parse_all was also filling defaults back in while parsing, in three separate places:

  • an option missing from the submission silently became its default
  • a cleared text field got the default the user had just deleted put back
  • a multi-select with everything unticked got its default selection back

So a value the user never sent could sail straight through every check that follows. It also made option_given() a lie: the stored parameters are documented as holding only what was actually submitted, and this was quietly filling them with things that weren't.

What I did

The parser reports instead of silently correcting. Strict mode now actually raises where it used to quietly fix things, and the messages match the problem. Multi-value options accept a real list as well as a comma-separated string (previously a genuine multi-value submission crashed with a 500). Because every real caller already asked for strict parsing, silently_correct now defaults to False which seemed a better default to me.

A default is the form's starting value, and nothing else

I removed every place where parsing filled one back in. Strict parsing now never invents a value:

The user... What happens
left the pre-filled default alone that default — because they submitted it
cleared a text field stays empty; "nothing" is an answer
cleared a number field error — there is no honest empty number
unticked every box an empty selection, not the default back
never saw the option (requires/board_specific unmet, unchecked toggle, file upload) left out, as before
left a field out of the submission entirely error naming the option

silently_correct=True still fills defaults in, so tolerant internal callers are unaffected.

One wrinkle worth knowing: a browser leaves a <select> with nothing selected out of the submission altogether, which is indistinguishable from the field never having been shown. So the form now submits an empty marker alongside multi-selects and parsing discards it — the same trick the daterange fields already used. Without it, unticking everything and never seeing the field are the same event.

mandatory

A new option flag for "you must fill this in". About ten processors hand-write that check in validate_query, each with its own wording. mandatory: True says it once — and composes with requires, which is the nice part: an API key that only exists when no site-wide key is configured and is gated on model_host==openai is now demanded exactly when the user can actually see it, with no Python condition re-typing the requires (that duplication is what drifts).

Nothing uses it in this PR — applying it across the processors is the follow-up.

Added tests to avoid drift in the future

Option declarations are checked by the test suite. UserInput.validate_default feeds an option's own default back through parse_value. Two module tests run it across every processor and datasource: one checks that each default is valid under that
option's own rules, the other that every setting key is one the framework actually reads.

These tests identified quite a few bugs.

Every row of the table above is pinned too, strict and tolerant — which earned its keep immediately: it caught me breaking the tolerant path within one commit of writing it.

Added a live warning as well

We get the warning and users get a message helping them fix it. parse_all takes an optional log warns when an option's default is invalid — that is our bug, not the user's. I deliberately did not correct it (for the same reason as above). But the user gets a message on how to correct any issue e.g. "Pixel intensity delta threshold: Provide a value of 5 or lower" (even though our default was 27.0 and the max was bad). We should catch these from the tests now, but with the whole "options can depend on DataSets" this seemed a good fallback if we do manage to get a bug by the tests.

Processors fixed

I tried to ensure that these preserve what the interface already did or ought to have been doing.

Processor What was wrong
video-scene-detector Rebuilt its default with list(options.values())[0], so when ffmpeg is unavailable the default became a label ("PySceneDetect ContentDetector") instead of a key. Also cd_threshold declared max: 5 against a 0–255 range, so every run was trimmed to 5 and over-detected scenes; and hidden_in_explorer was a typo for hide_in_explorer, so its annotation was never actually hidden.
image-wall Replaced the inherited sort options with image-specific ones but kept video-wall's default "shortest" — sorting images by video length.
render-rankflow size_property defaulted to "change", copy-pasted from the colour option above it.
column-filter, column-processor-filter match-style still defaulted to "exact", a stale name from before the rename to value-equals.
histwords-vectspace method defaulted to "tsne" where the options are t-SNE / PCA / TruncatedSVD. threshold was a text field with a string default and no type or range.
video-frames, video-scene-frames frame_size defaulted to "medium", which was not one of the sizes.
clarifai-api annotation_threshold declared coerce_type: "float" — the string, not the type — so every value anyone typed was silently discarded and replaced by the default.
random-filter, random-processor-filter coerce was a typo for coerce_type and was silently ignored, so the sample size was never coerced at all (was coerced later in process). Fixing it made the processor's validate_query redundant so it was removed.
author-info-remover For CSV parents the default was the generator itertools.chain returns, rather than a list. itertools.chain is a ONE TIME generator (I've seen it used wrong before in our code).
collocations, similar-word2vec Numeric defaults where the option keys are strings.
4chan, 8chan, 8kun search (datasources) scope_density, scope_length and valid_ids were kept out of submissions by the form disabling them — a hand-rolled requires that predates the mechanism. They now declare requires: search_scope==dense-threads (and match-ids). This one is required by the change above: strict parsing would otherwise reject a form that never showed those fields. 4chan's validate_query also del'd two of them unguarded, which was a latent KeyError — the silent default fill was the only thing hiding it.

Notes

This targets paramters_update and now depends on it. It started out independent, but the "a missing option is an error" rule leans on that branch's requires gating: options whose condition is unmet have to be dropped before the missing-option check runs, or e.g. the 4chan scope fields get rejected on a form that never showed them. GitHub should retarget this to master once paramters_update merges.

Both branches touch user_input.py, and both are groundwork for the actual PR I want: adding validate_query to processors (and updating more get_options functions to lean on parse_all's validation instead of hand-writing checks).

🥱 That was a mouth full. But I feel really good about it.

dale-wahl added 21 commits July 16, 2026 12:20
…ber first to improve user exception message, inform user when input not a number (instead of always correcting)
A default is the form's starting value. It was also being filled back in
while parsing, which meant a value the user never sent could pass the
checks that follow. Three ways in: an option missing from the submission, a
cleared text field, and an unticked multi-select all quietly became the
default again.

Parsing strictly now says so instead:
- a missing option raises, naming it, since our own form submits every
  field it shows. Options a form legitimately leaves out stay exempt:
  unchecked toggles, file uploads (sent outside the form values), and
  fields gated by an unmet requires or board_specific condition.
- a cleared text field stays empty; a cleared numeric field raises, as
  there is no honest empty number.
- unticking every option of a multi-select means an empty selection.

A browser leaves a select with nothing selected out of the submission
altogether, which is indistinguishable from the field never having been
shown, so the form now submits an empty marker alongside it; parsing
discards the marker.

The chan scope fields were kept out of submissions by the form disabling
them, which is a hand-rolled "requires"; they now declare it instead.

Parsing tolerantly (silently_correct) still fills the default in, so
internal callers that ask to be corrected quietly are unaffected.
@dale-wahl
dale-wahl changed the base branch from master to paramters_update July 17, 2026 14:01
@dale-wahl

dale-wahl commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

I changed the target from master to paramters_update. In the parameters update, I wanted to check requirements in multiple places and thus edited parse_all. This fixes the merge conflicts and actually allows some improvements. I.e. the mandatory option flag which means we can simplify even more validate_query methods down or remove them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant