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

Distributing typing information (#127) #135

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

justin13601
Copy link
Owner

@justin13601 justin13601 commented Sep 26, 2024

Summary by CodeRabbit

  • New Features

    • Introduced new data aggregation functions for enhanced data processing.
    • Added configuration classes for managing predicates and temporal windows.
    • New utility functions for logging and time parsing.
  • Bug Fixes

    • Updated function signatures for clarity and simplicity.
  • Documentation

    • Enhanced documentation for new functions and classes to improve usability.

Copy link
Contributor

coderabbitai bot commented Sep 26, 2024

Walkthrough

The changes introduced in this pull request encompass the addition of new configuration settings in the pyproject.toml file and the creation of several new .pyi files. These files define various functions and data classes related to data aggregation, configuration management, constraint checking, and utility functions for logging and time parsing. The modifications enhance the overall structure and functionality of the codebase, particularly in the context of data processing and event handling.

Changes

Files Change Summary
pyproject.toml Added configurations to include package data and specify data files for the package.
src/aces/*.pyi Introduced multiple new files defining functions and classes for data aggregation, configuration management, constraint checking, and utilities. Each file includes method signatures and data class definitions relevant to their respective functionalities.
src/aces/utils.py Updated function signatures by removing type hints for parse_timedelta and hydra_loguru_init.
src/aces/utils.pyi Added utility functions and context managers, including parse_timedelta, capture_output, log_tree, and hydra_loguru_init, with appropriate type annotations.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Config
    participant Aggregator
    participant Constraints
    participant Utils

    User->>Config: Load configuration
    Config->>Aggregator: Provide aggregation settings
    User->>Aggregator: Call aggregate functions
    Aggregator->>Utils: Log aggregation process
    User->>Constraints: Check constraints on data
    Constraints->>Utils: Log constraint checks
Loading

Poem

In the garden of code, new blooms arise,
With functions and classes reaching for the skies.
A rabbit hops by, with a joyful cheer,
Celebrating changes that bring us near.
Data flows freely, like a stream in the sun,
Together we code, and together we run! 🐇✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@justin13601
Copy link
Owner Author

Should typing information be included inline and also with these stubs @mmcdermott?

Copy link

codecov bot commented Sep 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines Coverage Δ
src/aces/utils.py 96.42% <100.00%> (ø)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 10

🧹 Outside diff range and nitpick comments (7)
src/aces/query.pyi (1)

8-8: LGTM! Consider adding a docstring for better documentation.

The function signature is well-defined with clear type hints. However, to improve the documentation, consider adding a docstring to describe the purpose of the function, its parameters, and the return value.

Here's a suggested docstring to add above the function signature:

def query(cfg: TaskExtractorConfig, predicates_df: pl.DataFrame) -> pl.DataFrame:
    """
    Query and process data based on the given configuration and predicates.

    Args:
        cfg (TaskExtractorConfig): Configuration for task extraction.
        predicates_df (pl.DataFrame): DataFrame containing predicates for querying.

    Returns:
        pl.DataFrame: Resulting DataFrame after applying the query and processing.
    """
    ...
src/aces/extract_subtree.pyi (1)

6-11: LGTM: Well-structured function signature with clear type annotations.

The function signature is well-defined with appropriate type annotations, which enhances maintainability and IDE support. The parameter names are descriptive, improving readability.

Consider adding a docstring to provide more context about the function's purpose and parameter descriptions. This would further improve the usability of the stub file. For example:

def extract_subtree(
    subtree: Node,
    subtree_anchor_realizations: pl.DataFrame,
    predicates_df: pl.DataFrame,
    subtree_root_offset: timedelta = timedelta(0),
) -> pl.DataFrame:
    """
    Extract a subtree based on the provided parameters.

    Args:
        subtree (Node): The subtree to extract.
        subtree_anchor_realizations (pl.DataFrame): DataFrame containing anchor realizations.
        predicates_df (pl.DataFrame): DataFrame containing predicates.
        subtree_root_offset (timedelta, optional): Offset for the subtree root. Defaults to timedelta(0).

    Returns:
        pl.DataFrame: The extracted subtree data.
    """
    ...
src/aces/utils.pyi (1)

16-16: Consider a more descriptive name for log_tree function.

While the log_tree function signature is clear, the function name could be more descriptive. Consider renaming it to something like log_node_tree or log_tree_structure to better convey its purpose.

src/aces/utils.py (1)

Line range hint 1-70: Reconsider the approach to distributing typing information.

The removal of type hints from these functions seems to be part of the PR objective "Distributing typing information". However, this approach may not be ideal for several reasons:

  1. It reduces code clarity and self-documentation in the main implementation file.
  2. It may lead to inconsistencies between the implementation and any separate type definitions.
  3. It goes against the trend in the Python community towards more type annotations in code.

Instead of removing type hints from the main implementation, consider:

  1. Keeping type hints in the main implementation file for clarity and immediate understanding.
  2. If the goal is to provide separate type definitions, consider using stub files (.pyi) in addition to, not instead of, in-line type hints. This allows for more detailed or alternative type definitions without sacrificing the benefits of in-line hints.
  3. Ensure that any separate type definitions (if used) are consistent with the in-line hints and are kept up-to-date with changes in the implementation.

This approach would maintain code quality in the main implementation while still allowing for the distribution of additional typing information if needed.

src/aces/predicates.pyi (1)

1-7: Organize import statements for improved readability.

  • Group imports in the order: standard library imports, third-party imports, local imports.
  • Include necessary imports from the typing module due to added type hints.
  • Remove any unused imports to keep the code clean.

Apply this diff to reorganize and update imports:

 from pathlib import Path

+from typing import Any, Dict, List, Optional

 import polars as pl
 from omegaconf import DictConfig

 from .config import TaskExtractorConfig
src/aces/config.pyi (2)

31-31: Confirm the necessity of the empty __post_init__ method in DerivedPredicateConfig.

The __post_init__ method is declared but contains ..., indicating no current implementation. If it's not performing any initialization, it might be unnecessary.

Consider removing the __post_init__ method if it's not needed, or provide an implementation if future initialization steps are anticipated.


90-95: Simplify type annotations in the load method of TaskExtractorConfig.

In the load method, the class type TaskExtractorConfig is quoted in the type annotations. Since we're within the class definition, and forward references are not needed here, the quotes can be omitted.

Update the type annotations for clarity:

-        cls: Type["TaskExtractorConfig"],
+        cls: Type[TaskExtractorConfig],
...
-    ) -> "TaskExtractorConfig": ...
+    ) -> TaskExtractorConfig: ...

This makes the annotations more straightforward and avoids unnecessary use of forward references.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between e826a7b and b6f6b09.

📒 Files selected for processing (10)
  • pyproject.toml (1 hunks)
  • src/aces/aggregate.pyi (1 hunks)
  • src/aces/config.pyi (1 hunks)
  • src/aces/constraints.pyi (1 hunks)
  • src/aces/extract_subtree.pyi (1 hunks)
  • src/aces/predicates.pyi (1 hunks)
  • src/aces/query.pyi (1 hunks)
  • src/aces/types.pyi (1 hunks)
  • src/aces/utils.py (2 hunks)
  • src/aces/utils.pyi (1 hunks)
🔇 Additional comments not posted (26)
src/aces/query.pyi (2)

1-8: Overall, the type stub file is well-structured and provides clear type hints.

The file successfully defines the type information for the query function, which appears to be a key component in a task extraction process. The use of Polars DataFrames suggests efficient data handling. To further improve the file:

  1. Verify the usage of imported modules as suggested earlier.
  2. Consider adding a docstring to the query function for better documentation.

These minor improvements will enhance the overall quality and maintainability of the code.


1-6: Verify the usage of imported modules

The following imports are not directly used in this type stub file:

  1. Optional from typing
  2. Node from bigtree

While they might be used in the implementation file, it's good practice to keep imports minimal in stub files.

Please run the following script to check if these imports are used in the corresponding implementation file:

If these imports are not used in the implementation file, consider removing them from the stub file.

✅ Verification successful

Unused imports confirmed

The imports Optional from typing and Node from bigtree are not used in either the stub file src/aces/query.pyi or the implementation file src/aces/query.py. It is recommended to remove these imports from the stub file to keep the codebase clean and maintainable.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if unused imports in the stub file are used in the implementation file

# Test: Search for usage of Optional and Node in the implementation file
implementation_file=$(echo "src/aces/query.pyi" | sed 's/\.pyi$/.py/')
echo "Checking usage in $implementation_file"
rg --type python "Optional|Node" "$implementation_file"

Length of output: 254


Script:

#!/bin/bash
# Description: Verify if 'Optional' and 'Node' are used in the implementation file

implementation_file="src/aces/query.py"

if [ -f "$implementation_file" ]; then
    echo "Checking usage of 'Optional' and 'Node' in $implementation_file"
    rg "Optional|Node" "$implementation_file"
else
    echo "Implementation file $implementation_file does not exist."
    echo "Please ensure that the implementation file is present."
fi

Length of output: 252

src/aces/extract_subtree.pyi (2)

1-4: LGTM: Imports are appropriate and well-structured.

The imports are relevant to the function being defined:

  • timedelta is used in the function signature.
  • polars (as pl) is used for DataFrame operations.
  • Node from bigtree is used for tree-like data structures.

These choices suggest a focus on performance and appropriate data structures for the task at hand.


1-11: Overall, excellent stub file implementation.

This stub file is well-structured and provides clear type information for the extract_subtree function. The use of type annotations, appropriate imports, and a well-defined function signature contributes to improved maintainability and developer experience. The only suggested improvement is the addition of a docstring for enhanced documentation.

src/aces/constraints.pyi (5)

6-8: LGTM! Well-defined function signature.

The check_constraints function signature is well-typed and clearly defines its input parameters and return type. The use of type hints is appropriate and will aid in type checking and code understanding.


9-9: LGTM! Well-defined function signature.

The check_static_variables function signature is well-typed and clearly defines its input parameters and return type. The use of type hints is appropriate and will aid in type checking and code understanding.


1-9: Overall, the constraints.pyi file looks good.

This file provides clear type hints for the check_constraints and check_static_variables functions, which will improve type checking and code understanding. The function signatures are well-defined and use appropriate type hints.

A few suggestions for improvement:

  1. Verify the usage of the logger import from the loguru library.
  2. Confirm the usage of the ANY_EVENT_COLUMN import from the local .types module.

If these imports are not used in the actual implementation, consider removing them to keep the type hints file concise and focused.


4-4: LGTM! Verify ANY_EVENT_COLUMN usage.

The import from a local module is good for maintaining a modular structure.

However, ANY_EVENT_COLUMN is not visibly used in this file. Let's verify its usage:

#!/bin/bash
# Description: Verify the usage of ANY_EVENT_COLUMN in the constraints module.

# Test: Search for ANY_EVENT_COLUMN usage in the constraints module
rg --type python 'ANY_EVENT_COLUMN' src/aces/constraints.py

If the script doesn't return any results, consider removing the unused import.


1-2: LGTM! Verify logger usage.

The imports look good. The Polars library is correctly imported and used in the function signatures.

However, the logger import is not visibly used in this file. Let's verify its usage:

If the script doesn't return any results, consider removing the unused import.

src/aces/utils.pyi (5)

1-11: LGTM: Import statements are well-organized and appropriate.

The import statements are properly structured, separating standard library imports from third-party imports. They cover all the necessary modules for the utility functions declared in this file.


13-13: LGTM: parse_timedelta function signature is well-defined.

The function signature for parse_timedelta is clear and uses appropriate type hints. It correctly handles an optional input and returns a timedelta object.


14-15: LGTM: capture_output context manager is well-defined.

The capture_output context manager is correctly decorated and has an appropriate return type hint for a generator that yields a StringIO object.


17-17: LGTM: hydra_loguru_init function signature is well-defined.

The hydra_loguru_init function signature is clear and uses appropriate type hints. It correctly specifies a filename input and indicates that it doesn't return a value.


1-17: Overall, the utils.pyi stub file is well-structured and provides clear type information.

This stub file effectively defines the type information for the utility functions in the aces.utils module. The use of type hints and the structure of the file are appropriate for a .pyi stub file. The only suggestion is to consider renaming the log_tree function for better clarity, but this is a minor improvement.

pyproject.toml (3)

32-33: LGTM: Enabling package data inclusion

The addition of include-package-data = true in the [tool.setuptools] section is correct and aligns with the PR objective. This configuration ensures that package data, including typing information files, will be included when building the package.


32-36: Summary: Proper configuration for distributing typing information

The changes in this file correctly set up the project for distributing typing information:

  1. Enabling include-package-data ensures all package data is included during build.
  2. Specifying *.pyi and py.typed files as package data ensures type information is distributed.

These changes align well with the PR objective and follow best practices for Python packaging. Once the package name is verified and corrected if necessary, these modifications will effectively enable the distribution of typing information with your package.


35-36: Verify the package name and approve type information inclusion

The inclusion of *.pyi and py.typed files as package data is correct and aligns with the PR objective of distributing typing information. This ensures that type hint stub files and the marker file for type checking support are included in the package.

However, please verify if your_package is the correct package name. It appears to be a placeholder and should be replaced with the actual package name (likely "es-aces" based on the project name).

To confirm the correct package name, you can run the following command:

If the result doesn't match "your_package", please update the package name accordingly.

✅ Verification successful

Package name verified and type information inclusion approved

  • Replace your_package with es-aces in the [tool.setuptools.package-data] section.
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correct package name in pyproject.toml

# Test: Search for the package name in pyproject.toml
rg --type toml 'name\s*=\s*"([^"]+)"' pyproject.toml

Length of output: 194

src/aces/aggregate.pyi (1)

7-10: 🛠️ Refactor suggestion

Ensure type annotations are compatible with supported Python versions

The use of the | operator for type unions (e.g., TemporalWindowBounds | tuple[...] and timedelta | None) requires Python 3.10 or later. If the project needs to support earlier Python versions, consider using Union and Optional from the typing module to maintain compatibility.

You can verify the project's Python version compatibility by checking the pyproject.toml or setup.py. If earlier versions need to be supported, apply this diff:

+from typing import Union, Optional

-def aggregate_temporal_window(
-    predicates_df: pl.DataFrame,
-    endpoint_expr: TemporalWindowBounds | tuple[bool, timedelta, bool, timedelta | None],
-) -> pl.DataFrame: ...
+def aggregate_temporal_window(
+    predicates_df: pl.DataFrame,
+    endpoint_expr: Union[TemporalWindowBounds, tuple[bool, timedelta, bool, Optional[timedelta]]],
+) -> pl.DataFrame: ...

Run the following script to check the Python version specified in pyproject.toml:

src/aces/predicates.pyi (1)

20-20: Confirm the accessibility of TaskExtractorConfig.

The function get_predicates_df uses TaskExtractorConfig as a parameter type. Verify that TaskExtractorConfig is properly defined and accessible from .config. This ensures that type checking and code navigation work correctly.

src/aces/types.pyi (4)

6-11: Global variables are declared appropriately.

The global variables PRED_CNT_TYPE, START_OF_RECORD_KEY, END_OF_RECORD_KEY, and ANY_EVENT_COLUMN are correctly declared with appropriate type annotations.


12-24: TemporalWindowBounds data class is well-defined.

The TemporalWindowBounds class and its methods are correctly declared with appropriate type annotations and method signatures.


25-36: ToEventWindowBounds data class is well-defined.

The ToEventWindowBounds class and its methods are correctly declared with appropriate type annotations and method signatures.


1-5: ⚠️ Potential issue

Add missing import for dataclasses.

The dataclasses module is used but not imported. Please add the import statement for dataclasses to ensure the decorators function correctly.

Apply this diff to fix the issue:

 from datetime import timedelta
+import dataclasses
 from typing import Any, Iterator

 import polars as pl

Likely invalid or redundant comment.

src/aces/config.pyi (3)

51-61: Check return types of property methods in WindowConfig.

The property methods start_endpoint_expr and end_endpoint_expr return Optional[Union[ToEventWindowBounds, TemporalWindowBounds]]. Ensure that these types are correctly defined and imported.

Confirm that ToEventWindowBounds and TemporalWindowBounds are properly defined:

#!/bin/bash
# Description: Verify the definitions of ToEventWindowBounds and TemporalWindowBounds.

# Search for the type definitions in the codebase.
rg --type py --files-with-matches 'class ToEventWindowBounds'
rg --type py --files-with-matches 'class TemporalWindowBounds'

45-48: Verify the implementations of _check_reference and _parse_boundary methods in WindowConfig.

The class methods _check_reference and _parse_boundary are declared with ..., suggesting their implementations should be provided elsewhere.

Ensure these methods are implemented in the codebase:

#!/bin/bash
# Description: Verify the existence of method implementations for _check_reference and _parse_boundary.

# Search for the implementations in `.py` files excluding stub files.
rg --type py --glob '!*.pyi' -l 'def _check_reference'
rg --type py --glob '!*.pyi' -l 'def _parse_boundary'

20-21: Ensure implementations of MEDS_eval_expr and ESGPT_eval_expr exist.

The methods MEDS_eval_expr and ESGPT_eval_expr in PlainPredicateConfig are declared with ..., indicating that their implementations should exist elsewhere.

Run the following script to confirm that these methods are implemented in the codebase:

✅ Verification successful

Method Implementations Confirmed

The implementations of MEDS_eval_expr and ESGPT_eval_expr exist in src/aces/config.py.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the existence of method implementations for MEDS_eval_expr and ESGPT_eval_expr.

# Search for the implementations in `.py` files excluding stub files.
rg --type py --glob '!*.pyi' -l 'def MEDS_eval_expr'
rg --type py --glob '!*.pyi' -l 'def ESGPT_eval_expr'

Length of output: 258


Script:

#!/bin/bash
# Description: Verify the existence of method implementations for MEDS_eval_expr and ESGPT_eval_expr.

# Search for the implementation of MEDS_eval_expr in `.py` files excluding stub files.
rg --type py --glob '!*.pyi' -l 'def MEDS_eval_expr'

# Search for the implementation of ESGPT_eval_expr in `.py` files excluding stub files.
rg --type py --glob '!*.pyi' -l 'def ESGPT_eval_expr'

Length of output: 148

@@ -67,7 +67,7 @@ def log_tree(node):
logger.info("\n" + captured.getvalue()) # Log the captured output


def hydra_loguru_init(filename) -> None:
def hydra_loguru_init(filename):
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider retaining return type hint and adding parameter type hint.

The removal of the return type hint from the hydra_loguru_init function may reduce code clarity and self-documentation. Additionally, the filename parameter lacks a type hint.

Consider reverting the change and adding a type hint for the filename parameter:

-def hydra_loguru_init(filename):
+def hydra_loguru_init(filename: str) -> None:

This change:

  1. Maintains code quality and clarity.
  2. Explicitly indicates that the function doesn't return a value.
  3. Adds type information for the filename parameter, improving code documentation.
  4. Enables better tooling support (IDE suggestions, linter warnings, etc.).
  5. Aligns with Python's trend towards more type annotations (PEP 484).
📝 Committable 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. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def hydra_loguru_init(filename):
def hydra_loguru_init(filename: str) -> None:

@@ -10,7 +10,7 @@
from pytimeparse import parse


def parse_timedelta(time_str: str) -> timedelta:
def parse_timedelta(time_str):
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider retaining type hints for improved code quality and consistency.

The removal of type hints from the parse_timedelta function signature may have unintended consequences:

  1. It reduces code clarity and self-documentation.
  2. It may lead to runtime errors if incorrect types are passed.
  3. It loses the benefit of static type checking.
  4. It's inconsistent with Python's trend towards more type annotations (PEP 484).
  5. The docstring still mentions parameter and return types, which is now inconsistent with the signature.

Consider reverting the change and keeping the type hints:

-def parse_timedelta(time_str):
+def parse_timedelta(time_str: str) -> timedelta:

This maintains code quality, consistency with the docstring, and enables better tooling support (IDE suggestions, linter warnings, etc.).

📝 Committable 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. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def parse_timedelta(time_str):
def parse_timedelta(time_str: str) -> timedelta:

src/aces/aggregate.pyi Show resolved Hide resolved
src/aces/predicates.pyi Show resolved Hide resolved
src/aces/predicates.pyi Show resolved Hide resolved
src/aces/predicates.pyi Show resolved Hide resolved
src/aces/predicates.pyi Show resolved Hide resolved
Comment on lines +67 to +82
class PlainPredicateConfig:
pass

class DerivedPredicateConfig:
input_predicates: Any

class WindowConfig:
referenced_predicates: set[str]
label: Optional[str]
index_timestamp: Optional[str]
start_endpoint_expr: Optional[str]
end_endpoint_expr: Optional[str]
has: dict[str, Any]
root_node: str
referenced_event: list[str]

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid duplicating class definitions for PlainPredicateConfig, DerivedPredicateConfig, and WindowConfig.

Classes PlainPredicateConfig, DerivedPredicateConfig, and WindowConfig are defined again starting at line 67. This duplication can cause confusion and potential type checking issues.

Consider removing the duplicate class definitions to prevent conflicts:

# Assuming these classes are defined elsewhere
class EventConfig:
    predicate: str
-class PlainPredicateConfig:
-    pass
-
-class DerivedPredicateConfig:
-    input_predicates: Any
-
-class WindowConfig:
-    referenced_predicates: set[str]
-    label: Optional[str]
-    index_timestamp: Optional[str]
-    start_endpoint_expr: Optional[str]
-    end_endpoint_expr: Optional[str]
-    has: dict[str, Any]
-    root_node: str
-    referenced_event: list[str]

If these classes are meant to be placeholders or interfaces, consider renaming them or ensuring they are only defined once.

📝 Committable 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. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
class PlainPredicateConfig:
pass
class DerivedPredicateConfig:
input_predicates: Any
class WindowConfig:
referenced_predicates: set[str]
label: Optional[str]
index_timestamp: Optional[str]
start_endpoint_expr: Optional[str]
end_endpoint_expr: Optional[str]
has: dict[str, Any]
root_node: str
referenced_event: list[str]
class EventConfig:
predicate: str

Comment on lines +83 to +88
class TaskExtractorConfig:
predicates: Dict[str, Union[PlainPredicateConfig, DerivedPredicateConfig]]
trigger: EventConfig
windows: Optional[Dict[str, WindowConfig]]
label_window: Optional[str]
index_timestamp_window: Optional[str]
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 26, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Provide default values for optional fields in TaskExtractorConfig.

The fields windows, label_window, and index_timestamp_window are annotated as optional but don't have default values. This might lead to TypeError if they're not supplied during instantiation.

Assign default values to optional fields:

-    windows: Optional[Dict[str, WindowConfig]]
+    windows: Optional[Dict[str, WindowConfig]] = None
-    label_window: Optional[str]
+    label_window: Optional[str] = None
-    index_timestamp_window: Optional[str]
+    index_timestamp_window: Optional[str] = None

This ensures that instances of TaskExtractorConfig can be created without explicitly providing these optional parameters.

📝 Committable 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. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
class TaskExtractorConfig:
predicates: Dict[str, Union[PlainPredicateConfig, DerivedPredicateConfig]]
trigger: EventConfig
windows: Optional[Dict[str, WindowConfig]]
label_window: Optional[str]
index_timestamp_window: Optional[str]
class TaskExtractorConfig:
predicates: Dict[str, Union[PlainPredicateConfig, DerivedPredicateConfig]]
trigger: EventConfig
windows: Optional[Dict[str, WindowConfig]] = None
label_window: Optional[str] = None
index_timestamp_window: Optional[str] = None

Copy link
Collaborator

Choose a reason for hiding this comment

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

@justin13601, this comment should also likely apply to other optional parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

src/aces/config.pyi Show resolved Hide resolved
Copy link
Collaborator

@mmcdermott mmcdermott left a comment

Choose a reason for hiding this comment

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

@justin13601 this looks good to me, though i don't know how the .pyi files usually work / should be formatted. Note that any remaining, non-resolved coderabbit comments are things that I agree with and recommend should be addressed before you push the PR.

Comment on lines +83 to +88
class TaskExtractorConfig:
predicates: Dict[str, Union[PlainPredicateConfig, DerivedPredicateConfig]]
trigger: EventConfig
windows: Optional[Dict[str, WindowConfig]]
label_window: Optional[str]
index_timestamp_window: Optional[str]
Copy link
Collaborator

Choose a reason for hiding this comment

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

@justin13601, this comment should also likely apply to other optional parameters.

@mmcdermott
Copy link
Collaborator

Should typing information be included inline and also with these stubs @mmcdermott?

Yes, I think so, but google around or check the google style guide to see if there are other recommendations in the community.

@mmcdermott
Copy link
Collaborator

@justin13601 can we merge or close this? Not sure which would be more appropriate, but I'd like to get it closed out rather than remaining indefinitely open. Thanks!

@justin13601
Copy link
Owner Author

@justin13601 can we merge or close this? Not sure which would be more appropriate, but I'd like to get it closed out rather than remaining indefinitely open. Thanks!

Yep will try to merge this weekend!

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