Refactor: Verify that dependencies listed in pyproject.toml and setup.py are consistent#5967
Refactor: Verify that dependencies listed in pyproject.toml and setup.py are consistent#5967aumiidutta wants to merge 1 commit intofeast-dev:masterfrom
Conversation
| home = C:\Users\Aumii\AppData\Local\Programs\Python\Python311 | ||
| include-system-site-packages = false | ||
| version = 3.11.9 | ||
| executable = C:\Users\Aumii\AppData\Local\Programs\Python\Python311\python.exe | ||
| command = C:\Users\Aumii\AppData\Local\Programs\Python\Python311\python.exe -m venv C:\Users\Aumii\Desktop\github\feast\.fvenv |
There was a problem hiding this comment.
🔴 Committed Windows virtual environment directory (.fvenv) with binaries and hardcoded user paths
The PR accidentally commits an entire Windows Python virtual environment directory (.fvenv/) containing ~64MB of Windows executables (python.exe, uv.exe, pip.exe, etc.) and scripts with hardcoded local Windows paths (C:\Users\Aumii\Desktop\github\feast\.fvenv).
Security and repository hygiene impact
The .fvenv directory is not covered by .gitignore (which only ignores .venv and venv/). This means:
- Binary bloat: 64MB of Windows executables are added to the repository history permanently, including
uv.exe(64MB alone),python.exe,pythonw.exe, etc. - Hardcoded paths: Files like
.fvenv/Scripts/activate(line 38) containVIRTUAL_ENV="C:\Users\Aumii\Desktop\github\feast\.fvenv"and.fvenv/pyvenv.cfgcontainshome = C:\Users\Aumii\AppData\Local\Programs\Python\Python311, leaking the contributor's local filesystem structure. - Security risk: Committing binary executables to a repository is a security concern as they could potentially be tampered with or contain unintended code.
Prompt for agents
Remove the entire .fvenv directory from the repository. This is an accidentally committed Windows Python virtual environment. Run: git rm -r .fvenv/ and add .fvenv to .gitignore to prevent future accidental commits. The .gitignore currently only has .venv and venv/ patterns.
Was this helpful? React with 👍 or 👎 to provide feedback.
| setup_extras = set(setup_deps.keys()) - {"core"} | ||
|
|
||
| # Skip composite groups that are managed differently in each file | ||
| skip_comparison = {"ci", "minimal-sdist-build", "minimal", "nlp", "dev", "docs", "image"} |
There was a problem hiding this comment.
🔴 Sync test fails for milvus group due to unhandled feast[] indirection in pyproject.toml
The test_optional_dependencies_in_sync test will report a false mismatch for the milvus dependency group because it doesn't account for feast[...] references that resolve to equivalent dependencies.
Root cause and detailed explanation
In pyproject.toml (lines 97-101), the milvus group uses feast[setuptools] to indirectly include setuptools>=60,<81:
milvus = ["pymilvus>2.5", "milvus-lite==2.4.12", "feast[setuptools]"]
In setup.py (line 168), MILVUS_REQUIRED directly includes setuptools>=60,<81:
MILVUS_REQUIRED = ["pymilvus>2.5", "milvus-lite==2.4.12", "setuptools>=60,<81"]The parse_pyproject_toml() function at sdk/python/tests/unit/test_dependency_sync.py:104 filters out all feast[...] references:
filtered_deps = [dep for dep in deps if not dep.startswith("feast[")]This leaves pyproject.toml's milvus as {"pymilvus>2.5", "milvus-lite==2.4.12"} while setup.py's milvus is {"pymilvus>2.5", "milvus-lite==2.4.12", "setuptools>=60,<81"}. The milvus group is not in the skip_comparison set at line 242, so the test will flag setuptools>=60,<81 as being in setup.py but not in pyproject.toml — a false positive since they are functionally equivalent.
Impact: The test_optional_dependencies_in_sync test will fail, defeating the purpose of the sync verification.
| skip_comparison = {"ci", "minimal-sdist-build", "minimal", "nlp", "dev", "docs", "image"} | |
| skip_comparison = {"ci", "minimal-sdist-build", "minimal", "nlp", "dev", "docs", "image", "milvus"} |
Was this helpful? React with 👍 or 👎 to provide feedback.
| # Normalize marker spacing | ||
| marker = re.sub(r'\s+', ' ', marker) | ||
| normalized = f"{normalized}; {marker}" |
There was a problem hiding this comment.
🟡 normalize_dependency doesn't normalize whitespace around operators in environment markers
The normalize_dependency function fails to normalize spacing around comparison operators within environment markers, causing semantically identical markers to produce different normalized strings.
Root cause and impact
At sdk/python/tests/unit/test_dependency_sync.py:79, the marker normalization only collapses multiple whitespace to single space:
marker = re.sub(r'\s+', ' ', marker)This means python_version == "3.10" (with spaces around ==) and python_version=="3.10" (no spaces) normalize to different strings:
'ray>=2.47.0; python_version == "3.10"''ray>=2.47.0; python_version=="3.10"'
The test at lines 324-330 masks this by only asserting "ray" in norm1 instead of norm1 == norm2. Currently both pyproject.toml and setup.py use the same marker spacing, so this doesn't cause a false positive today. However, if a future edit introduces different marker spacing between the two files, the sync test would incorrectly report them as out of sync.
| # Normalize marker spacing | |
| marker = re.sub(r'\s+', ' ', marker) | |
| normalized = f"{normalized}; {marker}" | |
| # Normalize marker spacing: normalize around comparison operators and collapse whitespace | |
| marker = re.sub(r'\s*(==|!=|>=|<=|>|<|~=|===)\s*', r' \1 ', marker) | |
| marker = re.sub(r'\s+', ' ', marker) | |
| normalized = f"{normalized}; {marker}" |
Was this helpful? React with 👍 or 👎 to provide feedback.
#5964
Visited the booth and tried to solve it!