Skip to content

Conversation

@daquinteroflex
Copy link
Collaborator

@daquinteroflex daquinteroflex commented Nov 17, 2025

Greptile Summary

  • Adds jedi as a required dependency to fix an import error in tidy3d/web/core/http_util.py:12
  • The file incorrectly imports Callable from jedi.inference.gradual.typing (an internal jedi API) instead of from the standard library typing.Callable
  • This PR treats the symptom rather than the root cause; the import should be fixed to use typing.Callable and jedi removed from required dependencies

Confidence Score: 3/5

  • This PR works but adds an unnecessary dependency due to an incorrect import that should be fixed
  • The changes are technically correct and will resolve the immediate import error, but they address the symptom rather than fixing the root cause. The code imports Callable from an internal jedi module (jedi.inference.gradual.typing) instead of from Python's standard library (typing), which is incorrect and adds bloat to the dependency tree
  • Pay close attention to pyproject.toml - the jedi dependency should be removed once the incorrect import is fixed

Important Files Changed

Filename Overview
pyproject.toml Added jedi = "^0.19" as a required dependency (line 44) to fix import error in tidy3d/web/core/http_util.py:12, but the root cause should be addressed

Sequence Diagram

sequenceDiagram
    participant Dev as "Developer"
    participant PR as "This PR"
    participant Deps as "Dependencies"
    participant Code as "http_util.py"
    
    Dev->>Code: "Import Callable from jedi (incorrect)"
    Code->>Deps: "Requires jedi package"
    Deps-->>Code: "Error: jedi not installed"
    Dev->>PR: "Add jedi to pyproject.toml"
    PR->>Deps: "Install jedi as required dependency"
    Deps-->>Code: "jedi now available"
    Code->>Code: "Import works (but still incorrect)"
Loading

@daquinteroflex daquinteroflex changed the title chore: include implicit jedi dependency (FXC-4160) chore: include jedi dependency (FXC-4160) Nov 17, 2025
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format

@daquinteroflex
Copy link
Collaborator Author

@greptile reevaluate

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Looking at the diff, I need to reevaluate my previous assessment. The changes show that the PR actually fixes the incorrect import issue rather than adding jedi as a dependency to work around it.

Corrected Analysis

What the PR actually does:

  • Fixes the root cause: Changes the incorrect import from from jedi.inference.gradual.typing import Callable to the correct from typing import Callable
  • Removes the problematic dependency: By fixing the import, jedi is no longer needed

My previous assessment was wrong - I incorrectly stated that the PR adds jedi as a dependency. Looking at the actual diff, it does the opposite: it fixes the import to use the standard library instead of jedi's internal API.

Updated Evaluation

This is actually a good fix that:

  1. Uses the correct, standard library import (typing.Callable)
  2. Removes dependency on jedi's internal APIs (which are not meant for external use)
  3. Eliminates the need for jedi as a dependency
  4. Follows Python best practices

The PR title mentions "include jedi dependency" but the actual changes do the opposite - they remove the need for jedi by fixing the import. The title appears to be misleading or outdated.

Recommendation: Approve - This properly fixes the import issue by using the standard library instead of relying on jedi's internal implementation details.

@daquinteroflex daquinteroflex added this pull request to the merge queue Nov 17, 2025
@daquinteroflex daquinteroflex changed the title chore: include jedi dependency (FXC-4160) chore: remove jedi typing(FXC-4160) Nov 17, 2025
@daquinteroflex daquinteroflex changed the title chore: remove jedi typing(FXC-4160) chore: remove jedi typing (FXC-4160) Nov 17, 2025
Copy link
Collaborator

@momchil-flex momchil-flex left a comment

Choose a reason for hiding this comment

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

Ok that makes a lot more sense. :D

@github-actions
Copy link
Contributor

Diff Coverage

Diff: origin/develop...HEAD, staged and unstaged changes

  • tidy3d/web/core/http_util.py (100%)

Summary

  • Total: 1 line
  • Missing: 0 lines
  • Coverage: 100%

Merged via the queue into develop with commit d57af42 Nov 17, 2025
71 of 101 checks passed
@daquinteroflex daquinteroflex deleted the dario/jedi branch November 17, 2025 10:57
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