Skip to content

User input refactor and option bug fixes#620

Open
dale-wahl wants to merge 16 commits into
masterfrom
user_input_review
Open

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

Conversation

@dale-wahl

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).

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.

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.

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.

Notes

Targets master and is independent of the parameters_update branch — both touch user_input.py though and both will be part of my actual desired PR adding validate_query (and updating more get_option functions to utilize the parse_all validation).

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