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

feat: add blog post and example for LLM-based reranker #1115

Merged
merged 5 commits into from
Oct 24, 2024
Merged

Conversation

jxnl
Copy link
Collaborator

@jxnl jxnl commented Oct 24, 2024

Important

Adds blog post and example code for LLM-based reranker using Instructor and Pydantic, detailing setup, model definitions, and testing.

  • Blog Post:
    • Adds llm-as-reranker.md detailing LLM-based reranker creation using Instructor and Pydantic.
    • Covers environment setup, model definitions, reranker function, and testing.
  • Example Code:
    • Adds run.py in examples/reranker/ demonstrating reranker implementation.
    • Defines Label and RerankedResults models using Pydantic.
    • Implements rerank_results() function to rerank text chunks based on query relevance.
    • Includes main() function to test reranker with sample data.

This description was created by Ellipsis for 87b184e. It will automatically update as commits are pushed.

@jxnl jxnl changed the title .. ... Oct 24, 2024
@ellipsis-dev ellipsis-dev bot changed the title ... feat: add blog post and example for LLM-based reranker Oct 24, 2024
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to 2610314 in 38 seconds

More details
  • Looked at 311 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 9 drafted comments based on config settings.
1. examples/reranker/run.py:25
  • Draft comment:
    @field_validator is deprecated in Pydantic v2. Use @validator instead. This applies to all instances of field_validator.
  • Reason this comment was not posted:
    Marked as duplicate.
2. docs/blog/posts/llm-as-reranker.md:82
  • Draft comment:
    Ensure that the response from client.chat.completions.create is properly parsed into a RerankedResults object, as it might not directly return this type.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The rerank_results function is expected to return a RerankedResults object, but the client.chat.completions.create method might not directly return this type. Ensure that the response is properly parsed into a RerankedResults object.
3. examples/reranker/run.py:32
  • Draft comment:
    Ensure that the response from client.chat.completions.create is properly parsed into a RerankedResults object, as it might not directly return this type.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The rerank_results function is expected to return a RerankedResults object, but the client.chat.completions.create method might not directly return this type. Ensure that the response is properly parsed into a RerankedResults object.
4. docs/blog/posts/llm-as-reranker.md:81
  • Draft comment:
    The function rerank_results is defined here and also in examples/reranker/run.py. Consider defining it in one place and importing it where needed to adhere to the DRY principle.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The function rerank_results is defined in both the blog post and the example file, which violates the DRY principle. It should be defined in one place and imported where needed.
5. examples/reranker/run.py:31
  • Draft comment:
    The function rerank_results is defined here and also in docs/blog/posts/llm-as-reranker.md. Consider defining it in one place and importing it where needed to adhere to the DRY principle.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The function rerank_results is defined in both the blog post and the example file, which violates the DRY principle. It should be defined in one place and imported where needed.
6. docs/blog/posts/llm-as-reranker.md:68
  • Draft comment:
    Add a comment explaining the purpose of the field_validator method in the RerankedResults class to improve code readability.
  • Reason this comment was not posted:
    Confidence changes required: 70%
    The field_validator method in the RerankedResults class is not documented, which makes it difficult to understand its purpose without reading the code. Adding a comment would help.
7. examples/reranker/run.py:25
  • Draft comment:
    Add a comment explaining the purpose of the field_validator method in the RerankedResults class to improve code readability.
  • Reason this comment was not posted:
    Confidence changes required: 70%
    The field_validator method in the RerankedResults class is not documented, which makes it difficult to understand its purpose without reading the code. Adding a comment would help.
8. docs/blog/posts/llm-as-reranker.md:116
  • Draft comment:
    The rerank_results function is defined here and also in examples/reranker/run.py. Consider defining it in one place and importing it where needed to adhere to the DRY principle.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The rerank_results function is defined in both the blog post and the example file, which violates the DRY principle. It should be defined in one place and imported where needed.
9. examples/reranker/run.py:66
  • Draft comment:
    The rerank_results function is defined here and also in docs/blog/posts/llm-as-reranker.md. Consider defining it in one place and importing it where needed to adhere to the DRY principle.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The rerank_results function is defined in both the blog post and the example file, which violates the DRY principle. It should be defined in one place and imported where needed.

Workflow ID: wflow_jdD1mprQTlcmKJ1L


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

docs/blog/posts/llm-as-reranker.md Show resolved Hide resolved
Copy link

cloudflare-workers-and-pages bot commented Oct 24, 2024

Deploying instructor-py with  Cloudflare Pages  Cloudflare Pages

Latest commit: 87b184e
Status: ✅  Deploy successful!
Preview URL: https://8f6b976b.instructor-py.pages.dev
Branch Preview URL: https://blog-reranker.instructor-py.pages.dev

View logs

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on dc77df7 in 15 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_6YKjWZ2NV7byXILv


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Incremental review on 7c95ec8 in 51 seconds

More details
  • Looked at 88 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. examples/reranker/run.py:23
  • Draft comment:
    The validate_chunk_id method expects v to be an int, but chunk_id is defined as a str. This will cause a type mismatch error. Update the type hint and logic accordingly.
  • Reason this comment was not posted:
    Marked as duplicate.
2. examples/reranker/run.py:67
  • Draft comment:
    Ensure the chunk_id in the template matches the updated chunk_id field in the Label class. This should be consistent with the data structure used.
  • Reason this comment was not posted:
    Confidence changes required: 70%
    The chunk_id in the template should match the updated chunk_id field in the Label class.
3. examples/reranker/run.py:115
  • Draft comment:
    Ensure the print statement handles cases where chunk_id is not found in chunks to prevent runtime errors. Consider using a default value or error handling.
  • Reason this comment was not posted:
    Confidence changes required: 70%
    The print statement should handle cases where chunk_id is not found in chunks. This prevents potential runtime errors.
4. examples/reranker/run.py:67
  • Draft comment:
    The template string uses {{ chunk.id }} but the chunks dictionary uses chunk['id']. Update the template to use {{ chunk['id'] }} for consistency.
                <chunk chunk_id="{{ chunk['id'] }}">
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment suggests a change for consistency, but the current code already uses chunk.id consistently. The suggestion to change to chunk['id'] is not necessary as the current implementation is consistent and correct. The comment does not point out a necessary change.
    I might be missing the context of why the comment suggests using chunk['id'] instead of chunk.id. However, the current code seems consistent and correct with the use of chunk.id.
    The use of chunk.id is consistent with the rest of the code, and there is no strong evidence that changing to chunk['id'] is necessary.
    The comment is not necessary as the current code is consistent and correct with the use of chunk.id. The comment should be deleted.
5. examples/reranker/run.py:115
  • Draft comment:
    The next function uses chunk['id'] but the Label class uses chunk_id. Ensure consistency by using chunk_id in both places.
            f"Text: {next(chunk['text'] for chunk in chunks if chunk['id'] == label.chunk_id)}"
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment suggests a change that would actually break the code. The 'chunks' list contains dictionaries with an 'id' key, so using 'chunk['id']' is correct. The Label class uses 'chunk_id' to refer to these ids, which is consistent with the data structure. The comment is incorrect because it suggests a change that would lead to a KeyError.
    I might be missing some context about why the comment was made, but based on the provided code, the current implementation is correct. The comment might be assuming a different data structure or misunderstanding the current one.
    The code is consistent with the data structure of the chunks list, and the comment's suggestion would introduce an error. The current implementation is correct as it is.
    The comment is incorrect because it suggests a change that would break the code. The current implementation is consistent with the data structure and should remain as is.

Workflow ID: wflow_CXRD2wF7rd8rN3E3


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.


@field_validator("chunk_id")
@classmethod
def validate_chunk_id(cls, v: int, info: ValidationInfo) -> int:
Copy link
Contributor

Choose a reason for hiding this comment

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

The validate_chunk_id method expects v to be an int, but chunk_id is defined as a str. Update the type of v to str for consistency.

Suggested change
def validate_chunk_id(cls, v: int, info: ValidationInfo) -> int:
def validate_chunk_id(cls, v: str, info: ValidationInfo) -> str:

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on b407427 in 11 seconds

More details
  • Looked at 40 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. examples/reranker/run.py:33
  • Draft comment:
    For compatibility with Python versions below 3.9, consider using List from the typing module instead of list. This applies to other instances in the code as well.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The code uses list instead of List from typing, which is not compatible with Python versions below 3.9.
2. examples/reranker/run.py:33
  • Draft comment:
    Use List from typing for type hints to maintain consistency.
    labels: List[Label] = Field(description="List of labeled and ranked chunks")
  • Reason this comment was not posted:
    Confidence changes required: 66%
    The code uses list type hints without importing List from typing, which is inconsistent with the rest of the code.
3. examples/reranker/run.py:41
  • Draft comment:
    Use List from typing for type hints to maintain consistency.
def rerank_results(query: str, chunks: List[dict]) -> RerankedResults:
  • Reason this comment was not posted:
    Confidence changes required: 66%
    The code uses list type hints without importing List from typing, which is inconsistent with the rest of the code.

Workflow ID: wflow_QR2QgF2IhbP5pOIw


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 87b184e in 13 seconds

More details
  • Looked at 35 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. docs/blog/posts/llm-as-reranker.md:65
  • Draft comment:
    Consider using List[Label] from typing for compatibility with Python versions earlier than 3.9.
labels: List[Label] = Field(description="List of labeled and ranked chunks")
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The use of list[Type] syntax is a Python 3.9+ feature. If the codebase supports older versions, this could cause compatibility issues.
2. docs/blog/posts/llm-as-reranker.md:69
  • Draft comment:
    Consider using List[Label] from typing for compatibility with Python versions earlier than 3.9.
def model_validate(cls, v: List[Label]) -> List[Label]:
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The use of list[Type] syntax is a Python 3.9+ feature. If the codebase supports older versions, this could cause compatibility issues.
3. docs/blog/posts/llm-as-reranker.md:80
  • Draft comment:
    Consider using List[dict] from typing for compatibility with Python versions earlier than 3.9.
def rerank_results(query: str, chunks: List[dict]) -> RerankedResults:
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The use of list[Type] syntax is a Python 3.9+ feature. If the codebase supports older versions, this could cause compatibility issues.
4. docs/blog/posts/llm-as-reranker.md:62
  • Draft comment:
    Ensure that the change from List to list in type annotations is consistent throughout the code. This applies to all instances in the file.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The change from List to list in type annotations is consistent and should be reflected in all relevant parts of the code.

Workflow ID: wflow_kObVB5ulSfAthjao


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@ivanleomk ivanleomk merged commit bc32f6c into main Oct 24, 2024
14 of 15 checks passed
@ivanleomk ivanleomk deleted the blog-reranker branch October 24, 2024 10:07
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