Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft: Add schema validation #40

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Draft: Add schema validation #40

wants to merge 2 commits into from

Conversation

leiteg
Copy link
Member

@leiteg leiteg commented Sep 5, 2024

Hey guys, I have an implementation of the schema for the spinner YAML configuration file. To make the implementation simpler, I made some changes to the current schema and I want opinions on this.

  1. The "application specification" that used to be nested inside the metadata was moved to its own top level section applications. The problem with the current approach is that pydantic doesn't like arbitrary key names in YAML (e.g. the name of the application) mixed with fixed key names (e.g. description, version, runs, ...).
  2. I did the same with the benchmarks that would reside in the top level of the YAML file, I created a new section called benchmarks for this purpose.

Here is an example that is currently accepted by the implementation.

metadata:
    description: Lorem ipsum dolor sit amet
    version: v1.0
    runs: 10

applications:
    sleep:
        command:
            template: sleep {{sleep_duration}}

benchmarks:
    sleep:
        sleep_duration: [1, 10, 100]

What do you guys think?

TODOs

Non-Trivial Validation

  • Benchmark identifier must exist in applications
  • Benchmark parameters must exist in the command template
  • Application parameters must exist in the corresponding benchmark
  • Plot axes and group must be a valid identifier
  • Output lambda must be valid python

Misc

  • Write more tests
  • Migrate from pytest to unittest
  • Present errors to user
  • Integrate new API into existing code

@leiteg leiteg self-assigned this Sep 5, 2024
@leiteg leiteg marked this pull request as draft September 5, 2024 16:09
@rodrigo-ceccato
Copy link
Contributor

LG, overall I prefer this schema over the previous one. Just a few questions:

  1. Do we have to add pytest to the requirements?
  2. Can we invoke the tests in /tests in the CI?
  3. @cl3to what do you guys think about hierarchical overrides (e.g., setting a timeout inside a specific benchmark overrides the whole-experiment value for the timeout)? This could be another PR, tho
  4. Will the schema check work with the --extra-args in PR Add --extra-args option #39?

@leiteg
Copy link
Member Author

leiteg commented Sep 5, 2024

Thanks @rodrigo-ceccato!

  1. Do we have to add pytest to the requirements?

Not at all, I am just used to pytest. We can do it with unittests if you prefer.

  1. Can we invoke the tests in /tests in the CI?

Yes! I will leave this for last right before we merge.

  1. Will the schema check work with the --extra-args in PR Add --extra-args option #39?

I was not aware of --extra-args, I will take a look asap.

@leiteg leiteg force-pushed the feat/schema branch 2 times, most recently from 7abf796 to 2637701 Compare September 5, 2024 20:50
@rodrigo-ceccato
Copy link
Contributor

rodrigo-ceccato commented Sep 5, 2024

  1. Do we have to add pytest to the requirements?

Not at all, I am just used to pytest. We can do it with unittests if you prefer.

I am fine with pytest, I just thought it was not added to the requirements of the package

@leiteg
Copy link
Member Author

leiteg commented Sep 5, 2024

I am fine with pytest, I just thought it was not added to the requirements of the package

Oh, I didn't understand what you said the first time hehe can I keep pytest then?

@rodrigo-ceccato
Copy link
Contributor

I am fine with pytest, I just thought it was not added to the requirements of the package

Oh, I didn't understand what you said the first time hehe can I keep pytest then?

Please do! hehe

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.

2 participants