-
Notifications
You must be signed in to change notification settings - Fork 53
feat: add the ability to run examples with pytest #198
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
Conversation
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Enforce conventional commitWonderful, this rule succeeded.Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
|
|
||
| return ExampleFile.from_parent(parent, path=file_path) | ||
|
|
||
| # TODO: Support running jupyter notebooks: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: you could do this with nbmake https://github.com/treebeardtech/nbmake
avinash2692
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM apart from a couple of things:
- might make sense to add
nbmakein this PR before merging so that we have notebooks also covered in the tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want all conftest.py to be in the same place. Not sure how the hierarchy works with pytest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe eventually, but I don't think so right now:
- I don't think we always want the examples to run by default. We would have to write new code to disable them on github runner's, etc...
- The tests are different enough that they should almost always be run separately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it requires changing how the pytest command is invoked. I think that change is fine, but I we should think about when we want to run the examples, etc...
I added |
avinash2692
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I'll look into addinf nbmake is a separate PR.
…ng#198) * feat: add conftest to run examples as tests * fix: fix errors with granite guardian req generation * fix: copy behavior with mots, add tests, add raises to genslot * fix: update codespell precommit to support ignore * fix: add note about nbmake
Add
pytest docsso that you can run most of the examples as a single pytest run. I had to modify a few tests to get them to be runnable from any directory and without user input.Also updated our codespell precommit hook so that we can use
#ignore:codespellsyntax for individual lines.Found and fixed bugs as a part of this:
__copy__and__deepcopy__dunder methods to ModelOutputThunk to prevent errors when generating with sampling strategies since sampling strategies copy actionsraisesclause to the docstring of generative slots since validation can occasionally failOutput looks like regular pytest and identifies the example file:
Some of the examples have to be skipped since they can't easily be run in a pytest compatible way. When pytest runs on the docs directory, a reminder is output to run those files manually: