Validate config min/max values #474
Conversation
| syntax.spacy_model = syntax.spacy_model or defaults.spacy_model | ||
| syntax.subj_tags = syntax.subj_tags or defaults.subj_tags | ||
| syntax.obj_tags = syntax.obj_tags or defaults.obj_tags | ||
| if similarity := values.get("similarity"): |
There was a problem hiding this comment.
I changed values["similarity"] to values.get("similarity") (as well as the others) because the individual fields get validated by Pydantic first, before calling this @root_validator(). If Pydantic raises a ValidationError while validating similarity (like if no_close_threshold is outside the min/max range), it caries on with the rest of the validation to get an exhaustive list of validation errors, so we end up here with similarity that doesn't exist. In this context, values["similarity"] was producing a no-helpful KeyError (and not a ValidationError) that was "hiding" the validation error.
There was a problem hiding this comment.
Thanks for the explanation!
| syntax.obj_tags = syntax.obj_tags or defaults.obj_tags | ||
| if values["similarity"]: | ||
| similarity = values["similarity"] | ||
| if syntax := values.get("syntax"): |
There was a problem hiding this comment.
Even though syntax is never optional at the moment?
There was a problem hiding this comment.
Yes, for the reason I gave here.
There was a problem hiding this comment.
Ah ok. if I rephrase, do you mean that if there is a problem with one syntax field (like the value being out of bound), the syntax options will be set to None or something like that, which will then break the old line 418, hiding the original error with the syntax field? If so, can we think of some comments to add so we don't forget? Or is basically your test testing that we have the appropriate error message?
There was a problem hiding this comment.
Yes, "hiding the original error" with a KeyError on old line 418.
My test ensures that we get the expected ValidationError, which is the only type of exception pydantic is supposed to return. A KeyError makes the test fail.
| no_close_threshold: float = 0.5 | ||
| faiss_encoder: str = Field("", description="Language-based dynamic default value.") | ||
| conflicting_neighbors_threshold: float = Field( | ||
| 0.9, ge=0, le=1, description="Threshold to use when finding conflicting neighbors." |
There was a problem hiding this comment.
Great idea to convert comments into descriptions!
There was a problem hiding this comment.
I think we should do it throughout maybe? But it can be in a future PR.
JosephMarinier
force-pushed
the
joseph/validate-config-min-max-values
branch
2 times, most recently
from
7ad8b87
to
7eb79be
Compare
…oad-config-history * commit '674585e9bdfd543771ee74b407059b41fc979f24': Clean up docker image (#316) Validate config min/max values (#474) Bump werkzeug from 2.1.0 to 2.2.3 (#443) Bump dns-packet from 5.3.1 to 5.4.0 in /webapp (#473) Update tensorflow (#355) Bump json5 from 1.0.1 to 1.0.2 in /webapp (#359) Fix negative query params (#471)
Description:
Very simple and repetitive PR about validating the min and max values to the number fields in the config.
Checklist:
You should check all boxes before the PR is ready. If a box does not apply, check it to acknowledge it.
ran
pre-commit run --all-filesat the end.our users.
READMEfiles and our wiki for any big design decisions, if relevant.