Fix schema validation and add custom validators #3220
Conversation
|
Sorry @dlstadther , I went too fast in #3217 . This time it should be fine. |
adrien-berchet
force-pushed
the
improve_schema
branch
from
8deef12
to
e60b153
Compare
luigi/parameter.py
Outdated
| unfrozen_value = recursively_unfreeze(frozen_value) | ||
| try: | ||
| self.schema.validate(unfrozen_value) # Validators may update the instance inplace | ||
| frozen_value = super().normalize(unfrozen_value) |
There was a problem hiding this comment.
I understand the try/except here to handle for when self.schema is a custom validator, but could you explain why you re-freeze the unfrozen value again? Why/how would the validator modify the value of the instance such that we cannot return the frozen_value from the beginning of this method?
There was a problem hiding this comment.
It is possible to define custom validators to handle specific schemas or do extra stuffs: https://python-jsonschema.readthedocs.io/en/stable/creating/
For example, it is possible to fill missing values with default values: https://python-jsonschema.readthedocs.io/en/stable/faq/#why-doesn-t-my-schema-s-default-property-set-the-default-on-my-instance
In this example the given instance is updated with default values during the validation, so the new instance must be frozen again and returned.
Nevertheless, we can indeed simplify this a bit: unfreeze the given value when a schema is provided and just freeze the result, instead of freezing, unfreezing and freezing again. I pushed a new commit for this.
| unfrozen_value = recursively_unfreeze(value) | ||
| try: | ||
| self.schema.validate(unfrozen_value) | ||
| value = unfrozen_value # Validators may update the instance inplace |
There was a problem hiding this comment.
I defined the unfrozen_value variable for clarity but we could just overwrite the value variable at line 1141. What do you prefer?
|
Let me try to understand the situation a bit better. Does it mean the main branch of Luigi is broken? We plan to drop a new release and if that is the case, we would suggest reverting the breaking PR so we have a healthy main branch, and you will also be able to work on this without a rush @adrien-berchet . |
|
Hi @honnix |
|
Note that the CI passed, only the coverage reported a failure but I am pretty sure it is not due to this PR (2 lines in EDIT: The issue is here: https://github.com/spotify/luigi/actions/runs/3939604194/jobs/6739654892 |
|
You're welcome 😊 |
Description
Fixes #3217 that breaks when
jsonschemais not installed (sorry about that 🤦 ).Also add the possibility to provide a custom validator as schema instead of just relying on
jsonschema.validate.