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

chore: Fix Typing Issues #27

Merged
merged 10 commits into from
Aug 23, 2024
Merged

chore: Fix Typing Issues #27

merged 10 commits into from
Aug 23, 2024

Conversation

danielchalef
Copy link
Member

@danielchalef danielchalef commented Aug 23, 2024

🚀 This description was created by Ellipsis for commit 089a7ec

Summary:

This PR refines type annotations and imports across multiple files for improved clarity and consistency.

Key points:

  • Refine type annotations and imports for clarity and consistency.
  • Import Message model in core/llm_client/client.py and core/llm_client/openai_client.py.
  • Update generate_response method to use list[Message].
  • Replace any with typing.Any in type annotations.
  • Import EntityEdge in core/search/search.py.
  • Update SearchResults class to use list[EpisodicNode], list[EntityNode], list[EntityEdge].
  • Update imports in core/utils/__init__.py.
  • Remove unused import Any in core/utils/maintenance/edge_operations.py.
  • Update type annotations in core/utils/maintenance/node_operations.py.
  • Affected files: core/graphiti.py, core/nodes.py, core/prompts, core/utils/maintenance.

Generated with ❤️ by ellipsis.dev

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! Reviewed everything up to d4d73b1 in 50 seconds

More details
  • Looked at 426 lines of code in 15 files
  • Skipped 0 files when reviewing.
  • Skipped posting 8 drafted comments based on config settings.
1. core/graphiti.py:84
  • Draft comment:
    Use typing.Any instead of any for type annotations. This change is already applied in other parts of the codebase.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is redundant because the change it suggests has already been made in the code. It does not provide any new information or require further action from the author.
    I might be overlooking the possibility that the comment is meant to confirm the change was correctly applied, but the rules specify not to make purely informative comments.
    Even if the comment is meant to confirm the change, it is not necessary as the change is already visible in the diff.
    The comment should be removed because it is redundant and does not require any further action.
2. core/llm_client/client.py:14
  • Draft comment:
    Use typing.Any instead of any for type annotations. This change is already applied in other parts of the codebase.
  • Reason this comment was not posted:
    Marked as duplicate.
3. core/llm_client/openai_client.py:19
  • Draft comment:
    Use typing.Any instead of any for type annotations. This change is already applied in other parts of the codebase.
  • Reason this comment was not posted:
    Marked as duplicate.
4. core/nodes.py:38
  • Draft comment:
    Use typing.Any instead of any for type annotations. This change is already applied in other parts of the codebase.
  • Reason this comment was not posted:
    Marked as duplicate.
5. core/prompts/dedupe_edges.py:18
  • Draft comment:
    Use typing.Any instead of any for type annotations. This change is already applied in other parts of the codebase.
  • Reason this comment was not posted:
    Marked as duplicate.
6. core/prompts/dedupe_nodes.py:19
  • Draft comment:
    Use typing.Any instead of any for type annotations. This change is already applied in other parts of the codebase.
  • Reason this comment was not posted:
    Marked as duplicate.
7. core/prompts/extract_edges.py:17
  • Draft comment:
    Use typing.Any instead of any for type annotations. This change is already applied in other parts of the codebase.
  • Reason this comment was not posted:
    Marked as duplicate.
8. core/prompts/extract_nodes.py:19
  • Draft comment:
    Use typing.Any instead of any for type annotations. This change is already applied in other parts of the codebase.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_KOHzzHWvRQQfe5Bi


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

@danielchalef danielchalef changed the title chore: Import Message model in llm_client chore: Fix Typing Issues Aug 23, 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.

👍 Looks good to me! Incremental review on a7e5a88 in 46 seconds

More details
  • Looked at 719 lines of code in 16 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 drafted comments based on config settings.
1. core/graphiti.py:74
  • Draft comment:
    The sources parameter in the retrieve_episodes method is removed, but this change is not mentioned in the PR description. Ensure this change is intentional and update the PR description if necessary.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR description mentions replacing any with typing.Any for type annotations. However, in core/graphiti.py, the sources parameter in the retrieve_episodes method is removed, which is not mentioned in the PR description. This change might be unintentional or undocumented.
2. core/llm_client/client.py:14
  • Draft comment:
    The get_embedder method returns typing.Any. Consider using a more specific type for better type safety.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The get_embedder method in LLMClient and OpenAIClient returns typing.Any. This is not ideal for type safety. Consider using a more specific type if possible.
3. core/llm_client/openai_client.py:23
  • Draft comment:
    The generate_response method returns dict[str, typing.Any]. Consider using a more specific type for better type safety.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In openai_client.py, the generate_response method uses typing.Any for the return type. This could be more specific for better type safety.
4. core/prompts/lib.py:84
  • Draft comment:
    The PromptLibraryWrapper class uses # type: ignore[arg-type]. Consider ensuring type correctness to avoid suppressing type errors.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In core/prompts/lib.py, the PromptLibraryWrapper class uses # type: ignore[arg-type] to suppress type errors. This should be avoided if possible by ensuring type correctness.
5. core/search/search.py:32
  • Draft comment:
    The embedder parameter uses typing.Any. Consider using a more specific type for better type safety.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In core/search/search.py, the hybrid_search function uses typing.Any for the embedder parameter. Consider using a more specific type for better type safety.
6. core/utils/bulk_utils.py:191
  • Draft comment:
    The resolve_edge_pointers function does not return a value, but it is used as if it does. Consider returning the modified list explicitly.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In core/utils/bulk_utils.py, the resolve_edge_pointers function does not return a value, but it is used as if it does. This could lead to confusion.

Workflow ID: wflow_hAY2DlmmNIN7CeSp


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

core/graphiti.py Outdated Show resolved Hide resolved
core/graphiti.py Show resolved Hide resolved
core/graphiti.py Outdated Show resolved Hide resolved
core/prompts/dedupe_edges.py Outdated Show resolved Hide resolved
core/prompts/dedupe_edges.py Outdated Show resolved Hide resolved
core/utils/search/search_utils.py Outdated Show resolved Hide resolved
core/utils/search/search_utils.py Outdated Show resolved Hide resolved
core/utils/search/search_utils.py Outdated Show resolved Hide resolved
core/utils/search/search_utils.py Outdated Show resolved Hide resolved
tests/utils/maintenance/test_temporal_operations_int.py Outdated Show resolved Hide resolved
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 f9ac342 in 40 seconds

More details
  • Looked at 392 lines of code in 9 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. core/utils/maintenance/__init__.py:1
  • Draft comment:
    Add EntityEdge to the imports to ensure invalidate_edges works correctly.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment seems speculative as it suggests adding an import without any evidence from the diff that 'EntityEdge' is needed. The diff does not show any changes related to 'EntityEdge' or 'invalidate_edges' that would necessitate this import.
    I might be missing context from other parts of the codebase where 'EntityEdge' is indeed required for 'invalidate_edges'. However, based on the diff alone, there is no indication of this requirement.
    The task is to review the diff provided, and based on this diff, there is no evidence that 'EntityEdge' is needed. Therefore, the comment seems unnecessary.
    The comment should be removed as it suggests a change that is not supported by the diff provided.

Workflow ID: wflow_tOquQCmLXJfZbeyh


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 129eefd in 17 seconds

More details
  • Looked at 80 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. core/utils/maintenance/edge_operations.py:4
  • Draft comment:
    Any is used in this file but not imported. Consider importing Any from typing.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is incorrect because Any is not used in the file, and the import was removed for that reason. The comment does not suggest a necessary code change.
    I might be missing some context about the usage of Any elsewhere, but based on the provided code, it seems clear that Any is not needed.
    The code provided does not show any usage of Any, so the comment is not applicable.
    The comment should be removed because it suggests an unnecessary import of Any.

Workflow ID: wflow_yYE8GFSa9bpaNsOY


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 67dc5c6 in 17 seconds

More details
  • Looked at 25 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. core/search/search.py:10
  • Draft comment:
    Reordering imports without a specific reason can reduce readability. Consider keeping imports in alphabetical order for consistency.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    The import statement for EpisodicNode and EntityNode was reordered, but this change is not necessary and does not improve the code. It is better to keep the imports in alphabetical order for consistency.

Workflow ID: wflow_8J0UQ5qNtd2Z1FpF


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 ac18d4c in 36 seconds

More details
  • Looked at 93 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. core/nodes.py:38
  • Draft comment:
    Changing valid_at from datetime | None to datetime removes the possibility of None. Ensure this aligns with the intended logic, as it may cause issues if None is a valid state.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment highlights a change that could affect the logic of the application if 'None' was previously a valid state for 'valid_at'. This is a valid concern as it could lead to runtime errors if the code expects 'valid_at' to sometimes be 'None'.
    The comment could be seen as speculative since it assumes that 'None' might have been a valid state without confirming it. However, it is a valid point to consider when changing a field's type.
    While the comment could be seen as speculative, it is important to consider the implications of changing a field's type, especially if it could lead to runtime errors.
    Keep the comment as it highlights a potential issue with the change in the field's type that could affect the logic of the application.
2. core/search/search_utils.py:45
  • Draft comment:
    Ensure record['created_at'] is always a neo4j_time.DateTime object. If it can be None, use safely_parse_db_date for safety.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is speculative and asks for verification, which is against the rules. The code assumes record['created_at'] is a neo4j_time.DateTime object, and the change aligns with this assumption. The comment does not point out a definite issue but rather suggests a precautionary measure, which is not actionable or necessary given the context.
    I might be overlooking a scenario where record['created_at'] could indeed be None, but the code change does not introduce this possibility. The comment does not address a definite issue in the code.
    The code change does not introduce a scenario where record['created_at'] could be None, and the comment does not address a definite issue. It is speculative and not actionable.
    Remove the comment as it is speculative and not actionable, and the code change does not introduce a definite issue.
3. core/search/search_utils.py:174
  • Draft comment:
    Ensure record['created_at'] is always a neo4j_time.DateTime object. If it can be None, use safely_parse_db_date for safety.
  • Reason this comment was not posted:
    Marked as duplicate.
4. core/search/search_utils.py:209
  • Draft comment:
    Ensure record['created_at'] is always a neo4j_time.DateTime object. If it can be None, use safely_parse_db_date for safety.
  • Reason this comment was not posted:
    Marked as duplicate.
5. core/utils/bulk_utils.py:43
  • Draft comment:
    Ensure episode.valid_at is always set. If it can be None, this change might cause issues.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is speculative and asks the author to ensure a condition, which is against the rules. It does not point out a definite issue but rather suggests a potential one. The rules clearly state not to make speculative comments or ask the author to ensure something.
    The comment might be pointing out a potential issue that could cause a bug if episode.valid_at is None. However, the rules are clear about not making speculative comments.
    Even if the comment points out a potential issue, it is speculative and asks the author to ensure something, which is against the rules.
    The comment should be removed as it is speculative and asks the author to ensure a condition, which violates the rules.

Workflow ID: wflow_oNdXLY6pHrQb73E8


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 c6e7938 in 20 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. core/utils/search/search_utils.py:105
  • Draft comment:
    Consider using safely_parse_db_date for created_at to ensure consistent date parsing and handle potential parsing issues.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is suggesting a change that the author has already considered and decided against. The author likely has a reason for not using safely_parse_db_date for created_at, such as the data already being in the correct format. The comment does not provide new information or a clear reason why the change is necessary.
    I might be missing the context of why the author made the change. There could be a valid reason for not using safely_parse_db_date, such as performance considerations or data format guarantees.
    The author likely has a reason for their change, and the comment does not provide a compelling reason to revert it. Without additional context, the comment seems unnecessary.
    The comment should be removed as it suggests a change that the author has already considered and decided against, without providing a compelling reason for the change.

Workflow ID: wflow_nNAQGm79o0FuyLHY


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 089a7ec in 20 seconds

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

Workflow ID: wflow_xCE1scHmaQh7XA2i


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.

@prasmussen15 prasmussen15 self-requested a review August 23, 2024 15:14
@@ -35,7 +34,7 @@ async def extract_new_edges(
llm_client: LLMClient,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should actually just remove this function instead, I deprecated wit the architecture change of splitting two prompts

@prasmussen15 prasmussen15 merged commit 9cc9883 into main Aug 23, 2024
5 checks passed
@prasmussen15 prasmussen15 deleted the chore/fix-simple-typing-issues branch August 23, 2024 15:15
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.

3 participants