User input refactor and option bug fixes#620
Open
dale-wahl wants to merge 16 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)
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).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.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.
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-word2vecNotes
Targets master and is independent of the
parameters_update branch— both touchuser_input.pythough and both will be part of my actual desired PR addingvalidate_query(and updating moreget_optionfunctions to utilize theparse_allvalidation).