Skip to content

Conversation

@Suor
Copy link
Contributor

@Suor Suor commented Nov 14, 2019

voluptuous is compiling schema to a python function, which makes it around 7x faster for me. This is the second slowest part of stage collection, see #2671.

This only replaces schema validation library for stages, config still uses the old one. There is no point in using both, so if everyone is ok with this switch, I will rewrite the config schema too.

voluptuous is compiling schema to python function, which makes it
faster.
@Suor Suor requested review from efiop and shcheklein November 14, 2019 21:37
Optional(PARAM_REV): str,
Optional(PARAM_REV_LOCK): str,
}
REPO_SCHEMA = {PARAM_URL: str, PARAM_REV: str, PARAM_REV_LOCK: str}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So how does it differentiate required field from an optional one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a Required() wrapper.

@Suor Suor changed the title [WIP] perf: switch schema validation library perf: switch schema validation library Nov 18, 2019
@Suor
Copy link
Contributor Author

Suor commented Nov 18, 2019

Switched config validation to voluptuous too, removed the req for schema.

@efiop efiop requested review from a user and pared November 19, 2019 00:09
@efiop
Copy link
Contributor

efiop commented Nov 19, 2019

@Suor Please rebase.

"nanotime>=0.5.2",
"pyasn1>=0.4.1",
"schema>=0.6.7",
"voluptuous>=0.11.7",
Copy link
Contributor

@efiop efiop Nov 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: need to not forget to update it in conda package deps.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is great 👍

@efiop efiop merged commit c7c852a into treeverse:master Nov 20, 2019
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.

4 participants