Skip to content

fix(testing+linting): add nox lint+format directives #123

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

Merged
merged 2 commits into from
Jan 30, 2025
Merged

fix(testing+linting): add nox lint+format directives #123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
68616cd
fix(testing+linting): add nox lint+format directives
odeke-em Dec 26, 2024
e1b166d
Rebased from main and re-ran nox -s format/lint
odeke-em Jan 30, 2025
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 105 additions & 4 deletions noxfile.py
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove these changes. This project (and consistently across other projects) does not use nox to run lints. Please see https://github.com/googleapis/langchain-google-spanner-python/blob/main/.github/workflows/lint.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @averikitsch, Do we have a reason for not using it?
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • The dependencies are in the pyproject.toml that get updated by dependabot. This file does not get automatically updated
  • This config is copied from repos that use a different package naming schema, different directory structure, and setup.py

Also, for the future we should not be using "fix" in the commit message, that will trigger a new release and we do not create new releases for CI updates.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@averikitsch thanks for your reply. The purpose of the PR is to get functionality to allow code to be formatted, unit tests to be run locally et al as code is being built by developers so as to speed up development. Do you perhaps have alternative suggestions for how to accomplish the above?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please remove these changes. This project (and consistently across other projects) does not use nox to run lints.

@averikitsch python-spanner the main repo representing Cloud Spanner does use nox https://github.com/googleapis/python-spanner/blob/main/noxfile.py for linting, testing and all, for more than 7 years https://github.com/googleapis/python-spanner/commits/main/noxfile.py?after=32e761b0d4052938bf67cfec63a0e83702a35ada+34.

Could you perhaps explain your objections and help out with suggestions? This PR has been sitting for more than 2 weeks and it moving is necessary to accomplish other tasks.
Copy link
Contributor

@gauravpurohit06 gauravpurohit06 Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @averikitsch,

@odeke-em is trying to define a nox task for liniting and running the test locally and for linting he is using black (i.e. we are using same in our workflows).

If noxfile.py is not getting copied from any other repos or configurations.. I don't see any problem with the change.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will be diverting from the other langchain repos. I am fine with adding it, but the Spanner team will have to keep it up to date as I mentioned that it doesn't get auto-updates and will not stay in sync with the repo CI setup. This may cause long term confusion for maintainers.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @averikitsch. It won't need many updates except indeed as you mention mirror-ing pyproject dependencies into setup.py and those won't be updates needed even with a year. However, the benefits of having non-Googlers be able to develop and make updates robustly I believe shall heavily outweigh any of those. Also currently I can't even look at the Kokoro statuses as one needs to be a Googler per https://console.cloud.google.com/cloud-build/triggers/edit/16ba25ff-d74a-42ef-8f19-9780571439ab?project=585680705374
Screenshot 2025-01-19 at 10 15 12 PM
Copy link
Collaborator

Choose a reason for hiding this comment

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

Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,28 @@
import pathlib
import shutil
from pathlib import Path
from typing import Optional

import nox

DEFAULT_PYTHON_VERSION = "3.10"
CURRENT_DIRECTORY = pathlib.Path(__file__).parent.absolute()
LINT_PATHS = ["src", "tests", "noxfile.py"]


nox.options.sessions = [
"docs",
"blacken",
"docfx",
"docs",
"format",
"lint",
"unit",
]

# Error if a python version is missing
nox.options.error_on_missing_interpreters = True


@nox.session(python="3.10")
@nox.session(python=DEFAULT_PYTHON_VERSION)
def docs(session):
"""Build the docs for this library."""

Expand Down Expand Up @@ -71,7 +76,7 @@ def docs(session):
)


@nox.session(python="3.10")
@nox.session(python=DEFAULT_PYTHON_VERSION)
def docfx(session):
"""Build the docfx yaml files for this library."""

Expand Down Expand Up @@ -115,3 +120,99 @@ def docfx(session):
os.path.join("docs", ""),
os.path.join("docs", "_build", "html", ""),
)


@nox.session(python=DEFAULT_PYTHON_VERSION)
def lint(session):
"""Run linters.

Returns a failure if the linters find linting errors or
sufficiently serious code quality issues.
"""
session.install(".[lint]")
session.run(
"black",
"--check",
*LINT_PATHS,
)
session.run("flake8", "src", "tests")


@nox.session(python=DEFAULT_PYTHON_VERSION)
def lint_setup_py(session):
"""Verify that setup.py is valid (including an RST check)."""
session.install("docutils", "pygments")
session.run("python", "setup.py", "check", "--restructuredtext", "--strict")


@nox.session(python=DEFAULT_PYTHON_VERSION)
def blacken(session):
session.install(".[lint]")
session.run(
"black",
*LINT_PATHS,
)


@nox.session(python=DEFAULT_PYTHON_VERSION)
def format(session):
session.install(".[lint]")
# Sort imports in strict alphabetical order.
session.run(
"isort",
*LINT_PATHS,
)
session.run(
"black",
*LINT_PATHS,
)


@nox.session(python=DEFAULT_PYTHON_VERSION)
def unit(session):
install_unittest_dependencies(session)
session.run(
"py.test",
"--quiet",
os.path.join("tests", "unit"),
*session.posargs,
)


def install_unittest_dependencies(session, *constraints):
session.install(".[test]")
session.run(
"pip",
"install",
"--no-compile", # To ensure no byte recompliation which is usually super slow
"-q",
"--disable-pip-version-check", # Avoid the slow version check
".",
"-r",
"requirements.txt",
)


@nox.session(python=DEFAULT_PYTHON_VERSION)
def integration(session):
install_integrationtest_dependencies(session)
session.run(
"py.test",
"--quiet",
os.path.join("tests", "integration"),
*session.posargs,
)


def install_integrationtest_dependencies(session):
session.install(".[test]")
session.run(
"pip",
"install",
"--no-compile", # To ensure no byte recompliation which is usually super slow
"-q",
"--disable-pip-version-check", # Avoid the slow version check
".",
"-r",
"requirements.txt",
)
7 changes: 7 additions & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,16 @@ Repository = "https://github.com/googleapis/langchain-google-spanner-python.git"
Changelog = "https://github.com/googleapis/langchain-google-spanner-python/blob/main/CHANGELOG.md"

[project.optional-dependencies]
lint = [
"black[jupyter]==24.8.0",
"flake8==6.1.0",
"isort==5.13.2",
]

test = [
"black[jupyter]==24.8.0",
"bs4==0.0.2",
"flake8==6.1.0",
"isort==5.13.2",
"mypy==1.11.2",
"pytest==8.3.3",
Expand Down
1 change: 0 additions & 1 deletion src/langchain_google_spanner/graph_qa.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,6 @@ def _call(
inputs: Dict[str, Any],
run_manager: Optional[CallbackManagerForChainRun] = None,
) -> Dict[str, str]:

intermediate_steps: List = []

"""Generate gql statement, uses it to look up in db and answer question."""
Expand Down
6 changes: 1 addition & 5 deletions src/langchain_google_spanner/graph_retriever.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,7 @@
from typing import Any, List, Optional

from langchain.schema.retriever import BaseRetriever
from langchain_community.graphs.graph_document import GraphDocument
from langchain_core.callbacks import (
CallbackManagerForChainRun,
CallbackManagerForRetrieverRun,
)
from langchain_core.callbacks import CallbackManagerForRetrieverRun
from langchain_core.documents import Document
from langchain_core.embeddings import Embeddings
from langchain_core.example_selectors import SemanticSimilarityExampleSelector
Expand Down
1 change: 0 additions & 1 deletion src/langchain_google_spanner/loader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

from google.cloud.spanner import Client, KeySet # type: ignore
from google.cloud.spanner_admin_database_v1.types import DatabaseDialect # type: ignore
from google.cloud.spanner_v1.data_types import JsonObject # type: ignore
from langchain_community.document_loaders.base import BaseLoader
from langchain_core.documents import Document

Expand Down
1 change: 0 additions & 1 deletion tests/integration/test_spanner_graph_qa.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,6 @@ def load_data(graph: SpannerGraphStore):


class TestSpannerGraphQAChain:

@pytest.fixture(scope="module")
def setup_db_load_data(self):
graph = get_spanner_graph()
Expand Down
12 changes: 6 additions & 6 deletions tests/integration/test_spanner_loader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
import uuid

import pytest
from google.cloud.spanner import Client, KeySet # type: ignore
from google.cloud.spanner import Client # type: ignore
from langchain_core.documents import Document

from langchain_google_spanner.loader import Column, SpannerDocumentSaver, SpannerLoader
Expand Down Expand Up @@ -363,7 +363,7 @@ def test_loader_custom_format_error(self, client):
client=client,
format="NOT_A_FORMAT",
)
docs = loader.load()
docs = loader.load() # noqa

def test_loader_custom_content_key_error(self, client):
query = f"SELECT * FROM {table_name}"
Expand All @@ -375,7 +375,7 @@ def test_loader_custom_content_key_error(self, client):
client=client,
content_columns=["NOT_A_COLUMN"],
)
docs = loader.load()
docs = loader.load() # noqa

def test_loader_custom_metadata_key_error(self, client):
query = f"SELECT * FROM {table_name}"
Expand All @@ -387,7 +387,7 @@ def test_loader_custom_metadata_key_error(self, client):
client=client,
metadata_columns=["NOT_A_COLUMN"],
)
docs = loader.load()
docs = loader.load() # noqa

def test_loader_custom_json_metadata(self, client):
database = client.instance(instance_id).database(google_database)
Expand Down Expand Up @@ -792,7 +792,7 @@ def test_loader_custom_content_key_error(self, client):
client=client,
content_columns=["NOT_A_COLUMN"],
)
docs = loader.load()
docs = loader.load() # noqa

def test_loader_custom_metadata_key_error(self, client):
query = f"SELECT * FROM {table_name}"
Expand All @@ -804,7 +804,7 @@ def test_loader_custom_metadata_key_error(self, client):
client=client,
metadata_columns=["NOT_A_COLUMN"],
)
docs = loader.load()
docs = loader.load() # noqa

def test_loader_custom_json_metadata(self, client):
database = client.instance(instance_id).database(pg_database)
Expand Down
4 changes: 2 additions & 2 deletions tests/integration/test_spanner_vector_store.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ def test_spanner_vector_delete_data(self, setup_database):

deleted = db.delete(documents=[docs[0], docs[1]])

assert deleted == True
assert deleted

def test_spanner_vector_search_data1(self, setup_database):
loader, embeddings = setup_database
Expand Down Expand Up @@ -483,7 +483,7 @@ def test_spanner_vector_delete_data(self, setup_database):

deleted = db.delete(documents=[docs[0], docs[1]])

assert deleted == True
assert deleted

def test_spanner_vector_search_data1(self, setup_database):
loader, embeddings = setup_database
Expand Down