Skip to content

found a case where jedi's full name did not start with the module nam… #124

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

Merged
merged 2 commits into from
Apr 4, 2025

Conversation

alvin-r
Copy link
Contributor

@alvin-r alvin-r commented Apr 4, 2025

User description

…e. we need this check so it doesn't error out


PR Type

  • Enhancement

Description

  • Add debugging print using code_print

  • Check that full function names start with module names

  • Ensure proper function identification using Jedi API


Changes walkthrough 📝

Relevant files
Enhancement
code_context_extractor.py
Add logging and module prefix checks                                         

codeflash/context/code_context_extractor.py

  • Import and call code_print for extra logging
  • Ensure name.full_name starts with name.module_name in function
    extraction
  • Validate definition's module prefix in Jedi function sources
  • +4/-1     
    function_context.py
    Update function qualified check for module prefix               

    codeflash/optimization/function_context.py

  • Modify qualified function check to confirm module name prefix
  • Update condition in belongs_to_function_qualified for enhanced safety
  • +1/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • …e. we need this check so it doesn't error out
    @alvin-r alvin-r requested a review from KRRT7 April 4, 2025 21:14
    @@ -73,6 +73,7 @@ def get_code_optimization_context(

    # Handle token limits
    tokenizer = tiktoken.encoding_for_model("gpt-4o")
    code_print(final_read_writable_code)
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    do we want to print this to the output?

    Copy link

    github-actions bot commented Apr 4, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Debug Print

    The added call to code_print may be used for debugging purposes only. Verify that it is removed or properly gated in production to avoid unwanted console output.

    code_print(final_read_writable_code)
    Name Validation

    The PR adds conditions to ensure that full names start with module names. Confirm these checks are robust and handle edge cases consistently, especially as similar validations are introduced in multiple locations.

                and name.full_name.startswith(name.module_name)
                and get_qualified_name(name.module_name, name.full_name) == function_to_optimize.qualified_name
            ):
                function_source = FunctionSource(
                    file_path=function_to_optimize.file_path,
                    qualified_name=function_to_optimize.qualified_name,
                    fully_qualified_name=name.full_name,
                    only_function_name=name.name,
                    source_code=name.get_line_code(),
                    jedi_definition=name,
                )
                return function_source
    
        raise ValueError(
            f"Could not find function {function_to_optimize.function_name} in {function_to_optimize.file_path}"
        )
    
    
    def get_function_sources_from_jedi(
        file_path_to_qualified_function_names: dict[Path, set[str]], project_root_path: Path
    ) -> tuple[dict[Path, set[FunctionSource]], list[FunctionSource]]:
        file_path_to_function_source = defaultdict(set)
        function_source_list: list[FunctionSource] = []
        for file_path, qualified_function_names in file_path_to_qualified_function_names.items():
            script = jedi.Script(path=file_path, project=jedi.Project(path=project_root_path))
            file_refs = script.get_names(all_scopes=True, definitions=False, references=True)
    
            for qualified_function_name in qualified_function_names:
                names = [
                    ref
                    for ref in file_refs
                    if ref.full_name and belongs_to_function_qualified(ref, qualified_function_name)
                ]
                for name in names:
                    try:
                        definitions: list[Name] = name.goto(follow_imports=True, follow_builtin_imports=False)
                    except Exception as e:
                        try:
                            logger.exception(f"Error while getting definition for {name.full_name}: {e}")
                        except Exception as e:
                            # name.full_name can also throw exceptions sometimes
                            logger.exception(f"Error while getting definition: {e}")
                        definitions = []
                    if definitions:
                        # TODO: there can be multiple definitions, see how to handle such cases
                        definition = definitions[0]
                        definition_path = definition.module_path
    
                        # The definition is part of this project and not defined within the original function
                        if (
                            str(definition_path).startswith(str(project_root_path) + os.sep)
                            and not path_belongs_to_site_packages(definition_path)
                            and definition.full_name
                            and definition.type == "function"
                            and not belongs_to_function_qualified(definition, qualified_function_name)
                            and definition.full_name.startswith(definition.module_name)
    Name Validation

    A new condition using startswith has been added in the function qualification. Ensure this logic is aligned with intended behavior and correctly handles all input variations.

    if name.full_name.startswith(name.module_name) and get_qualified_name(name.module_name, name.full_name) == qualified_function_name:

    Copy link

    github-actions bot commented Apr 4, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Security
    guard sensitive output

    Conditionally print code to avoid exposing potentially sensitive content.

    codeflash/context/code_context_extractor.py [76]

    -code_print(final_read_writable_code)
    +if DEBUG_MODE:
    +    code_print(final_read_writable_code)
    Suggestion importance[1-10]: 5

    __

    Why: While conditionally printing to avoid exposing sensitive code can be beneficial in certain contexts, the change is context‐specific and not clearly required from the diff, reducing its overall impact.

    Low
    Possible issue
    validate module presence

    Verify that name.module_name is non-empty to ensure the startswith check works as
    expected.

    codeflash/context/code_context_extractor.py [360]

    -and name.full_name.startswith(name.module_name)
    +and name.module_name and name.full_name.startswith(name.module_name)
    Suggestion importance[1-10]: 3

    __

    Why: Adding a check for a non-empty module name can help avoid potential edge cases, although the underlying behavior of startswith with an empty string is well-defined, making this a minor improvement.

    Low
    check non-empty module

    Ensure that definition.module_name is not empty before applying startswith to avoid
    incorrect matches.

    codeflash/context/code_context_extractor.py [415]

    -and definition.full_name.startswith(definition.module_name)
    +and definition.module_name and definition.full_name.startswith(definition.module_name)
    Suggestion importance[1-10]: 3

    __

    Why: Similar to suggestion 2, the added check guards against empty module names before using startswith, but since empty strings are valid inputs, the change is only a slight safeguard.

    Low
    ensure module value

    Add a non-empty check for name.module_name to ensure the startswith condition is
    valid.

    codeflash/optimization/function_context.py [34]

    -if name.full_name.startswith(name.module_name) and get_qualified_name(name.module_name, name.full_name) == qualified_function_name:
    +if name.module_name and name.full_name.startswith(name.module_name) and get_qualified_name(name.module_name, name.full_name) == qualified_function_name:
    Suggestion importance[1-10]: 3

    __

    Why: The recommendation to check for a non-empty module name before the startswith condition offers a minor improvement for edge cases, but its overall impact is limited.

    Low

    @alvin-r alvin-r merged commit 8819383 into main Apr 4, 2025
    15 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants