ramalama icon indicating copy to clipboard operation
ramalama copied to clipboard
verified publisher icon containers

adds dev dependencies and pre-commit hooks

Open ieaves opened this issue 8 months ago • 11 comments

Additions

  • adds pre-commit hooks to automatically run isort and black on git commit. This can also be implemented as a CI action if there was interest.
  • Expands the dev dependency group to include all dev requirements.

Notes

Some of the dev dependencies listed in the Makefile are incompatible with python3.8, the required versions were decremented to maintain compatibility but tell me if that's the wrong decision.

Questions

These are really just ideas to help improve developer experience. If any of this isn't desirable that's totally okay!

It looked like pytest-cov and a few assorted dependencies were references in the Makefile as well -- would those belong in the dev group or even a cov group?

Summary by Sourcery

Group dev dependencies under pyproject.toml’s dev extras, simplify installation via the Makefile, and introduce pre-commit hooks for automatic code formatting.

Enhancements:

  • Consolidate all development dependencies into a single ‘dev’ extras group and update the Makefile to install them via pipx using “.[dev]”.
  • Add a pre-commit configuration to run black and isort on commits for consistent code formatting.
  • Pin dev dependency versions to maintain compatibility with Python 3.8.

ieaves avatar May 12 '25 20:05 ieaves

Reviewer's Guide

This PR streamlines the development setup by consolidating Makefile dev installs into a single pipx command, expanding the optional dev dependencies in pyproject.toml with pinned versions for compatibility, and adding a pre-commit configuration to automatically run black and isort on commits.

File-Level Changes

Change Details Files
Consolidate dev dependency installation in Makefile
  • replaced explicit pipx install calls with a single pipx install "[.dev]" command
 Makefile
Expand dev dependencies list in pyproject.toml
  • updated pytest version to ~8.3
  • added isort, codespell, flake8, huggingface-hub, black, wheel, and pre-commit entries
 pyproject.toml
Introduce pre-commit configuration
  • created .pre-commit-config.yaml
  • configured black and isort hooks scoped to the ramalama directory
 .pre-commit-config.yaml
Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an issue from a review comment by replying to it. You can also reply to a review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull request title to generate a title at any time. You can also comment @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in the pull request body to generate a PR summary at any time exactly where you want it. You can also comment @sourcery-ai summary on the pull request to (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the pull request to resolve all Sourcery comments. Useful if you've already addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull request to dismiss all existing Sourcery reviews. Especially useful if you want to start fresh with a new review - don't forget to comment @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

  • Contact our support team for questions or feedback.
  • Visit our documentation for detailed guides and information.
  • Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai[bot] avatar May 12 '25 20:05 sourcery-ai[bot]

Why do we need to support python3.8? We only support python 3.11 or greater.

rhatdan avatar May 12 '25 20:05 rhatdan

pyproject.toml and your previous setup.py both declared support for python 3.8 and above. There's no reason you have to support any specific versions but I was going off the build declarations y'all have been using.

ieaves avatar May 12 '25 20:05 ieaves

Well that was a mistake. We are looking for python3.11 or greater inside of our container-images so we should just stick with that.

rhatdan avatar May 12 '25 20:05 rhatdan

Got it! I can update accordingly then.

ieaves avatar May 13 '25 03:05 ieaves

@ieaves We already apply black and isort in the make target format/check-format (see here) in the CI during the Lint Code job here. I'd consider the results in the CI the source of truth, so the pre-commit hook should use the same rules (not sure if it can just call the make target?) in order to prevent a back and forth with the CI.

engelmi avatar May 13 '25 11:05 engelmi

@ieaves We already apply black and isort in the make target format/check-format (see here) in the CI during the Lint Code job here. I'd consider the results in the CI the source of truth, so the pre-commit hook should use the same rules (not sure if it can just call the make target?) in order to prevent a back and forth with the CI.

Agree with @engelmi , but also not sure we should invade on someones local dev environment here with pre-commit hooks at all, same goes for when people start checking in their personalised IDE configuration files.

ericcurtin avatar May 13 '25 12:05 ericcurtin

Like for example if I do a quick documentation change and I want to commit it quickly and push, I don't want this to occur everytime I do a micro commit, it's invasive

ericcurtin avatar May 13 '25 14:05 ericcurtin

I feel that tension as well, what are your thoughts about a ci job that pushes lint updates back to the commit rather than failing the workflow?

On Tue, May 13, 2025 at 9:11 AM, Eric Curtin < @.*** > wrote:

ericcurtin left a comment (containers/ramalama#1397) ( https://github.com/containers/ramalama/pull/1397#issuecomment-2876693511 )

Like for example if I do a quick documentation change and I want to commit it quickly and push, I don't want this to occur everytime I do a micro commit, it's invasive

— Reply to this email directly, view it on GitHub ( https://github.com/containers/ramalama/pull/1397#issuecomment-2876693511 ) , or unsubscribe ( https://github.com/notifications/unsubscribe-auth/AB3MV564Q7B4I5OJ6EGA4IL26H4SZAVCNFSM6AAAAAB46ZKZ4KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQNZWGY4TGNJRGE ). You are receiving this because you were mentioned. Message ID: <containers/ramalama/pull/1397/c2876693511 @ github. com>

ieaves avatar May 14 '25 07:05 ieaves

@rhatdan I've updated the project build to only accept python 3.11+!

I'm trying to consolidate the dev dependencies into the pyproject.toml so they can be centrally managed. We are using pipx pretty extensively in the make file but pipx doesn't naturally symlink dependency entrypoints which is causing subsequent tests to fail and I don't want to break existing workflows. I have a few ideas for how to proceed

Forget centralized dev dependencies

Leave them duplicated in both the pyproject.toml and in the Makefile.

Use venv in the tests

Self explanatory

Switch from pipx to uv

It looks like uv, unlike pipx, automatically adds tools into the path so rather than pipx install ".[dev]" we could switch to uv pip install ".[dev]". This requires us to install uv in the make command though.

Add pipx to path

Add something like the following in CI / the Makefile so that all installed tools are in PATH.

export PATH="$HOME/.local/pipx/venvs/ramalama/bin:$PATH"

What do y'all think?

ieaves avatar May 14 '25 22:05 ieaves

@rhatdan I've updated the project build to only accept python 3.11+!

I'm trying to consolidate the dev dependencies into the pyproject.toml so they can be centrally managed. We are using pipx pretty extensively in the make file but pipx doesn't naturally symlink dependency entrypoints which is causing subsequent tests to fail and I don't want to break existing workflows. I have a few ideas for how to proceed

Forget centralized dev dependencies

Leave them duplicated in both the pyproject.toml and in the Makefile.

Use venv in the tests

Self explanatory

Switch from pipx to uv

It looks like uv, unlike pipx, automatically adds tools into the path so rather than pipx install ".[dev]" we could switch to uv pip install ".[dev]". This requires us to install uv in the make command though.

Add pipx to path

Add something like the following in CI / the Makefile so that all installed tools are in PATH.

export PATH="$HOME/.local/pipx/venvs/ramalama/bin:$PATH"

What do y'all think?

My vote is switch to uv, I find it a better tool than pipx and easier to use, don't have to worry about venvs, it's convenient, it hides those complexities of managing a python3 userspace.

We already install uv in our installer if there is no package available for your platform:

https://github.com/containers/ramalama/blob/main/install-uv.sh

ericcurtin avatar May 15 '25 09:05 ericcurtin

Looks like this PR is kind of a mess, need to rebase and squash down to a single commit.

rhatdan avatar Jun 05 '25 00:06 rhatdan