Skip to content

chore(langchain): add ruff rules N #32098

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
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

chore(langchain): add ruff rules N #32098

wants to merge 2 commits into from
+40 −70

Conversation

cbornet
Copy link
Collaborator

@cbornet cbornet commented Jul 18, 2025 •
edited
Loading

@vercel Vercel
Copy link

vercel bot commented Jul 18, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Visit Preview Jul 27, 2025 5:35pm

@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Jul 18, 2025 •
edited
Loading

CodSpeed WallTime Performance Report

Merging #32098 will not alter performance

Comparing cbornet:langchain-ruff-n (606e63f) with master (efdfa00)

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

Summary

✅ 13 untouched benchmarks

@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Jul 18, 2025 •
edited
Loading

CodSpeed Instrumentation Performance Report

Merging #32098 will not alter performance

Comparing cbornet:langchain-ruff-n (606e63f) with master (efdfa00)

Summary

✅ 14 untouched benchmarks

@cbornet cbornet force-pushed the langchain-ruff-n branch from 187a642 to 1e48184 Compare July 22, 2025 08:35
@mdrxy mdrxy added the linting Linting or formatting label Jul 22, 2025
@cbornet cbornet force-pushed the langchain-ruff-n branch from 1e48184 to a6cfb9f Compare July 23, 2025 09:08
eyurtsev
eyurtsev reviewed Jul 23, 2025
View reviewed changes
libs/langchain/langchain/agents/agent_toolkits/csv/__init__.py

def __getattr__(name: str) -> Any:
"""Get attr name."""

if name == "create_csv_agent":
# Get directory of langchain package
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think there was an edge case here which is why i did it this way rather

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change replaces the dynamic determination of the package names with a static one.
I don't see how it could be wrong.
It even fixes the target path as the dynamic way resolves to langchain_experimental.agents.agent_toolkits.csv. create_csv_agent which doesn't exist.

eyurtsev
eyurtsev reviewed Jul 23, 2025
View reviewed changes
libs/langchain/tests/unit_tests/stubs.py Outdated
@@ -20,28 +20,28 @@ def __eq__(self, other: object) -> bool:
# subclassed strings.


def _AnyIdDocument(**kwargs: Any) -> Document:
def any_id_document(**kwargs: Any) -> Document:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer to keep these as they were as they stand in for replacement classes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about using real classes then ?
Something like

class _AnyIDMixin(BaseModel):
    def __eq__(self, other):
        if isinstance(other, BaseModel):
            dump = self.model_dump()
            dump.pop("id")
            other_dump = other.model_dump()
            other_dump.pop("id")
            return dump == other_dump
        return False

class AnyIdAiMessage(AIMessage, _AnyIDMixin):
    pass
    
class AnyIdAiMessageChunk(AIMessageChunk, _AnyIDMixin):
    pass

class AnyIdHumanMessage(HumanMessage, _AnyIDMixin):
    pass

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I opened #32209 .
LMK what you think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done.

@eyurtsev eyurtsev changed the title feat(langchain): add ruff rules N chore(langchain): add ruff rules N Jul 23, 2025
@cbornet cbornet force-pushed the langchain-ruff-n branch from a6cfb9f to 7ab006e Compare July 23, 2025 15:01
@cbornet cbornet mentioned this pull request Jul 23, 2025
Merged
eyurtsev pushed a commit that referenced this pull request Jul 24, 2025
@cbornet
refactor(langchain): refactor unit test stub classes (#32209)
0b34be4 
See
#32098 (comment)
@cbornet cbornet force-pushed the langchain-ruff-n branch from 08408b3 to 861027e Compare July 25, 2025 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eyurtsev eyurtsev eyurtsev left review comments

Assignees
No one assigned
Labels
linting Linting or formatting
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cbornet @eyurtsev @mdrxy