-
Notifications
You must be signed in to change notification settings - Fork 2.4k
feat(tests): add initial integration tests to repo #1048
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
| # Set up a basic teleprompter, which will compile our RAG program. | ||
| teleprompter = BootstrapFewShot(metric=validate_context_and_answer) | ||
| # Compile the RAG model | ||
| compiled_rag = teleprompter.compile(RAG(), trainset=trainset) |
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.
is there a way we can check the stdout during compilation and assert the existing printed message? Bootstrapped 4 full traces after 11 examples in round 0.
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.
it's a bit dirty to do that but it is doable to add it here.
I would recommend that for this, refactor the teleprompter and assign some values and function for retuning that. checking stdout wouldn't be the best practice here.
I suggest after merging this create a new issue for fixing this.
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| python-version: ["3.9"] |
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.
not fully sure but is it needed to specify a python version here? (can we extend this to >=3.9 as in dspy-ai?) and how does this affect users who don't have 3.9 - (saw a followup to this issue here more so for dspy-ai but similar impact).
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.
I haven't tested on other pythons, I would say definitely we can add other python versions testing but I suggest defer it after merging my other PR about moving all dependencies to the poetry.
pip requirements doesn't have good caching system in the CI and if we want to add other python version testing in our CI it would consume our CI resources very fast.
|
Thanks @ammirsm ! this looks great - excited to have these integration tests on board! Regarding the poetry dependencies, is the plan to still support both Additionally, what's the best way to test the integration tests in this PR? Would we have to create mock PRs or run GitHub actions locally? would love to have that documented as well as this PR is being reviewed. Also probably good to squash the commit history here :). |
ammirsm
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.
Thanks @ammirsm ! this looks great - excited to have these integration tests on board!
left a few general comments to add comprehensiveness.
thanks @arnavsinghvi11 for reviewing the PR. just replied to your comments and added some changes to the PR regarding to them.
Regarding the poetry dependencies, is the plan to still support both requirements.txt and poetry dependencies? it was a bit unclear from the comments above. Also, will we remove the extra requirements.txt before merging the PR?
In regard to poetry dependencies, we plan to eventually move everything to poetry as it's faster in the CI. After this PR is merged and auto deployment is integrated (#919), we'll proceed with the transition to poetry, which has been initiated here: #819
To clarify further:
Currently, we don't have any way to develop using pytest with our original requirements.txt. All dependencies for development and document generation are in poetry. I've added a new requirements-dev.txt to include all existing requirements for development there. This allows us to maintain everything in CI using pip installer for now. Once we've stabilized the requirements, we'll transition everything to poetry.
Additionally, what's the best way to test the integration tests in this PR? Would we have to create mock PRs or run GitHub actions locally? would love to have that documented as well as this PR is being reviewed.
I didn't quite understand this one but will try to add some clarification, lmk if I haven't got your question right.
To run the tests, you simply execute 'pytest tests_integration' locally. When do these tests run? They're executed with each merge and pull request to ensure they don't break our package. Additionally, Hanna will add another workflow that tests the package installed from PyPi, ensuring our package on PyPi is also functioning correctly.
Also probably good to squash the commit history here :).
haha! definitely will squash it for the merge.
| # Set up a basic teleprompter, which will compile our RAG program. | ||
| teleprompter = BootstrapFewShot(metric=validate_context_and_answer) | ||
| # Compile the RAG model | ||
| compiled_rag = teleprompter.compile(RAG(), trainset=trainset) |
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.
it's a bit dirty to do that but it is doable to add it here.
I would recommend that for this, refactor the teleprompter and assign some values and function for retuning that. checking stdout wouldn't be the best practice here.
I suggest after merging this create a new issue for fixing this.
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| python-version: ["3.9"] |
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.
I haven't tested on other pythons, I would say definitely we can add other python versions testing but I suggest defer it after merging my other PR about moving all dependencies to the poetry.
pip requirements doesn't have good caching system in the CI and if we want to add other python version testing in our CI it would consume our CI resources very fast.
|
Thanks @ammirsm - the comments look good from my end!
I see so just to clarify, we have a temporary
yup this is perfect! |
|
thanks for the reply @arnavsinghvi11 .
yes sir, basically bc we don't want to install dev dependencies during the package installation we put them in the new requirements file and we will remove it when we moved to poetry. |
📝 Description
🛠 Type of Change
🔍 How Has This Been Tested?
✅ Suggested Checklist:
requirements.txtfileruff check . --fix-onlyto appease the lint godsWe need to clean up and move to poetry completely, some places we are using poetry and some places requirements which is making it very hard to maintain dependencies.