feat: update Python version handling and refactor base images retrieval#60
feat: update Python version handling and refactor base images retrieval#60
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This pull request adds support for configurable Python versions in Docker base images and refactors the project layout detection logic. The main purpose is to enable users to specify which Python version their project should use when building containers.
Changes:
- Introduced a
python_versionfield toQ8SPythonEnvdataclass with a default of "3.12" - Refactored base image definitions from constants to a dynamic function
get_base_images()that interpolates Python versions - Added src-layout detection support for both
config.cfgandpyproject.tomlconfiguration files - Extracted shared test utility function
write()intotests/utils.pyfor reuse across test files
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/utils.py | New shared test utility file containing the write() helper function for creating test files |
| tests/test_workload.py | New test suite for validating src-layout detection from configuration files |
| tests/test_multifiles.py | Refactored to use shared write() utility instead of local implementation |
| src/q8s/workload.py | Extended to detect src-layout from both config.cfg and pyproject.toml files |
| src/q8s/project.py | Updated to use dynamic base images with configurable Python version |
| src/q8s/constants.py | Converted BASE_IMAGES from constant to function supporting Python version parameter |
| pyproject.toml | Version bumped to 0.12.0-dev1 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def get_base_images(python_version: str = "3.12") -> dict[str, str]: | ||
| return { | ||
| "cpu": f"python:{python_version}-slim", | ||
| "gpu": f"ghcr.io/qubernetes-dev/cuda:12.8.1-r2-py{python_version}", | ||
| "qpu": f"python:{python_version}-slim", | ||
| } |
There was a problem hiding this comment.
The python_version parameter in get_base_images lacks input validation. Consider adding validation to ensure the version string is in the expected format (e.g., "3.12", "3.11") to prevent invalid Docker image tags from being generated.
src/q8s/workload.py
Outdated
| if where == "src": | ||
| self.__pyproject_root = root | ||
| return True | ||
| cfg_path = root / "config.cfg" |
There was a problem hiding this comment.
The filename should be "setup.cfg" not "config.cfg". The test in test_workload.py expects "config.cfg", but standard Python packaging uses "setup.cfg". This appears to be a bug - either the code should look for "setup.cfg" or the test should be updated to reflect a non-standard configuration filename.
| cfg_path = root / "config.cfg" | |
| cfg_path = root / "setup.cfg" |
src/q8s/workload.py
Outdated
| pyproject_path = root / "pyproject.toml" | ||
| if pyproject_path.exists(): | ||
| config = configparser.ConfigParser() | ||
| config.read(pyproject_path) | ||
|
|
||
| if config.has_section("tool.setuptools.packages.find"): | ||
| where = config.get( | ||
| "tool.setuptools.packages.find", "where", fallback="" | ||
| ).strip() | ||
| if where == '["src"]': | ||
| self.__pyproject_root = root | ||
| return True |
There was a problem hiding this comment.
Using ConfigParser to parse TOML files is incorrect. TOML and INI formats are different and ConfigParser cannot properly parse TOML syntax. For example, TOML list syntax like ["src"] will be read as a literal string '["src"]' rather than being parsed as a list. You should use a TOML parser library like tomli or tomllib (built-in for Python 3.11+) instead.
src/q8s/workload.py
Outdated
| where = config.get( | ||
| "tool.setuptools.packages.find", "where", fallback="" | ||
| ).strip() | ||
| if where == '["src"]': |
There was a problem hiding this comment.
Comparing the parsed value to the literal string '["src"]' indicates that ConfigParser is being used incorrectly for TOML parsing. In proper TOML parsing, the value would be a Python list, not a string. This string comparison will only work by accident because ConfigParser treats TOML as INI format and returns the raw value as a string.
src/q8s/constants.py
Outdated
| # BASE_IMAGES = { | ||
| # "cpu": "python:3.12-slim", | ||
| # "gpu": "ghcr.io/qubernetes-dev/cuda:12.8.1", | ||
| # "qpu": "python:3.12-slim", | ||
| # } |
There was a problem hiding this comment.
Commented-out code should be removed rather than left in the codebase. If the old BASE_IMAGES definition is no longer needed, it should be deleted. If it's kept for reference, consider adding a clear comment explaining why it's preserved.
| # BASE_IMAGES = { | |
| # "cpu": "python:3.12-slim", | |
| # "gpu": "ghcr.io/qubernetes-dev/cuda:12.8.1", | |
| # "qpu": "python:3.12-slim", | |
| # } |
| @dataclass | ||
| class Q8SPythonEnv: | ||
| dependencies: List[str] | ||
| python_version: str = "3.12" |
There was a problem hiding this comment.
The new python_version parameter in Q8SPythonEnv and its usage in get_base_images lacks test coverage. Consider adding tests that verify different Python versions generate the correct base image strings, particularly for the GPU image where the version is interpolated into the image tag.
No description provided.