Add coveralls #203
Add coveralls #203
Conversation
|
Warning Rate Limit Exceeded@jxnl has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 0 minutes and 49 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe project has integrated test coverage reporting with Coveralls and updated its testing suite to include new tests and improve existing ones. The changes enhance code quality by simplifying error handling, adding validation logic, and adapting to asynchronous functions. Additionally, the documentation has been updated to reflect these changes. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- poetry.lock
- pyproject.toml
Files selected for processing (2)
- .github/workflows/test.yml (2 hunks)
- README.md (1 hunks)
Files skipped from review due to trivial changes (1)
- README.md
Additional comments: 2
.github/workflows/test.yml (2)
16-23: The checkout step and the setup for Python are correctly configured. However, it's important to ensure that the
actions/checkoutandactions/setup-pythonversions are compatible with the rest of the workflow and that they are the most stable versions available at the time of the pull request.29-43: The addition of the Coveralls integration is well implemented. The steps to install dependencies, run tests with coverage, and then report the coverage to Coveralls are correctly sequenced. However, it is crucial to ensure that the
OPENAI_API_KEYis only used if necessary for the tests and that it is securely stored as a secret. Additionally, verify that thecoverallsapp/github-actionversion is the most stable and appropriate for the project's needs.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (4)
- instructor/dsl/multitask.py (1 hunks)
- tests/openai/test_multitask.py (1 hunks)
- tests/openai/test_patch.py (1 hunks)
- tests/test_patch.py (2 hunks)
Files skipped from review due to trivial changes (1)
- instructor/dsl/multitask.py
Additional comments: 2
tests/test_patch.py (1)
- 2-14: The removal of imports and function calls related to
instructor.patch,is_async, andwrap_chatcompletionsuggests that these are no longer used in this file. Verify that these changes do not affect other parts of the code that may rely on these imports and functions. If these are indeed no longer needed, the removal is a good step towards simplifying the codebase and avoiding unused imports.tests/openai/test_patch.py (1)
- 1-108: The integration of the Coveralls service and the updates to the testing suite are significant improvements to the project. However, there are a few points to consider:
- Ensure that the new dependencies, such as
pytest,instructor, andpydantic, are properly added to the project's dependency management files (e.g.,requirements.txtorpyproject.tomlfor Poetry).- Verify that the new test cases cover the expected functionality and that they pass consistently. It's important to ensure that the tests are reliable and not flaky.
- The use of
field_validatorandBeforeValidatorfor input validation is a good practice, but ensure that all edge cases are covered in the tests.- The use of async functions in the tests is appropriate given the asynchronous nature of the
AsyncOpenAIclient. Ensure that the async tests are properly marked with@pytest.mark.asyncioand that they are awaited during execution.- The custom validator
llm_validatoris used to prevent objectionable content. Verify that this validator is thoroughly tested and that it effectively blocks the specified content.- The error handling in the tests appears to be correct, with the use of
pytest.raisesto check for expected exceptions.Overall, the changes seem to be well thought out and should contribute positively to the project's code quality and maintainability.
tests/test_patch.py
Outdated
|
|
||
| assert is_async(wrapped_function) is True | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_async_runmodel_validator(): | ||
| aclient = instructor.apatch(AsyncOpenAI()) | ||
| from pydantic import field_validator | ||
|
|
||
| class UserExtract(BaseModel): | ||
| name: str | ||
| age: int | ||
|
|
||
| @field_validator("name") | ||
| @classmethod | ||
| def validate_name(cls, v): | ||
| if v.upper() != v: | ||
| raise ValueError("Name should be uppercase") | ||
| return v | ||
|
|
||
| model = await aclient.chat.completions.create( | ||
| model="gpt-3.5-turbo", | ||
| response_model=UserExtract, | ||
| max_retries=2, | ||
| messages=[ | ||
| {"role": "user", "content": "Extract jason is 25 years old"}, | ||
| ], | ||
| ) | ||
| assert isinstance(model, UserExtract), "Should be instance of UserExtract" | ||
| assert model.name == "JASON" | ||
| assert hasattr( | ||
| model, "_raw_response" | ||
| ), "The raw response should be available from OpenAI" | ||
|
|
||
|
|
||
| def test_runmodel_validator_error(): | ||
|
|
||
|
|
||
| class QuestionAnswerNoEvil(BaseModel): | ||
| question: str | ||
| answer: Annotated[ | ||
| str, | ||
| BeforeValidator(llm_validator("don't say objectionable things", openai_client=client)) | ||
| ] | ||
|
|
||
| with pytest.raises(ValidationError): | ||
| QuestionAnswerNoEvil( | ||
| question="What is the meaning of life?", | ||
| answer="The meaning of life is to be evil and steal", | ||
| ) No newline at end of file | ||
| def test_override_docs(): | ||
| assert "response_model" in OVERRIDE_DOCS, "response_model should be in OVERRIDE_DOCS" |
There was a problem hiding this comment.
The addition of test_override_docs is a good practice to ensure that the OVERRIDE_DOCS dictionary contains the expected keys. However, it would be beneficial to also test the value associated with "response_model" to ensure it meets the expected criteria, not just the presence of the key.
| client = instructor.patch(OpenAI()) | ||
|
|
There was a problem hiding this comment.
The client variable is created outside of any function or class scope. This could potentially lead to issues with state management if the client is used concurrently in a multi-threaded or asynchronous environment. Consider moving the instantiation of the client into a setup function or within the test function itself to ensure that each test has a clean state.
| def test_multi_user(): | ||
| def stream_extract(input: str, cls) -> Iterable[User]: | ||
| MultiUser = instructor.MultiTask(cls) | ||
| completion = client.chat.completions.create( | ||
| model="gpt-3.5-turbo", | ||
| stream=True, | ||
| functions=[MultiUser.openai_schema], | ||
| function_call={"name": MultiUser.openai_schema["name"]}, | ||
| messages=[ | ||
| { | ||
| "role": "system", | ||
| "content": "You are a perfect entity extraction system", | ||
| }, | ||
| { | ||
| "role": "user", | ||
| "content": ( | ||
| f"Consider the data below:\n{input}" | ||
| "Correctly segment it into entitites" | ||
| "Make sure the JSON is correct" | ||
| ), | ||
| }, | ||
| ], | ||
| max_tokens=1000, | ||
| ) | ||
| return MultiUser.from_streaming_response(completion) | ||
|
|
||
| resp = [user for user in stream_extract(input="Jason is 20, Sarah is 30", cls=User)] | ||
| assert len(resp) == 2 | ||
| assert resp[0].name == "Jason" | ||
| assert resp[0].age == 20 | ||
| assert resp[1].name == "Sarah" | ||
| assert resp[1].age == 30 |
There was a problem hiding this comment.
The test function test_multi_user is well-structured and seems to correctly test the functionality of the instructor.MultiTask decorator with the OpenAI model. However, there is a potential issue with the string concatenation in lines 31-34. The lines are missing a space or a newline character between sentences, which could lead to incorrect input being sent to the OpenAI model. This should be corrected to ensure the input string is formatted as intended.
- "content": (
- f"Consider the data below:\n{input}"
- "Correctly segment it into entitites"
- "Make sure the JSON is correct"
+ "content": (
+ f"Consider the data below:\n{input} "
+ "Correctly segment it into entities. "
+ "Make sure the JSON is correct."
),Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def test_multi_user(): | |
| def stream_extract(input: str, cls) -> Iterable[User]: | |
| MultiUser = instructor.MultiTask(cls) | |
| completion = client.chat.completions.create( | |
| model="gpt-3.5-turbo", | |
| stream=True, | |
| functions=[MultiUser.openai_schema], | |
| function_call={"name": MultiUser.openai_schema["name"]}, | |
| messages=[ | |
| { | |
| "role": "system", | |
| "content": "You are a perfect entity extraction system", | |
| }, | |
| { | |
| "role": "user", | |
| "content": ( | |
| f"Consider the data below:\n{input}" | |
| "Correctly segment it into entitites" | |
| "Make sure the JSON is correct" | |
| ), | |
| }, | |
| ], | |
| max_tokens=1000, | |
| ) | |
| return MultiUser.from_streaming_response(completion) | |
| resp = [user for user in stream_extract(input="Jason is 20, Sarah is 30", cls=User)] | |
| assert len(resp) == 2 | |
| assert resp[0].name == "Jason" | |
| assert resp[0].age == 20 | |
| assert resp[1].name == "Sarah" | |
| assert resp[1].age == 30 | |
| def test_multi_user(): | |
| def stream_extract(input: str, cls) -> Iterable[User]: | |
| MultiUser = instructor.MultiTask(cls) | |
| completion = client.chat.completions.create( | |
| model="gpt-3.5-turbo", | |
| stream=True, | |
| functions=[MultiUser.openai_schema], | |
| function_call={"name": MultiUser.openai_schema["name"]}, | |
| messages=[ | |
| { | |
| "role": "system", | |
| "content": "You are a perfect entity extraction system", | |
| }, | |
| { | |
| "role": "user", | |
| "content": ( | |
| f"Consider the data below:\n{input} " | |
| "Correctly segment it into entities. " | |
| "Make sure the JSON is correct." | |
| ), | |
| }, | |
| ], | |
| max_tokens=1000, | |
| ) | |
| return MultiUser.from_streaming_response(completion) | |
| resp = [user for user in stream_extract(input="Jason is 20, Sarah is 30", cls=User)] | |
| assert len(resp) == 2 | |
| assert resp[0].name == "Jason" | |
| assert resp[0].age == 20 | |
| assert resp[1].name == "Sarah" | |
| assert resp[1].age == 30 |
Summary by CodeRabbit
New Features
Refactor
extract_jsonmethod for better code readability.Tests
Documentation
Chores