Initial evaluation schema validation#59
Conversation
dmjoy
left a comment
dmjoy
left a comment
There was a problem hiding this comment.
This seems like a totally fine place to start. I just left a few comments / minor requests. Would also be curious if it's possible to log warnings about missing or incomplete fields rather than just failing validation (i.e. we could warn that they haven't completed the "recommended" fields instead of strictly requiring them).
| ) | ||
| return self | ||
|
|
||
| # TODO: check with David what this should look like |
There was a problem hiding this comment.
The claim_aggregation_strategy field looks something like:
claim_aggregation_strategy:
type: fraction # could be 'fraction', 'all', 'any'
parameters: # this is an optional dict with arbitrary parameters in theory, though we currently only have the 'fraction' type implemented that has a paremeter, and that paremeter is 'threshold'
threshold: 0.6
| description: str | ||
| claim: ClaimSchema | ||
|
|
||
| # --- Recommended metadata --- |
There was a problem hiding this comment.
I'm leaning toward these being required metadata (if teams are missing these fields currently we are asking them to provide it).
There was a problem hiding this comment.
@b-fenelon - have we settled on whether to make these fields required?
There was a problem hiding this comment.
I would err on the side of making fields required. Those fields don't dictate evaluation strategy and probably shouldn't be a requirement for running a card (e.g. in testing), but we expect these entries for official submissions.
If this causes failures because of the example cards, I can populate fields as needed
| pipeline: Optional[dict[str, Any]] = None | ||
|
|
||
| @model_validator(mode='after') | ||
| def exclusive_backends(self) -> 'EvaluationCardSchema': |
There was a problem hiding this comment.
Might be good to check in here that if they don't define a kwdagger or pipeline field, that they should have symbols defined.
There was a problem hiding this comment.
@b-fenelon just to clarify the order of checks, is it possible to have a card that defines both symbols and one of pipeline and kwdagger?
There was a problem hiding this comment.
Yes, it's safe to say symbols is always valid (for now). Though, the current iteration of evaluating the kwdagger field may not make use of symbols, but I don't think we should restrict it
|
I should mention I also played around with these changes locally and it seemed to work well enough on the few cards I threw at it (some valid, some not) |
nitesh201
changed the title
|
@b-fenelon ready for review. |
|
No objections here @Erotemic Any strong thoughts? |
Erotemic
left a comment
Erotemic
left a comment
There was a problem hiding this comment.
Two nitpicks and one real requested change in the inline comments.
Additionally, I don't see any tests. We should add tests for schemas that correctly validate and error. This is critical. This feature requires corresponding tests.
| links: list[LinkSchema] | ||
|
|
||
| # --- Recommended --- | ||
| category: Optional[str] = None |
There was a problem hiding this comment.
I prefer ... | None over Optional[...], but I'm not going to block the PR on this detail.
| with open(path, 'r') as f: | ||
| cfg = yaml.safe_load(f) | ||
|
|
||
| EvaluationCardSchema.model_validate(cfg) |
There was a problem hiding this comment.
Can we make validation optional? Type checking is growing on me, but I've hit too much friction in libraries that do aggressive type validation (cough jsonargparse cough), even though if you ignored the check the code would run correctly.
I suggest change validate_only (the name of which is sort of a code smell), to --validate which is a Value with choices=['only', 'error', 'warning', 'off']. If it is set to "only", we do the validate only behavior. If it is "error" we do the current behavior. For "warning" it is similar, but catches the error emits a warning and tries to proceed anyway, and "off" ignores it entirely. The default should be "error".
|
@nitesh201 Can you make a push to address the final reviewer comments and get this merged? |
|
LGTM. |
5 participants
No description provided.