Skip to content

core: Add ruff rules SLF #30666

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 4 commits into from
May 14, 2025
Merged

core: Add ruff rules SLF #30666

merged 4 commits into from
May 14, 2025

Conversation

cbornet
Copy link
Collaborator

@cbornet cbornet commented Apr 4, 2025

Copy link

vercel bot commented Apr 4, 2025

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 May 14, 2025 6:40pm

@cbornet cbornet requested a review from eyurtsev April 4, 2025 11:46
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. 🤖:nit Small modifications/deletions, fixes, deps or improvements to existing code or docs labels Apr 4, 2025
@@ -350,8 +350,8 @@ def _should_stream(
**kwargs: Any,
) -> bool:
"""Determine if a given model call should hit the streaming API."""
sync_not_implemented = type(self)._stream == BaseChatModel._stream
async_not_implemented = type(self)._astream == BaseChatModel._astream
sync_not_implemented = type(self)._stream == BaseChatModel._stream # noqa: SLF001
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -49,8 +49,8 @@ def log_error_once(method: str, exception: Exception) -> None:

def wait_for_all_tracers() -> None:
"""Wait for all tracers to finish."""
if rt._CLIENT is not None:
rt._CLIENT.flush()
if rt._CLIENT is not None: # noqa: SLF001
Copy link
Collaborator Author

@cbornet cbornet Apr 4, 2025

Choose a reason for hiding this comment

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

There's probably something to fix in LangSmith regarding the visibility of the cached client.
Maybe add a def cached_client_exists() -> bool method ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, this would make sense.

@eyurtsev eyurtsev self-assigned this Apr 4, 2025
@cbornet cbornet force-pushed the ruff-core-slf branch 4 times, most recently from 7ef9664 to 566457b Compare April 11, 2025 22:24
Copy link
Collaborator

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

Looking good, just a few nit picks. Thanks so much!

if rt._CLIENT is not None:
rt._CLIENT.flush()
if rt._CLIENT is not None: # noqa: SLF001
get_client().flush()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that this is just a ruff rules PR, can we keep the logic as is and implement logical changes in a new PR?

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: see #30912

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#30912 has been merged and this PR has been rebased.

Comment on lines 166 to 173
class _EdgeViewer:
def __init__(self) -> None:
self.pts: list[tuple[float]] = []

def setpath(self, pts: list[tuple[float]]) -> None:
self.pts = pts


Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as below - happy to implement a logical change here, but let's not do it in a PR where we're just adding a ruff rule.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#30912 has been merged and this PR has been rebased.

@cbornet cbornet requested a review from sydney-runkle April 17, 2025 21:26
eyurtsev added a commit that referenced this pull request May 12, 2025
See #30666

---------

Co-authored-by: Eugene Yurtsev <[email protected]>
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels May 13, 2025
@cbornet
Copy link
Collaborator Author

cbornet commented May 13, 2025

@sydney-runkle the PR now only adds noqa where needed. PTAL.

@dosubot dosubot bot added the lgtm PR looks good. Use to confirm that a PR is ready for merging. label May 14, 2025
@@ -123,8 +123,8 @@ def _start_trace(self, run: Run) -> None:
run.tags = self.tags.copy()

super()._start_trace(run)
if run._client is None:
run._client = self.client # type: ignore[misc]
if run.ls_client is None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe that this breaks with current constraints on the langsmith client. ls_client was available as of 0.1.126. Bumping the requirement

Copy link
Collaborator

Choose a reason for hiding this comment

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

Pushed a PR to update the min constraint and updated the uv lock file

@eyurtsev eyurtsev enabled auto-merge (squash) May 14, 2025 18:40
@eyurtsev eyurtsev merged commit 921573e into langchain-ai:master May 14, 2025
58 checks passed
@cbornet cbornet deleted the ruff-core-slf branch May 14, 2025 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm PR looks good. Use to confirm that a PR is ready for merging. 🤖:nit Small modifications/deletions, fixes, deps or improvements to existing code or docs size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants