User input refactor and option bug fixes#620
Open
dale-wahl wants to merge 21 commits into
Open
Conversation
…ber first to improve user exception message, inform user when input not a number (instead of always correcting)
…pe" bug and potentially others)
…dd validate_default and log for devs
…ue being overwritten)
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.
Member
Author
|
I changed the target from |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What happened
Working on
validate_query, we rely onUserInput.parse_allto do many checks related to options (e.g. viacoerce_typeorminandmax). I found thatsillently_correct--which all callers set toFalse--actually still corrected certain values silently.QueryParametersExceptionwas not raisedreturninside afinallyand becameNoneThen, I found that own option declarations had drifted in a few ways:
coerce_typethat is a string instead of a type (nothing would actually coerceNone of these were announced and ended up as whatever our
defaultwas or the first option in case of choices in the browser (those could bite in presets andnextchains).And then the one that took longest to see, because it hides in plain sight. A
defaultis 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"). Butparse_allwas also filling defaults back in while parsing, in three separate places: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_correctnow defaults toFalsewhich seemed a better default to me.A
defaultis the form's starting value, and nothing elseI removed every place where parsing filled one back in. Strict parsing now never invents a value:
requires/board_specificunmet, unchecked toggle, file upload)silently_correct=Truestill 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.mandatoryA 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: Truesays it once — and composes withrequires, which is the nice part: an API key that only exists when no site-wide key is configured and is gated onmodel_host==openaiis now demanded exactly when the user can actually see it, with no Python condition re-typing therequires(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_defaultfeeds an option's own default back throughparse_value. Two module tests run it across every processor and datasource: one checks that each default is valid under thatoption'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_alltakes an optionallogwarns 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.
video-scene-detectorlist(options.values())[0], so when ffmpeg is unavailable the default became a label ("PySceneDetect ContentDetector") instead of a key. Alsocd_thresholddeclaredmax: 5against a 0–255 range, so every run was trimmed to 5 and over-detected scenes; andhidden_in_explorerwas a typo forhide_in_explorer, so its annotation was never actually hidden.image-wall"shortest"— sorting images by video length.render-rankflowsize_propertydefaulted to"change", copy-pasted from the colour option above it.column-filter,column-processor-filtermatch-stylestill defaulted to"exact", a stale name from before the rename tovalue-equals.histwords-vectspacemethoddefaulted to"tsne"where the options aret-SNE/PCA/TruncatedSVD.thresholdwas a text field with a string default and no type or range.video-frames,video-scene-framesframe_sizedefaulted to"medium", which was not one of the sizes.clarifai-apiannotation_thresholddeclaredcoerce_type: "float"— the string, not the type — so every value anyone typed was silently discarded and replaced by the default.random-filter,random-processor-filtercoercewas a typo forcoerce_typeand was silently ignored, so the sample size was never coerced at all (was coerced later inprocess). Fixing it made the processor'svalidate_queryredundant so it was removed.author-info-removeritertools.chainreturns, rather than a list.itertools.chainis a ONE TIME generator (I've seen it used wrong before in our code).collocations,similar-word2vec4chan,8chan,8kunsearch (datasources)scope_density,scope_lengthandvalid_idswere kept out of submissions by the form disabling them — a hand-rolledrequiresthat predates the mechanism. They now declarerequires: search_scope==dense-threads(andmatch-ids). This one is required by the change above: strict parsing would otherwise reject a form that never showed those fields. 4chan'svalidate_queryalsodel'd two of them unguarded, which was a latentKeyError— the silent default fill was the only thing hiding it.Notes
This targets
paramters_updateand now depends on it. It started out independent, but the "a missing option is an error" rule leans on that branch'srequiresgating: 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 onceparamters_updatemerges.Both branches touch
user_input.py, and both are groundwork for the actual PR I want: addingvalidate_queryto processors (and updating moreget_optionsfunctions to lean onparse_all's validation instead of hand-writing checks).🥱 That was a mouth full. But I feel really good about it.