[MNT] [DOC] use uv in build, add precommit, fix ruff style and typo #107

matteoettam09 · 2024-12-22T15:10:44Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Does your contribution introduce a new dependency? If yes, which one?

What should a reviewer concentrate their feedback on?

Did you add any tests for the change?

Any other comments?

PR checklist

For all contributions

The PR title starts with either [ENH], [MNT], [DOC], or [BUG]. [BUG] - bugfix, [MNT] - CI, test framework, [ENH] - adding or improving code, [DOC] - writing or improving documentation or docstrings.
Optionally, I've added myself and possibly others to the CODEOWNERS file - do this if you want to become the owner or maintainer of an estimator you added.

For new estimators

I've added the estimator to the API reference in docs/api.rst.
I've added one or more illustrative usage examples to the docstring and the examples section.

HugoDelatte

Thx the PR!

Moving to uv and adding pre-commit is a good idea.
Let's ensure the Pipeline is as simple and clean as possible.

Also, could you rename the PR to smth related to uv and pre-commit?

HugoDelatte · 2024-12-22T19:02:14Z

Makefile

By experience, Makefile should only be used when you need to re-use long commands which is not our case.
I would be in favour of simplicity and readability by keeping the commands in the ymls.

Done, please check

HugoDelatte · 2024-12-22T19:03:17Z

Makefile

@@ -0,0 +1,46 @@
+.PHONY: uv-download


using the action "astral-sh/setup-uv@v4" seems to be the preferred way:
https://docs.astral.sh/uv/guides/integration/github/#installation

Was already done

HugoDelatte · 2024-12-22T19:06:03Z

Makefile

+	uv venv
+ifeq ($(OS),Windows_NT)
+	# Windows-specific virtual environment activation
+	. .venv/Scripts/activate && uv pip install -e .[tests]


I think we should use "uv sync --dev", rename "tests" by "dev" in pyproject.toml and move it to the new "[dependency-groups]" instead of "[project.optional-dependencies]"

I don't think we are ready to move to [dependency-groups]:

astral-sh/uv#8590 (comment)

The only one supporting it is https://hatch.pypa.io/latest/
but I wouldn't go that far.

Updated the toml, see if good enough now

HugoDelatte · 2024-12-22T19:07:21Z

.github/workflows/tests.yml

@@ -28,17 +28,27 @@ jobs:
      - name: Upgrade pip
        run: python -m pip install --upgrade pip


I guess we don't need that "Upgrade pip" anymore?

HugoDelatte · 2024-12-22T19:14:16Z

.github/workflows/tests.yml

@@ -28,17 +28,27 @@ jobs:
      - name: Upgrade pip
        run: python -m pip install --upgrade pip

-      - name: Install dependencies
-        run: pip install ".[tests]"
+      - name: Cache Python packages


I think it should be done with https://docs.astral.sh/uv/guides/integration/github/#caching

- name: Enable caching uses: astral-sh/setup-uv@v4 with: enable-cache: true

Nice, thank you, done

pyproject.toml

HugoDelatte · 2024-12-22T19:20:36Z

pyproject.toml

@@ -85,6 +86,12 @@ docs = [
    "jupyterlite-pyodide-kernel",
    "nbformat",
 ]
+dev = [


let's merge test into dev and move them (with docs) to the new "[dependency-groups]" (we don't need to build dev and docs from PyPI, just locally).
I don't think isort is useful as sorting is now done in ruff
What's the reason to include pydocstyle?

See my comment above on why not dependency-groups.

isort can go, did some experiments with pydocstyle, pushed by mistake

codecov · 2024-12-22T22:17:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.86%. Comparing base (a4d8db5) to head (f7d4e93).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #107      +/-   ##
==========================================
- Coverage   99.07%   94.86%   -4.21%     
==========================================
  Files          41      126      +85     
  Lines        3226     8083    +4857     
==========================================
+ Hits         3196     7668    +4472     
- Misses         30      415     +385

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

HugoDelatte · 2024-12-23T10:25:10Z

.github/workflows/docs.yml

+        with:
+          version: "latest"
+          enable-cache: true
+          cache-dependency-glob: "requirements**.txt"


requirements files are now legacy, they should be avoided (duplicate info with pyproject.toml).
I guess you did that because you want to avoid cache invalidation from uv each time unrelated change occurs in pyproject.toml. In that case, having more frequent invalidation would be better than re-introducing legacy files.
Let's drop this lien, "cache-dependency-glob" (the default will be pyproject.toml)

Ok, I still would like to have a requirements.in / uv.lock file to be used for both caching and releases:

https://docs.astral.sh/uv/guides/projects/#uvlock

uv.lock is a cross-platform lockfile that contains exact information about your project's dependencies. Unlike the pyproject.toml which is used to specify the broad requirements of your project, the lockfile contains the exact resolved versions that are installed in the project environment. This file should be checked into version control, allowing for consistent and reproducible installations across machines.

And removing this defaults to uv.lock, instead of the toml:

https://github.com/astral-sh/setup-uv/tree/v4/?tab=readme-ov-file#cache-dependency-glob

I have just fixed pushing the lock (uv lock)

HugoDelatte · 2024-12-23T10:32:43Z

.github/workflows/docs.yml

-        run: pip install ".[docs]"
+        run: |
+          uv venv
+          uv pip install ".[docs]"


The recommended way seems to be uv install --extras doc, otherwise uv still relies on pip.
Correct me if my understand is wrong.

I would also separate the virtual env setup in specific step:

- name: Set up virtual environment run: uv venv

Updated.

Note:

uv install
error: unrecognized subcommand 'install'
tip: a similar subcommand exists: 'uv pip install'
Usage: uv [OPTIONS]
For more information, try '--help'.

Locking seems to work (beside caching) without additional cli inputs:

uv pip install -r pyproject.toml --extra dev Resolved 34 packages in 615ms Prepared 34 packages in 11.91s Installed 34 packages in 344ms + cfgv==3.4.0 + clarabel==0.9.0 + coverage==7.6.9 + cvxpy==1.6.0 + distlib==0.3.9 + filelock==3.16.1 + identify==2.6.3 + iniconfig==2.0.0 + joblib==1.4.2 + nodeenv==1.9.1 + numpy==2.2.1 + osqp==0.6.7.post3 + packaging==24.2 + pandas==2.2.3 + platformdirs==4.3.6 + plotly==5.24.1 + pluggy==1.5.0 + pre-commit==4.0.1 + pyscipopt==5.2.1 + pytest==8.3.4 + pytest-cov==6.0.0 + python-dateutil==2.9.0.post0 + pytz==2024.2 + pyyaml==6.0.2 + qdldl==0.1.7.post4 + ruff==0.8.4 + scikit-learn==1.6.0 + scipy==1.14.1 + scs==3.2.7 + six==1.17.0 + tenacity==9.0.0 + threadpoolctl==3.5.0 + tzdata==2024.2 + virtualenv==20.28.0

HugoDelatte · 2024-12-23T10:33:48Z

.github/workflows/tests.yml

+        with:
+          version: "latest"
+          enable-cache: true
+          cache-dependency-glob: "requirements**.txt"


see same comment in docs.yml

Done, see above

HugoDelatte · 2024-12-23T10:34:03Z

.github/workflows/tests.yml

-        run: ruff check --output-format=github
+        run: |
+          uv venv
+          uv pip install -e ".[dev]"


see same comment in docs.yml

Done, see above

HugoDelatte · 2024-12-23T10:36:24Z

.github/workflows/tests.yml


      - name: Run and write pytest
-        run: pytest --cov=./ --cov-report=xml
+        run: |
+          uv run pytest --cov=./ --cov-report=xml


there is only one cmd line, not sure the "|" is needed

HugoDelatte · 2024-12-23T10:53:17Z

pyproject.toml

@@ -1,28 +1,19 @@
 [build-system]
-requires = ["setuptools"]
-build-backend = "setuptools.build_meta"
+requires = ["hatchling"]


as discussed, let's keep setuptools for the moment (more flexible and stable). Let's monitor Hatchling to see if it becomes relevant to use it in the future (but we will need to properly test the build output before we integrate it).

Yes, ok, done

HugoDelatte · 2024-12-23T10:56:02Z

pyproject.toml

@@ -80,18 +88,11 @@ docs = [
    "sphinx-prompt",
    "sphinxext.opengraph",
    "sphinx-sitemap",
-    "sphinx-favicon",


what' the reason to remove sphinx-favicon ?

Duplicate entry

HugoDelatte · 2024-12-23T10:56:31Z

requirements.in

let's remove (see comment in docs.yml)

HugoDelatte · 2024-12-23T10:56:40Z

requirements.txt

let's remove (see comment in docs.yml)

HugoDelatte · 2024-12-23T11:09:21Z

pyproject.toml

Let's add "I005", # Explicitly ensure sorting of __all__ in [tool.ruff.lint] select.
This rule was deactivated in project.toml because of the other ones, but not in your initial pre-commit args.

ruff failed
Cause: Failed to parse /Users/mattettam09/Desktop/code/skfolio/pyproject.toml
Cause: TOML parse error at line 121, column 1
|
121 | [tool.ruff.lint]
| ^^^^^^^^^^^^^^^^
Unknown rule selector: ⁠ I005 ⁠

… releases

HugoDelatte · 2024-12-24T12:23:06Z

.github/workflows/build.yml

Technically, the build is done in the realise GitHub action, so it's more appropriate to keep the name as "tests".

Ok

fade3c3

HugoDelatte · 2024-12-24T12:25:03Z

CONTRIBUTING.md

+
+   or using **uv**:
+   ```shell
+   $ uv pip install -r pyproject.toml --extra dev


looks like also with uv, install should be in editable mode: uv pip install -r pyproject.toml --editable . --extra dev

HugoDelatte · 2024-12-24T12:25:35Z

CONTRIBUTING.md

   ```shell
   $ pip install --editable ".[docs]"
   $ cd docs
   $ sphinx-build . _build
   ```
+   or using **uv**:
+   ```shell
+   $ uv pip install -r pyproject.toml --extra docs


uv pip install -r pyproject.toml --editable . --extra docs

HugoDelatte

Looks good now, thx!

matteoettam09 added 4 commits December 22, 2024 16:10

fix: docs typo

7e4e6b3

fix: cicd, makefile, precommit, use uv (closes #104)

16e73d5

fix: makefile os-generic

38e1e13

fix: install uv workflow

2bef854

HugoDelatte self-requested a review December 22, 2024 18:59

matteoettam09 changed the title ~~fix: docs typo~~ [MNT] [DOC] fix: docs typo Dec 22, 2024

HugoDelatte requested changes Dec 22, 2024

View reviewed changes

matteoettam09 changed the title ~~[MNT] [DOC] fix: docs typo~~ [MNT] [DOC] use uv in build, add precommit, fix ruff style and typo Dec 22, 2024

matteoettam09 added 9 commits December 22, 2024 21:25

fix: remove manual version bump

37b4c4a

fix: remove make, explicit ci

ecc3e43

fix: os-agnostic ci

0625362

fix: cache with uv

6bee441

chore: setuptools to hatchling, refacto dependencies

6814e2f

fix: lock dependencies for uv cache

4f0df73

fix: tests ci

e432f77

fix: remove explicit venv activation ci

ddfe387

chore: uniformize tests and docs cicd

f7d4e93

fix: cache with uv only

cc64499

HugoDelatte requested changes Dec 23, 2024

View reviewed changes

matteoettam09 added 8 commits December 23, 2024 14:22

fix: remove legacy requirements, push lock and use it for caching and…

5d4c24a

… releases

chore: refacto ci, use up-to-date cli

f463eda

chore: unnecessary pipe

455e73e

fix: pip-compile to uv-lock

dd8f23f

fix: precommit config from pyproject.toml

f5b879c

fix: setuptools

bd14304

fix: remove lock from release and cache

57f0a0a

docs: contributing

7e80c61

HugoDelatte requested changes Dec 24, 2024

View reviewed changes

fix: workflow naming

fade3c3

fix: editable install in contribution docs

586ce65

HugoDelatte approved these changes Dec 30, 2024

View reviewed changes

HugoDelatte merged commit 3218cfa into main Dec 30, 2024
9 checks passed

HugoDelatte deleted the docs branch December 30, 2024 16:54

		@@ -28,17 +28,27 @@ jobs:
		- name: Upgrade pip
		run: python -m pip install --upgrade pip

[MNT] [DOC] use uv in build, add precommit, fix ruff style and typo #107

[MNT] [DOC] use uv in build, add precommit, fix ruff style and typo #107

Conversation

matteoettam09 commented Dec 22, 2024 • edited Loading

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Does your contribution introduce a new dependency? If yes, which one?

What should a reviewer concentrate their feedback on?

Did you add any tests for the change?

Any other comments?

PR checklist

For all contributions

For new estimators

HugoDelatte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 22, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HugoDelatte left a comment

Choose a reason for hiding this comment

matteoettam09 commented Dec 22, 2024 •

edited

Loading