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.

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

all: Add pre-commit hook #26993

Merged
merged 5 commits into from
Dec 18, 2024
Merged

all: Add pre-commit hook #26993

merged 5 commits into from
Dec 18, 2024

Conversation

cbornet
Copy link
Collaborator

@cbornet cbornet commented Sep 30, 2024

This calls make format on projects that have modified files.
So poetry install --with lint must have been done for those projects.

@efriis efriis added the partner label Sep 30, 2024
@efriis efriis self-assigned this Sep 30, 2024
Copy link

vercel bot commented Sep 30, 2024

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 Dec 18, 2024 10:22pm

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Sep 30, 2024
@cbornet cbornet force-pushed the pre-commit-hook branch 4 times, most recently from 38fa568 to 5445261 Compare September 30, 2024 14:37
@cbornet cbornet marked this pull request as draft September 30, 2024 17:22
@cbornet cbornet marked this pull request as ready for review October 1, 2024 10:45
Copy link
Member

@efriis efriis left a comment

Choose a reason for hiding this comment

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

I'm lightly against this and open to being convinced. In general, I feel like encouraging small commits (even with lint/format errors) is good, even if it means running make format at the end.

Should we instead document how to add it for people who want it?

@cbornet
Copy link
Collaborator Author

cbornet commented Oct 4, 2024

Using the pre commit hook is opt-in.
You have to install it with pre-commit install.
So it’s up to the user taste to use it or not.

@efriis
Copy link
Member

efriis commented Oct 7, 2024

in that case could you

  1. move it into the scripts/ directory (to not clutter the root)
  2. add to contributing code documentation an optional step using pre-commit install --config configs/.pre-commit-config.yaml?

@cbornet
Copy link
Collaborator Author

cbornet commented Oct 8, 2024

move it into the scripts/ directory (to not clutter the root)

OK but that makes it less discoverable. People used to pre-commit that would see the file in the root directly know they can use it. Otherwise they have to read the setup doc which I believe most of the time they won't do (I didn't do it when starting contributing to LangChain, I saw a poetry.lock file in the root and so I ran poetry install. I also saw a Makefile in the root and knew there would be interesting stuff there)

@efriis
Copy link
Member

efriis commented Oct 21, 2024

given the lack of engagement from folks here or in issues/discussions on precommit hooks I'm going to close this - it increases maintenance surface area (e.g. devcontainers and docker stuff that is stale now)

happy to look into it again as part of a broader contributor experience sprint. agreed figuring out linting/formatting could be easier. don't think hardcoding some of the libraries like this is the way.

@efriis efriis closed this Oct 21, 2024
@efriis efriis reopened this Dec 11, 2024
@efriis
Copy link
Member

efriis commented Dec 11, 2024

have seen enough engagement here in the last few weeks that it's probably worth it. Some interesting learnings though

  • make doesn't work on windows machines - is there a way we can offer some different compatibility via the precommit hook
  • is there a way to have the hooks "discovered" for each of the libs/partners dirs as well

Copy link
Member

@efriis efriis left a comment

Choose a reason for hiding this comment

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

is there a way we can include the partners/ directories as well? Ideally in an auto-detected manner - ideally would extend to the main libs too. Not sure if this is possible in precommit / how much additional scripting would be needed. If helpful I could try to make the script at check_diff.py work for checking for directory changes in the same way we do in CI

name: format standard-tests
language: system
entry: make -C libs/standard-tests format
files: ^libs/standard-tests/(langchain_standard_tests|tests)/
Copy link
Member

Choose a reason for hiding this comment

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

might be worth adding the whole directory because changes to pyproject.toml/poetry.lock/makefile could make changes too, and this is what we detect in CI

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK. I'll update

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.

@cbornet
Copy link
Collaborator Author

cbornet commented Dec 17, 2024

is there a way we can include the partners/ directories as well?

Yes, I'll add them.

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Dec 17, 2024
@cbornet
Copy link
Collaborator Author

cbornet commented Dec 17, 2024

is there a way we can include the partners/ directories as well?

Done

@cbornet cbornet requested a review from efriis December 17, 2024 14:26
@efriis efriis enabled auto-merge (squash) December 18, 2024 22:22
@efriis efriis merged commit 1e88ada into langchain-ai:master Dec 18, 2024
12 checks passed
@cbornet cbornet deleted the pre-commit-hook branch December 19, 2024 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
partner size:L This PR changes 100-499 lines, ignoring generated files.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants