-
Notifications
You must be signed in to change notification settings - Fork 42
Fix docs build failure by ensuring stable SCM versioning in CI workflow #762
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughThe GitHub Actions CI workflow was updated to use the UV package manager for dependency installation and task runs, reference the Python version from a file, enable UV caching, and add a lock-file verification step. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
amaslenn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR adds a workaround addressing symptoms, not the root cause. The real issue is that #760 didn't update uv.lock file.
Please add the following command to avoid such issues in future: uv lock --check (right after "Install dependencies" step under "lint" job).
@srivatsankrishnan please do not create new PRs for the same topic as you did with (#756 and #760), that ruins review process.
The CI/CD you added (pivot towards uv) is buggy @amaslenn. The root cause is a mismatch time stamp getting generated as clearly described in the PR description. Ideally you are right that this ( |
Greptile SummaryFixed docs build failure by adding Key Changes:
Issues Found:
Confidence Score: 3/5
Important Files Changed
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (1)
-
.github/workflows/ci.yml, line 24-33 (link)style: uv is installed and lockfile is checked, but dependencies are still installed via pip on line 30. Consider using
uv sync --frozen --extra dev --extra docsfor consistency with the docs workflow and deterministic buildsNote: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
2 files reviewed, 1 comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml
🔇 Additional comments (3)
.github/workflows/ci.yml (3)
16-17: LGTM! Full git checkout enables stable SCM versioning.This change correctly addresses the root cause described in the PR: shallow checkouts were causing flaky SCM-version computation, leading to version mismatches between wheel metadata and filename.
21-22: LGTM! Pinning to .python-version ensures consistency.Reading the Python version from
.python-versionensures the lint job uses the repository's standard Python version, improving consistency across environments.
32-33: LGTM! Lock file validation prevents drift.This step correctly implements the requested
uv lock --checkvalidation immediately after dependency installation, as specified in the PR comments. It ensuresuv.lockremains synchronized withpyproject.toml, preventing the lock file drift that contributed to the versioning issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (2)
-
.github/workflows/ci.yml, line 29-30 (link)style: The lint job uses
pip installwhile the docs workflow usesuv sync --frozen. Consider usinguv sync --frozen --all-extrashere too for consistency and to ensure deterministic builds matchinguv.lock.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
.github/workflows/ci.yml, line 86-87 (link)style: Test job uses
pip installinstead ofuv sync. Consider usinguv sync --frozenfor deterministic builds matching the lockfile.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
2 files reviewed, 2 comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (3)
-
.github/workflows/ci.yml, line 14-17 (link)logic: PR description claims to fix docs build workflow, but these SCM versioning fixes are applied to
ci.yml(PR checks) instead of.github/workflows/docs.yml(main branch documentation deployment). The docs workflow still lacksfetch-depth: 0needed for stable SCM versioning. -
.github/workflows/ci.yml, line 32-33 (link)logic: The
uv lock --checkvalidation runs afterpip install '.[dev,docs]'on line 30. If dependencies changed, pip install may fail before the lock check can detect it. Consider moving this check before the pip install step, or switch to usinguv sync --frozeninstead of pip.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
.github/workflows/ci.yml, line 76-79 (link)style: The test job also adds
fetch-depth: 0but doesn't install uv or checkuv.lockconsistency. For consistency with the lint job, consider adding the same uv setup and lock validation here.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 file reviewed, 3 comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (1)
-
.github/workflows/ci.yml, line 76-87 (link)logic: test job still uses pip instead of uv, inconsistent with the PR's stated goal to "fix bugs in the original CI/CD when using uv as a default way to use CloudAI"
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 file reviewed, 1 comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
.github/workflows/ci.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Greptile Review
- GitHub Check: Run pytest (3.10)
- GitHub Check: Run pytest (3.12)
🔇 Additional comments (4)
.github/workflows/ci.yml (4)
16-17: LGTM: Full git checkout enables stable SCM versioning.The
fetch-depth: 0ensures the full git history (tags, commits) is available, which is necessary for SCM-based dynamic versioning to compute consistent versions.
21-22: LGTM: Using version file ensures consistency.Reading the Python version from
.python-versionensures the CI environment matches the repository's pinned version.
24-27: LGTM: UV installation with caching configured.The UV package manager is properly installed with caching enabled for improved CI performance.
29-30: LGTM: Lock file verification addresses maintainer feedback.The
uv lock --checkstep validates thatuv.lockis synchronized withpyproject.tomlbefore dependency installation, catching drift early. This directly addresses the maintainer's request in the comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (6)
-
.github/workflows/ci.yml, line 76-79 (link)style:
testjob still uses pip whilelintjob now uses uv, creating inconsistency. Should migrate test job to uv as well for consistency.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
.github/workflows/ci.yml, line 86-87 (link)style:
testjob still uses pip instead of uv. Should useuv sync --frozen --extra devfor consistency with lint job.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
.github/workflows/ci.yml, line 89-102 (link)style:
testjob should useuv runprefix for pytest commands to match lint job's pattern.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
.github/workflows/ci.yml, line 104-111 (link)style: Local installation test still uses pip. Consider migrating to uv for consistency, or document why pip is required here.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
.github/workflows/ci.yml, line 113-114 (link)style:
cloudaicommands should useuv runprefix for consistency with the lint job pattern.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
.github/workflows/ci.yml, line 120-132 (link)style:
cloudaicommands should useuv runprefix for consistency.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 file reviewed, 6 comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
86-87: Consider migrating test job touvfor consistency.The test job still uses
pip installwhile the lint job now usesuv sync --frozen. This creates an inconsistency where the two jobs may resolve different dependency versions:
- Lint job: Uses exact versions from
uv.lock- Test job: Resolves dependencies at runtime from
pyproject.tomlconstraintsFor consistency and to ensure both jobs test against the same locked dependencies, consider migrating to:
uv sync --frozen --extra devTrade-off consideration: If the intent is to validate that pip-based installation (as documented for users) still works, then keeping
pip installhas value. However, this should be documented, and you might want a separate job that explicitly tests pip installation while the main test job usesuvfor reproducibility.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Greptile Review
- GitHub Check: Run pytest (3.10)
- GitHub Check: Run pytest (3.12)
🔇 Additional comments (5)
.github/workflows/ci.yml (5)
16-17: Correct fix for SCM versioning.Setting
fetch-depth: 0ensures the full git history and tags are available, which is essential for pdm-backend's SCM-based dynamic versioning to compute stable, consistent versions. This directly addresses the root cause described in the PR objectives.
29-30: Good addition per PR requirements.The
uv lock --checkstep addresses the concern raised in the PR comments about catching missing or outdated lock files in CI. Positioning this check beforeuv syncensures fast failure if the lock file is out of date, preventing inconsistent dependency resolution.
33-33: Correctly includes--frozenflag.The
uv sync --frozen --extra dev --extra docscommand now strictly enforces the locked dependencies fromuv.lock, addressing the critical issue from the previous review. This ensures reproducible builds and prevents dependency drift.
36-63: Consistent use ofuv runfor all commands.All linter and documentation build commands now execute via
uv run, ensuring they use the uv-managed environment with locked dependencies. The preservation ofPYTHONPATH=srcon line 48 forlint-importsis correct and necessary.
21-22: The.python-versionfile exists and contains the valid Python version3.10, so the workflow configuration is correct.
@greptile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! I've created a rule to remember that CloudAI intentionally uses mixed pip/uv workflows in CI to ensure compatibility with both package managers. Future reviews won't flag this as an inconsistency.
This approach makes a lot of sense - it gives you confidence that CloudAI works correctly regardless of which installation method users choose, and it's good practice for a project that officially supports multiple package managers.
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - name: Set up Python |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you've also migrated to uv, setting up python separately is not needed, uv sync will do it automatically.
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
| with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the whole history is needed for this job?
|
Andrei will provide the fix to the CI bug. |
Summary
Fix docs build failure caused by SCM version mismatch during uv sync. This PR also fixes the bugs in the orignal CI/CD when using uv as a default way to use CloudAI. For instance, PR 701 and PR 702 makes the first version to upload the uv lock file.
This PR also fixes the bug introduced by PR 684 where deploys the CI flow for building and publishing the GH document but does not introduce necessary checks to ensure the
uv.lockfile is pushed everytime the python package dependencies are modified and developer usesuvto run.Root cause
The failing step was building/installing cloudai itself during the docs workflow’s uv sync.
cloudai uses SCM-based dynamic versioning (pdm-backend + source = "scm").
In CI, the repo was checked out in a way that could be shallow / missing full git history/tags, which makes SCM-version computation flaky.
Under that condition, the build backend can produce inconsistent versions between wheel filename and wheel metadata, leading to:
--metadata version: 0.1.dev…+g
--wheel filename version: 0.1.dev…+g.dYYYYMMDD
Tools reject this as an invalid wheel (“metadata version does not match wheel filename”), so the docs job fails during “Set up envir
Test Plan
Additional Notes
Keeping the pytest with pip based flow since we also support pip as described in the documentation.