Skip to content

Update and standardize processor parameters to support adding validate_query to all processors#619

Open
dale-wahl wants to merge 23 commits into
masterfrom
paramters_update
Open

Update and standardize processor parameters to support adding validate_query to all processors#619
dale-wahl wants to merge 23 commits into
masterfrom
paramters_update

Conversation

@dale-wahl

Copy link
Copy Markdown
Member

This PR standardizes the validate_query method so it can be used across all processors instead of just datasources. It is still required for datasources — now enforced at the queue endpoint (a datasource that only inherits the default pass-through is refused with a clear error, instead of the old raw NotImplementedError) — but NOT required for processors. BasicProcessor now provides a pass-through validate_query by default, so a processor opts in only when it has something to check or confirm. (We could require it on processors too, but all extensions would need updating, so in the off chance others have created their own, we'd need to deprecate and properly inform them somehow.)

In order to really do this properly, I reviewed self.parameters, the query argument to get_items (which only exists for Search classes aka datasources), and get_parameters which did not always return the same exact thing. #603

Core

  • BasicProcessor: added a default validate_query and a get_validated_query wrapper that both queue endpoints now call instead of validate_query directly. This makes it the single place any check/rule applies no matter where a dataset is created from. Right now it (a) strips framework-injected transient keys (TRANSIENT_QUERY_KEYS, i.e. frontend-confirm) from the stored result so they don't clutter the record, and (b) warns when a validate_query stores None for a key that wasn't in the submission (a common mistake in validate_query methods via "never_submitted": query.get("never_submitted") when rebuilding a query).
  • BasicProcessor: added option_given to check that a user actually submitted a specific option vs it being filled with the default via get_options. Backed by a snapshot of the stored parameters taken at run start.
  • UserInput.parse_all: now respects requirements_met (an abstraction of the existing _requirement_met) so that only user seen/chosen options are stored in the database. This makes the UI more truthful (options never seen by a user showed up as parameters in the database previously) and enables the option_given method above. It also closed a hole where an unmet-requires option whose hidden field wasn't submitted got its default stored anyway.

Sensitive parameters

I found some gaps in how these were handled and actually some are still in our database (need to take care of that in migrate script on next release).

  • DataSet: added remove_sensitive_parameters to standardize and allow use of the method in different places (checks both the producing processor's and the current-type processor's options, in case a filter adopted a new type).
  • DataSet: added remove_sensitive_parameters_from_next, which recursively strips sensitive values from queued next/preset chains. This was the real leak — a datasource/preset that embeds a follow-up's api_key (e.g. audio_to_text, annotate-images) left it in the record forever. Called after the follow-ups are created, so they get their own copy first.
  • ThingExpirer: added expire_sensitive_parameters, which deletes sensitive parameters (top-level and nested next) for unfinished datasets over 7 days old. They should already be deleted, so this is just a fallback safety measure. (Basically would catch a DataSet with sensitive info that cannot run/crashes. Would catch a disabled datasets in limbo.)
  • Marked api_key sensitive on google-vision-api and annotate-images and dropped their manual delete_parameter calls (fragile — a crash before that line leaks the key).

Dev-only parameter lint

Noticed some old params in presets and used this to find them. Enabled via control panel (possible point for extension or other dev gating).

  • BasicProcessor.warn_unexpected_parameters: warns (never raises) when a dataset is queued programmatically (preset pipelines, next chains) with a parameter its target processor doesn't accept — the thing that silently drops a preset's mistaken or misdirected key. Called at the two programmatic doorways (after_process's next loop and ProcessorPreset.process). Exempts framework keys (FRAMEWORK_PARAMETER_KEYS), _-prefixed internal plumbing, and a per-processor accepted_parameters tuple ("keys I read but don't always offer as an option" — e.g. a config-gated option a preset deliberately passes).
  • Gated behind a new dev.mode config setting (a live-toggleable "Developer mode" toggle, off by default.

Tests

  • Option defaults must match their type (test_option_declarations — fixed 12 processors whose defaults were the wrong type or not a valid choice), and any worker defining validate_query must make it a @staticmethod (test_validate_query_declarations). Plus coverage for gated-option storage, the wrapper, the nested sensitive scrub, and the parameter lint.

Bug fixes surfaced along the way

The dev lint (and the option-declaration checks) turned up a set of real bugs, now fixed:

  • annotate-images queued convert-vision-to-csv, a processor type that does not exist (it's convert-google-vision-to-csv), so its final CSV-conversion step silently never ran.
  • similar-words passed timeframe to tokenise-posts, which reads docs_per — so the timeframe selection was silently dropped.
  • The TikTok video downloader passed the user's column choice under the wrong key for uploaded datasets; fixed and restructured to only send it where the metadata processor reads it.
  • Removed a batch of no-op stale keys from presets (neologisms, annotate-images) and the redundant reversed-date-range checks in several datasources (parse_all already raises on those).
  • audio_to_text copied its whole merged option set into the follow-up via query.copy(); now filters to the audio processor's own options.

Discussion:

  • If we were willing, we now have robust catching mechanisms (remove_sensitive_parameters + ThingExpirer) that could remove sensitive parameters after a dataset was run and properly finished. We could therefore delete sensitive parameters then and allow interrupted and/or crashed processors to retry w/ them. We could tighten the ThingExpirer in that case to cleanup quicker. (This PR keeps the original behavior — deletion at run start, i.e. immediate removal from the DB — as the default.)
  • The query in validate_query(query) != the query in get_items(query). Always the case, but before this query was NOT re-hydrated via get_options and thus differed from self.parameters. Since query does not exist in BasicProcessor.process() and is only available in Search.get_items() this difference was meaningful, potentially confusing, and could result in unusual behavior. This PR fixes that by aligning query and self.parameters (and get_parameters). The new option_given method can now be used to check if a option key actually was provided by the user. Fixes Search.get_items(): query vs self.parameters divergence #603.
    • I looked at updating self.parameters to provide this information (use requirements met to ensure the user actually made a decision/choose a default), but it requires a lot of diligence on developers to understand that self.parameters.get('some key', "meaningful value with key absent"). There are many places in our code where that default is not the same as the declared default in get_options and there are a few places where we use self.parameters['some key that may no longer be there'] which would crash (that's actually a benefit, but would require behavior change on our part as keys have been always present in self.parameters). So self.parameters still fills every declared option, and option_given is the explicit way to ask "did the user provide this?".

Future fun:

  • Clean up the self.parameters.get("key", "meaningless default often conflicts declared default anyway"). If it is in get_options, the key has always been in self.parameters (always defined as "many years at least"). Could even be replaced with self.parameter["key"] to actually throw if we mistype or change an option key.
  • Look at result-metadata.html handling of parameters. The parameters going forward will only be those accepted by the user. It thus becomes useful to include non-truthy parameters as they were a decision. (And the long hide certain keys possibly should be defined somewhere else and could respect sensitive and cache attributes.)
  • A one-time migrate script to scrub sensitive parameters from existing (pre-this-PR) finished datasets — the runtime handling only covers new ones going forward. (Needs a version bump, so tracked separately.)

please ignore misfortune branch name typo

dale-wahl added 23 commits July 8, 2026 17:13
@dale-wahl
dale-wahl marked this pull request as ready for review July 15, 2026 14:29
@dale-wahl
dale-wahl requested a review from stijn-uva July 15, 2026 14:30
@dale-wahl

Copy link
Copy Markdown
Member Author

@stijn-uva We discussed this one in person. As all things I touch, they grow. This was actually pretty illuminating and fixes a fair number of bugs. Mostly with sensitive parameters and some drift in options presets were passing to processors. I added warnings and a dev only mode that we may want to run on our server which should warn us of that sort of drift.

I think it is solid. I am going to build on it with a validate_query PR to add the actual validate_query methods to processors that would benefit (that should be mostly merge-able directly to master but I may use some of the functionality I added here). I also am going to add the db column for internal parameters (currently leaning internal_parameters because explicit=good, but would accept internals). That one follows with a migrate script which can be used to clean up some "old" parameters.

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.

Search.get_items(): query vs self.parameters divergence

1 participant