Change YAML version exception into a warning #2385
Conversation
ZanSara
added
topic:pipeline
type:refactor
There was a problem hiding this comment.
Please make sure that in strict_mode the version error is properly displayed. Currently a "usual" validation error is shown. Other than that it looks good. However I'd reconsider the name master version. How about accepting no version at all? Checking for it and then deleting before validation looks a little odd ;-)
Another improvement could be introducing the strict_mode param during saving a YAML. So version might be ommitted or set to master or whatever if we are not using strict_mode.
| @@ -1,5 +1,5 @@ | |||
| # Dummy pipeline, used when the CI needs to load the REST API to extract the OpenAPI specs. DO NOT USE. | |||
| version: 'unstable' | |||
| version: 'master' | |||
There was a problem hiding this comment.
I'm not sure about calling this master. Wouldn't that relate too much to the master branch of haystack? Just some other suggestions that popped into my mind: latest, current, any or leaving out the version completely in this case would also be an option.
There was a problem hiding this comment.
Good point. I originally excluded latest because it often means "latest released version", which in this case might be misleading. current and any aren't bad but I've never seen them used, probably just because they're vague... And leaving out the version altogether is an option, but it might hide the fact that you can specify the version if the versionless YAML become more popular (which they might).
On the other hand, I'm not sure tying the name to github is an issue. Whoever needs to use this label is very likely to be actually installing from master. We could find more generic ones like source, git or something like this, to avoid referencing the branch name? Even though those are rarely used in the wild as well.
Or maybe version: ignore to be even more upfront?
I think I could go for ignore, source or any, but let me know what do you think.
There was a problem hiding this comment.
I like ignore the most. This gives the best hint about what it means.
There was a problem hiding this comment.
The json schema file will still be called haystack-pipelines-master.schema.json just because haystack-pipelines-ignore.schema.json sounded odd 😄 But it has no impact on the content of the version field in YAML. Renamed all the rest 👍
haystack/pipelines/config.py
Outdated
| loaded_custom_nodes = [] | ||
| while True: | ||
| try: | ||
| Draft7Validator(schema).validate(instance=config_to_validate) |
There was a problem hiding this comment.
In the strict_version case this might still raise a ValidationError, circumventing the following proper version check.
…ack into tame_yaml_version_check
…ack into tame_yaml_version_check
Problem:
Solution
unstableintomasterignorestrict_versionflag invalidate_configto be able to still enforce a specific version in case it's needed later.Closes #2382