[codex] fix Python 3.8/3.9 import compatibility#98
Merged
Conversation
Reviewer's GuideRestores Python 3.8/3.9 import compatibility by replacing runtime PEP 604/585 type syntax with typing-based annotations and adds a CI smoke test that imports all top-level xrobot modules from the built package across the Python matrix. Sequence diagram for CI import smoke test executionsequenceDiagram
participant GithubActionsRunner
participant PythonProcess
participant ci_import_smoke
participant importlib
GithubActionsRunner->>PythonProcess: python scripts/ci_import_smoke.py
activate PythonProcess
PythonProcess->>ci_import_smoke: main()
activate ci_import_smoke
ci_import_smoke->>ci_import_smoke: discover src/xrobot/*.py
loop each module file
ci_import_smoke->>importlib: import_module(xrobot.module)
alt import succeeds
importlib-->>ci_import_smoke: module object
ci_import_smoke->>ci_import_smoke: record imported module
else import fails
importlib-->>ci_import_smoke: raises Exception
ci_import_smoke->>ci_import_smoke: record failure(module, error)
end
end
ci_import_smoke->>PythonProcess: print results
alt failures exist
ci_import_smoke-->>PythonProcess: return code 1
else no failures
ci_import_smoke-->>PythonProcess: return code 0
end
deactivate ci_import_smoke
PythonProcess-->>GithubActionsRunner: exit with status
deactivate PythonProcess
Class diagram for updated xrobot type annotationsclassDiagram
class SourceManagerModule {
+dict load_yaml(source: Union_str_Path)
+None save_yaml(path: Union_str_Path, data: dict)
+None __init__(sources_yaml)
+None load_sources(yaml_path: Union_Path_str)
+list list_modules()
+Optional_str get_repo_url(modid: str)
}
class XRobotSetupModule {
+None ensure_modules_and_sources()
+None run_subprocess(cmd: List_str)
}
class typing {
<<module>>
+Union
+Optional
+List
}
class pathlib {
<<module>>
+Path
}
SourceManagerModule ..> typing : uses Union, Optional
SourceManagerModule ..> pathlib : uses Path
XRobotSetupModule ..> typing : uses List
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path=".github/workflows/python-publish.yml" line_range="128-131" />
<code_context>
name: python-package-${{ matrix.os }}-${{ matrix.arch }}-py${{ matrix.python-version }}
path: dist/*
+ - name: Install built package (Linux/macOS)
+ if: runner.os != 'Windows'
+ run: |
+ pip install dist/*.whl || pip install dist/*.tar.gz
+
+ - name: Install built package (Windows)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Use `python -m pip` to ensure the same interpreter is used for install and smoke tests
This step invokes `pip` directly, while the smoke tests use `python`. On some runners `pip` can point to a different interpreter than the job’s `python`, so you may end up installing into one environment and importing from another. Using `python -m pip install ...` (and likewise in the Windows step) ensures `pip` matches the matrix Python interpreter.
Suggested implementation:
```
- name: Install built package (Linux/macOS)
if: runner.os != 'Windows'
run: |
python -m pip install dist/*.whl || python -m pip install dist/*.tar.gz
```
```
- name: Install built package (Windows)
if: runner.os == 'Windows'
shell: pwsh
run: |
$pkg = Get-ChildItem dist\*.whl,dist\*.tar.gz | Select-Object -First 1
if ($pkg) {
python -m pip install $pkg.FullName
}
```
</issue_to_address>
### Comment 2
<location path="scripts/ci_import_smoke.py" line_range="11-16" />
<code_context>
+
+def main() -> int:
+ repo_root = Path(__file__).resolve().parents[1]
+ src_dir = repo_root / "src" / "xrobot"
+
+ failures = []
+ imported = []
+
+ for path in sorted(src_dir.glob("*.py")):
+ if path.name == "__init__.py":
+ continue
</code_context>
<issue_to_address>
**issue (bug_risk):** Fail fast if the expected `src/xrobot` directory does not exist
If `src_dir` is missing (e.g., due to layout changes or different working dirs), this will import 0 modules and still return success. Please add an explicit `if not src_dir.is_dir():` that exits with a non-zero status (or logs a clear error) to avoid silent false positives.
</issue_to_address>
### Comment 3
<location path="scripts/ci_import_smoke.py" line_range="24-25" />
<code_context>
+ try:
+ importlib.import_module(module_name)
+ imported.append(module_name)
+ except Exception as error:
+ failures.append((module_name, error))
+
+ print(f"Imported {len(imported)} modules.")
</code_context>
<issue_to_address>
**suggestion:** Include traceback details to make debugging import failures easier
Currently only `str(error)` is available for failed imports, which limits debuggability. Please also capture and log the full traceback (e.g., via `traceback.format_exc()` or `traceback.print_exc()` along with the module name) so CI logs contain enough context to diagnose import failures.
Suggested implementation:
```python
try:
importlib.import_module(module_name)
imported.append(module_name)
except Exception as error:
tb = traceback.format_exc()
failures.append((module_name, error, tb))
```
```python
if failures:
print(f"Failed to import {len(failures)} modules.", file=sys.stderr)
for module_name, error, tb in failures:
print(f"ERR {module_name}: {error}", file=sys.stderr)
print(tb, file=sys.stderr)
return 1
```
1. Add `import traceback` near the top of `scripts/ci_import_smoke.py` alongside the other imports.
2. If `failures` is typed or destructured elsewhere (e.g., via type hints or unpacking), update that code to expect a 3-tuple `(module_name, error, traceback_str)` instead of a 2-tuple.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+128
to
+131
| - name: Install built package (Linux/macOS) | ||
| if: runner.os != 'Windows' | ||
| run: | | ||
| pip install dist/*.whl || pip install dist/*.tar.gz |
There was a problem hiding this comment.
suggestion (bug_risk): Use python -m pip to ensure the same interpreter is used for install and smoke tests
This step invokes pip directly, while the smoke tests use python. On some runners pip can point to a different interpreter than the job’s python, so you may end up installing into one environment and importing from another. Using python -m pip install ... (and likewise in the Windows step) ensures pip matches the matrix Python interpreter.
Suggested implementation:
- name: Install built package (Linux/macOS)
if: runner.os != 'Windows'
run: |
python -m pip install dist/*.whl || python -m pip install dist/*.tar.gz
- name: Install built package (Windows)
if: runner.os == 'Windows'
shell: pwsh
run: |
$pkg = Get-ChildItem dist\*.whl,dist\*.tar.gz | Select-Object -First 1
if ($pkg) {
python -m pip install $pkg.FullName
}
Comment on lines
+11
to
+16
| src_dir = repo_root / "src" / "xrobot" | ||
|
|
||
| failures = [] | ||
| imported = [] | ||
|
|
||
| for path in sorted(src_dir.glob("*.py")): |
There was a problem hiding this comment.
issue (bug_risk): Fail fast if the expected src/xrobot directory does not exist
If src_dir is missing (e.g., due to layout changes or different working dirs), this will import 0 modules and still return success. Please add an explicit if not src_dir.is_dir(): that exits with a non-zero status (or logs a clear error) to avoid silent false positives.
Comment on lines
+24
to
+25
| except Exception as error: | ||
| failures.append((module_name, error)) |
There was a problem hiding this comment.
suggestion: Include traceback details to make debugging import failures easier
Currently only str(error) is available for failed imports, which limits debuggability. Please also capture and log the full traceback (e.g., via traceback.format_exc() or traceback.print_exc() along with the module name) so CI logs contain enough context to diagnose import failures.
Suggested implementation:
try:
importlib.import_module(module_name)
imported.append(module_name)
except Exception as error:
tb = traceback.format_exc()
failures.append((module_name, error, tb)) if failures:
print(f"Failed to import {len(failures)} modules.", file=sys.stderr)
for module_name, error, tb in failures:
print(f"ERR {module_name}: {error}", file=sys.stderr)
print(tb, file=sys.stderr)
return 1- Add
import tracebacknear the top ofscripts/ci_import_smoke.pyalongside the other imports. - If
failuresis typed or destructured elsewhere (e.g., via type hints or unpacking), update that code to expect a 3-tuple(module_name, error, traceback_str)instead of a 2-tuple.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #97.
This change fixes the Python-version compatibility regression reported in issue #97.
The immediate user-facing failure was that
xrobot_setupcrashed during import on Python 3.9 becausexrobot.SourceManagerused PEP 604 union type syntax such asstr | Path, which is only valid at runtime on Python 3.10 and newer. The package metadata, however, still declaresRequires-Python >=3.8, so the installed package could be pulled into Python 3.8 and 3.9 environments even though some modules were no longer importable there.The fix keeps the declared Python support floor and makes the affected modules compatible with that floor again. In
SourceManager.py, the PEP 604 annotations are replaced withtyping.Unionandtyping.Optional. InXRobotSetup.py, thelist[str]annotation is replaced withtyping.List[str], which is also required for real Python 3.8 compatibility. Together these changes remove the import-time incompatibilities while preserving behavior.This PR also adds a small CI guard so the same class of regression is caught automatically next time. After the wheel or sdist is built in the existing Python version matrix job, the workflow now installs the built package and runs
scripts/ci_import_smoke.py, which imports every top-levelxrobotmodule from the installed package. That turns this issue from a user-reported runtime surprise into a direct CI failure on unsupported syntax regressions.Validation for this change was done locally in two ways. First, the repository was rescanned to make sure no remaining top-level type annotations relied on PEP 604 or PEP 585 generics in the affected modules. Second, the package was built and installed into a clean virtual environment and
python scripts/ci_import_smoke.pycompleted successfully, importing all top-level modules, includingxrobot.SourceManagerandxrobot.XRobotSetup.Summary by Sourcery
Restore Python 3.8–3.9 compatibility and add a CI smoke test to catch import-time regressions for xrobot modules.
Bug Fixes:
Enhancements:
CI: