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.

Sign up for GitHub

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

Jump to bottom

Remove "type: ignore" comments #17048

Closed
1 task done
baskaryan opened this issue Feb 5, 2024 · 4 comments · Fixed by #19931
Closed
1 task done

Remove "type: ignore" comments #17048

baskaryan opened this issue Feb 5, 2024 · 4 comments · Fixed by #19931
Labels
good first issue Good for newcomers help wanted Good issue for contributors 🤖:refactor A large refactor of a feature(s) or restructuring of many files

Comments

@baskaryan
Copy link
Collaborator

Privileged issue

  • I am a LangChain maintainer, or was asked directly by a LangChain maintainer to create an issue here.

Issue Content

Currently we have many "type: ignore" comments in our code for ignoring mypy errors. We should rarely if ever use these. We've had to recently add a bunch of these ignore comments because of an error (i made) that was silently causing mypy not to run in CI.

We should work to remove as many of them as possible by fixing the underlying issues. To find the you can just grep for:

git grep "type: ignore" libs/

This is a big effort, even just removing a few at a time would be very helpful.
@baskaryan baskaryan added help wanted Good issue for contributors good first issue Good for newcomers labels Feb 5, 2024
@langchain-ai langchain-ai deleted a comment from dosubot bot Feb 5, 2024
@dosubot dosubot bot added Ɑ: langserve Related to LangServe package 🤖:refactor A large refactor of a feature(s) or restructuring of many files labels Feb 5, 2024
@baskaryan baskaryan mentioned this issue Feb 5, 2024
Merged
@baskaryan baskaryan removed the Ɑ: langserve Related to LangServe package label Feb 5, 2024
baskaryan added a commit that referenced this issue Feb 5, 2024
@baskaryan
community[patch]: fix agent_toolkits mypy (#17050)
e7b3290 
Related to #17048
@baskaryan baskaryan mentioned this issue Feb 5, 2024
Merged
baskaryan added a commit that referenced this issue Feb 5, 2024
@baskaryan
community[patch]: callbacks mypy fixes (#17058)
af5ae24 
Related to #17048
@baskaryan baskaryan mentioned this issue Feb 5, 2024
Merged
baskaryan added a commit that referenced this issue Feb 5, 2024
@baskaryan
community[patch]: chat message history mypy fixes (#17059)
d93de71 
Related to #17048
@baskaryan baskaryan mentioned this issue Feb 5, 2024
Merged
baskaryan added a commit that referenced this issue Feb 5, 2024
@baskaryan
community[patch]: chat model mypy fixes (#17061)
66e45e8 
Related to #17048
moorej-oci added a commit to moorej-oci/langchain that referenced this issue Feb 5, 2024
@moorej-oci
core[patch]: fix _sql_record_manager mypy for langchain-ai#17048
a9fedb5
@moorej-oci moorej-oci mentioned this issue Feb 5, 2024
Merged
baskaryan pushed a commit that referenced this issue Feb 6, 2024
@moorej-oci
core[patch]: fix _sql_record_manager mypy for #17048 (#17073)
912210a 
- **Description:** Add relevant type annotations for relevant session
and query objects to resolve mypy errors when `# type: ignore` comments
are removed.
  - **Issue:** #17048
  - **Dependencies:** None,
  - **Twitter handle:** [clesiemo3](https://twitter.com/clesiemo3)
 
I attempted to solve the `UpsertionRecord` ignore but it would require
added a deprecated plugin or moving completely to sqlalchemy 2.0+ from
my understanding. I'm assuming this is not something desired at this
point in time.
@ali-nobariasl
Copy link

Hello, I am a new contributor, I could handle this issue if it is still open.

@dosubot DosuBot
Copy link

dosubot bot commented Feb 6, 2024 •
edited
Loading

🤖

I understand your concern about the "type: ignore" comments in the LangChain codebase. These comments are indeed used to suppress type checking errors in the Python static type checker, mypy. However, they should be used sparingly and only as a last resort when other solutions to resolve the type checking errors are not feasible.

Here are the specific "type: ignore" comments currently present in the LangChain codebase:

  1. wrapped = obj.__init__ # type: ignore
  2. obj.__init__ = functools.wraps(obj.__init__)( # type: ignore[misc] warn_if_direct_instance )
  3. class _deprecated_property(type(obj)): # type: ignore
  4. _name = _name or obj.__name__ # type: ignore
  5. def finalize(_: Any, new_doc: str) -> Any: # type: ignore
  6. def finalize( # type: ignore wrapper: Callable[..., Any], new_doc: str ) -> T:

These comments are found in the libs/core/langchain_core/_api/deprecation.py file.

Unfortunately, I don't have the specific mypy error messages that these "type: ignore" comments are suppressing. It would be helpful to know these error messages to understand the underlying issues that need to be fixed.

As for the best practices for handling mypy errors in Python projects, they include using the mypy tool for type checking, configuring mypy options in the pyproject.toml file, excluding certain directories from mypy checks, using type annotations in your Python code, and handling mypy errors by fixing the type annotations in your code. You can find more details about these best practices in the libs/community/pyproject.toml file.

I hope this information helps. If you have more specific information about the mypy errors, please share it so we can provide a more targeted solution.

Sources

This response is meant to be useful and save you time. It is not meant to be a precise solution, but rather a starting point for your own research.

Help me be more useful! Please leave a 👍 if this is helpful and 👎 if it is irrelevant.

If you want to continue the conversation, start your reply with @dosu-bot.

This was referenced Feb 9, 2024
Closed
Merged
baskaryan pushed a commit that referenced this issue Feb 13, 2024
@h0rv @eyurtsev
community[patch]: doc loaders mypy fixes (#17368)
ece4b43 
**Description:** Fixed `type: ignore`'s for mypy for some
document_loaders.
**Issue:** [Remove "type: ignore" comments #17048
](#17048)

---------

Co-authored-by: Robby <[email protected]>
Co-authored-by: Eugene Yurtsev <[email protected]>
@baskaryan baskaryan pinned this issue Feb 14, 2024
@baskaryan baskaryan unpinned this issue Feb 14, 2024
@Lord-Haji Lord-Haji mentioned this issue Feb 28, 2024
Merged
@DanielChico DanielChico mentioned this issue Mar 1, 2024
Merged
baskaryan pushed a commit that referenced this issue Mar 1, 2024
@DanielChico
community[patch]: type ignore fixes (#18395)
7d96227 
Related to #17048
baskaryan pushed a commit that referenced this issue Mar 1, 2024
@Lord-Haji
community[patch]: chat message histrory mypy fix (#18250)
e2b901c 
Description: Fixed type: ignore's for mypy for
chat_message_histories(streamlit)
Adresses #17048 

Planning to add more based on reviews
@Smit-Parmar Smit-Parmar mentioned this issue Mar 4, 2024
Merged
@Lord-Haji Lord-Haji mentioned this issue Mar 4, 2024
Closed
@baskaryan
Copy link
Collaborator Author

We're bumping all our packages to use mypy v1 which is uncovering a bunch of issues that we're type: ignore-ing to start so we're not blocked on the upgrade, but which should fix as well. See #19163 for example

@Smit-Parmar
Copy link
Contributor

@baskaryan Is that mean we no longer need to work on this issue?

baskaryan pushed a commit that referenced this issue Mar 28, 2024
@Smit-Parmar
community[patch]: mypy ignore fix (#18483)
dfc4177 
Relates to #17048 
Description : Applied fix to dynamodb and elasticsearch file.

Error was : `Cannot override writeable attribute with read-only
property`
Suggestion:
instead of adding 
```
@messages.setter
def messages(self, messages: List[BaseMessage]) -> None:
    raise NotImplementedError("Use add_messages instead")
```

we can change base class property
`messages: List[BaseMessage]`
to
```
@Property
def messages(self) -> List[BaseMessage]:...
```

then we don't need to add `@messages.setter` in all child classes.
gkorland pushed a commit to FalkorDB/langchain that referenced this issue Mar 30, 2024
@DanielChico @gkorland
community[patch]: type ignore fixes (langchain-ai#18395)
2c84e4f 
Related to langchain-ai#17048
gkorland pushed a commit to FalkorDB/langchain that referenced this issue Mar 30, 2024
@Lord-Haji @gkorland
community[patch]: chat message histrory mypy fix (langchain-ai#18250)
f2074b6 
Description: Fixed type: ignore's for mypy for
chat_message_histories(streamlit)
Adresses langchain-ai#17048 

Planning to add more based on reviews
gkorland pushed a commit to FalkorDB/langchain that referenced this issue Mar 30, 2024
@Smit-Parmar @gkorland
community[patch]: mypy ignore fix (langchain-ai#18483)
ef2b3cd 
Relates to langchain-ai#17048 
Description : Applied fix to dynamodb and elasticsearch file.

Error was : `Cannot override writeable attribute with read-only
property`
Suggestion:
instead of adding 
```
@messages.setter
def messages(self, messages: List[BaseMessage]) -> None:
    raise NotImplementedError("Use add_messages instead")
```

we can change base class property
`messages: List[BaseMessage]`
to
```
@Property
def messages(self) -> List[BaseMessage]:...
```

then we don't need to add `@messages.setter` in all child classes.
@dontnet-wuenze dontnet-wuenze mentioned this issue Apr 2, 2024
Closed
UtkarshaGupte added a commit to UtkarshaGupte/langchain that referenced this issue Apr 2, 2024
@UtkarshaGupte
core/langchain_core/_api[Patch]: mypy ignore fixes langchain-ai#17048
936afde 
core/langchain_core/_api[Patch]: mypy ignore fixes langchain-ai#17048
@UtkarshaGupte UtkarshaGupte mentioned this issue Apr 2, 2024
Merged
eyurtsev pushed a commit that referenced this issue Apr 4, 2024
@UtkarshaGupte
core[patch]: mypy ignore fixes #17048 (#19931)
b27f81c 
core/langchain_core/_api[Patch]: mypy ignore fixes #17048
Related to #17048

Applied mypy fixes to below two files:
libs/core/langchain_core/_api/deprecation.py
libs/core/langchain_core/_api/beta_decorator.py

Summary of Fixes:
**Issue 1**
class _deprecated_property(type(obj)): # type: ignore
error: Unsupported dynamic base class "type"  [misc]
Fix: 
1. Added an __init__ method to _deprecated_property to initialize the
fget, fset, fdel, and __doc__ attributes.
2. In the __get__, __set__, and __delete__ methods, we now use the
self.fget, self.fset, and self.fdel attributes to call the original
methods after emitting the warning.

3. The finalize function now creates an instance of _deprecated_property
with the fget, fset, fdel, and doc attributes from the original obj
property.



**Issue 2**



 def finalize(  # type: ignore
                wrapper: Callable[..., Any], new_doc: str
            ) -> T:


error: All conditional function variants must have identical
signatures



Fix:
Ensured that both definitions of the finalize function have the
same signature

Twitter Handle -
https://x.com/gupteutkarsha?s=11&t=uwHe4C3PPpGRvoO5Qpm1aA
@dontnet-wuenze dontnet-wuenze mentioned this issue Apr 7, 2024
Merged
hinthornw pushed a commit that referenced this issue Apr 26, 2024
@Smit-Parmar @hinthornw
community[patch]: mypy ignore fix (#18483)
0193124 
Relates to #17048 
Description : Applied fix to dynamodb and elasticsearch file.

Error was : `Cannot override writeable attribute with read-only
property`
Suggestion:
instead of adding 
```
@messages.setter
def messages(self, messages: List[BaseMessage]) -> None:
    raise NotImplementedError("Use add_messages instead")
```

we can change base class property
`messages: List[BaseMessage]`
to
```
@Property
def messages(self) -> List[BaseMessage]:...
```

then we don't need to add `@messages.setter` in all child classes.
hinthornw pushed a commit that referenced this issue Apr 26, 2024
@UtkarshaGupte @hinthornw
core[patch]: mypy ignore fixes #17048 (#19931)
d7bab16 
core/langchain_core/_api[Patch]: mypy ignore fixes #17048
Related to #17048

Applied mypy fixes to below two files:
libs/core/langchain_core/_api/deprecation.py
libs/core/langchain_core/_api/beta_decorator.py

Summary of Fixes:
**Issue 1**
class _deprecated_property(type(obj)): # type: ignore
error: Unsupported dynamic base class "type"  [misc]
Fix: 
1. Added an __init__ method to _deprecated_property to initialize the
fget, fset, fdel, and __doc__ attributes.
2. In the __get__, __set__, and __delete__ methods, we now use the
self.fget, self.fset, and self.fdel attributes to call the original
methods after emitting the warning.

3. The finalize function now creates an instance of _deprecated_property
with the fget, fset, fdel, and doc attributes from the original obj
property.



**Issue 2**



 def finalize(  # type: ignore
                wrapper: Callable[..., Any], new_doc: str
            ) -> T:


error: All conditional function variants must have identical
signatures



Fix:
Ensured that both definitions of the finalize function have the
same signature

Twitter Handle -
https://x.com/gupteutkarsha?s=11&t=uwHe4C3PPpGRvoO5Qpm1aA
ccurme added a commit that referenced this issue May 6, 2024
@dontnet-wuenze @baskaryan @ccurme
community[patch]: chat message history mypy fixes #17048 (#20114)
32c61b3 
Relates [#17048]
Description : Applied fix to redis and neo4j file.

Error was : `Cannot override writeable attribute with read-only
property`

fix with the same solution of
[[langchain/libs/community/langchain_community/chat_message_histories/elasticsearch.py](https://github.com/langchain-ai/langchain/blob/d5c412b0a9a8af02754de60f93d93c3efa33ccdb/libs/community/langchain_community/chat_message_histories/elasticsearch.py#L170-L175)]

---------

Co-authored-by: Bagatur <[email protected]>
Co-authored-by: Chester Curme <[email protected]>
dglogo pushed a commit to dglogo/langchain that referenced this issue May 8, 2024
@dontnet-wuenze @baskaryan
@ccurme @dglogo
community[patch]: chat message history mypy fixes langchain-ai#17048 (l…
4cfd041 
…angchain-ai#20114)

Relates [langchain-ai#17048]
Description : Applied fix to redis and neo4j file.

Error was : `Cannot override writeable attribute with read-only
property`

fix with the same solution of
[[langchain/libs/community/langchain_community/chat_message_histories/elasticsearch.py](https://github.com/langchain-ai/langchain/blob/d5c412b0a9a8af02754de60f93d93c3efa33ccdb/libs/community/langchain_community/chat_message_histories/elasticsearch.py#L170-L175)]

---------

Co-authored-by: Bagatur <[email protected]>
Co-authored-by: Chester Curme <[email protected]>
kyle-cassidy pushed a commit to kyle-cassidy/langchain that referenced this issue May 10, 2024
@dontnet-wuenze @baskaryan
@ccurme @kyle-cassidy
community[patch]: chat message history mypy fixes langchain-ai#17048 (l…
c0ebad5 
…angchain-ai#20114)

Relates [langchain-ai#17048]
Description : Applied fix to redis and neo4j file.

Error was : `Cannot override writeable attribute with read-only
property`

fix with the same solution of
[[langchain/libs/community/langchain_community/chat_message_histories/elasticsearch.py](https://github.com/langchain-ai/langchain/blob/d5c412b0a9a8af02754de60f93d93c3efa33ccdb/libs/community/langchain_community/chat_message_histories/elasticsearch.py#L170-L175)]

---------

Co-authored-by: Bagatur <[email protected]>
Co-authored-by: Chester Curme <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Good for newcomers help wanted Good issue for contributors 🤖:refactor A large refactor of a feature(s) or restructuring of many files
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

core[Patch]: mypy ignore fixes #17048 UtkarshaGupte/langchain
3 participants
@baskaryan @ali-nobariasl @Smit-Parmar