Skip to content

Conversation

@Eyal-Danieli
Copy link
Member

No description provided.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR converts the project from traditional pip/pipenv to UV as the package manager, adding modern Python packaging standards and comprehensive documentation.

Key Changes:

  • Adds pyproject.toml with full project metadata, dependencies, and build configuration using hatchling
  • Updates mlrun version requirement from >=1.0.0 to >=1.10.0
  • Adds Makefile with common development tasks (sync, format, lint, test, cli)
  • Adds comprehensive README.md with installation, usage, and contribution guidelines

Reviewed changes

Copilot reviewed 3 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
pyproject.toml New file defining project metadata, dependencies in both optional-dependencies and dependency-groups formats, and build system configuration
requirements.txt Bumps mlrun minimum version from 1.0.0 to 1.10.0 for consistency with pyproject.toml
README.md New comprehensive documentation covering installation with UV, CLI usage, contribution guidelines, and troubleshooting
Makefile New file providing convenient shortcuts for common tasks (sync, format, lint, test, cli)
.gitignore Adds UV-specific entries (.uv/ directory and uv.lock file)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +88 to +117
[dependency-groups]
dev = [
"urllib3>=1.26.20",
"v3io-frames>=0.10.15",
"GitPython~=3.1, >=3.1.41",
"aiohttp~=3.11",
"aiohttp-retry~=2.9",
"click~=8.1",
"nest-asyncio~=1.0",
"ipython~=8.10",
"nuclio-jupyter~=0.11.2",
"numpy>=1.26.4, <1.27.0",
"pandas>=1.2, <2.2",
"pyarrow>=10.0, <18",
"requests~=2.32",
"tabulate~=0.8.6",
"python-dotenv~=1.0",
"setuptools>=75.2",
"deprecated~=1.2",
"orjson>=3.9.15, <4",
"docstring_parser~=0.16",
"aiosmtplib~=3.0",
"deepdiff>=8.6.1,<9.0.0",
# The following dependencies will be pulled by mlrun with correct versions
# to avoid conflicts, we let mlrun manage them:
# pyyaml, semver, v3io, pydantic, protobuf, mergedeep,
# fsspec, v3iofs, storey, inflection, jinja2,
# mlrun-pipelines-kfp-common, mlrun-pipelines-kfp-v1-8
]

Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

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

The [project.optional-dependencies] section (lines 42-79) and [dependency-groups] section (lines 88-116) both define a "dev" group with largely overlapping but not identical dependencies. This creates maintenance issues and potential conflicts. According to PEP 735, [dependency-groups] is the newer standard for UV and should be used instead of [project.optional-dependencies] for development dependencies. Consider removing the [project.optional-dependencies] section entirely and keeping only [dependency-groups].

Suggested change
[dependency-groups]
dev = [
"urllib3>=1.26.20",
"v3io-frames>=0.10.15",
"GitPython~=3.1, >=3.1.41",
"aiohttp~=3.11",
"aiohttp-retry~=2.9",
"click~=8.1",
"nest-asyncio~=1.0",
"ipython~=8.10",
"nuclio-jupyter~=0.11.2",
"numpy>=1.26.4, <1.27.0",
"pandas>=1.2, <2.2",
"pyarrow>=10.0, <18",
"requests~=2.32",
"tabulate~=0.8.6",
"python-dotenv~=1.0",
"setuptools>=75.2",
"deprecated~=1.2",
"orjson>=3.9.15, <4",
"docstring_parser~=0.16",
"aiosmtplib~=3.0",
"deepdiff>=8.6.1,<9.0.0",
# The following dependencies will be pulled by mlrun with correct versions
# to avoid conflicts, we let mlrun manage them:
# pyyaml, semver, v3io, pydantic, protobuf, mergedeep,
# fsspec, v3iofs, storey, inflection, jinja2,
# mlrun-pipelines-kfp-common, mlrun-pipelines-kfp-v1-8
]

Copilot uses AI. Check for mistakes.
# Check if you're in the right directory
pwd # Should be the project root

# Ensure dependencies are installed
Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

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

The documentation references a make install command that doesn't exist in the Makefile. Based on the Installation section and other commands in the Makefile, this should be changed to make sync or uv sync --all-groups --prerelease=allow.

Suggested change
# Ensure dependencies are installed
make sync

Copilot uses AI. Check for mistakes.
@awk 'BEGIN {FS = ":.*?## "} /^[a-zA-Z_-]+:.*?## / {printf " %-15s %s\n", $$1, $$2}' $(MAKEFILE_LIST)

sync: ## Sync dependencies from lockfile
uv sync --prerelease=allow
Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

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

The make sync command in the Makefile uses uv sync --prerelease=allow but does not include the --all-groups flag. This is inconsistent with the installation instructions in the README (line 40) which recommend uv sync --all-groups --prerelease=allow. Without --all-groups, development dependencies won't be installed. Consider adding --all-groups to the sync target.

Suggested change
uv sync --prerelease=allow
uv sync --all-groups --prerelease=allow

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant