Skip to content

Conversation

malcolmgreaves
Copy link
Contributor

@malcolmgreaves malcolmgreaves commented Aug 28, 2025

Fixes the nightly property tests by installing uv. Uses the astral-sh action for improved caching.

greptile-apps[bot]

This comment was marked as outdated.

@malcolmgreaves
Copy link
Contributor Author

@greptileai one more time

greptile-apps[bot]

This comment was marked as outdated.

@malcolmgreaves malcolmgreaves force-pushed the mg/fix_ci_nightly_property branch from 93e3dd2 to fd09202 Compare August 28, 2025 20:33
@malcolmgreaves malcolmgreaves changed the title fix: nightly property test & nightly wheel build & release fix: nightly property test Aug 28, 2025
@malcolmgreaves
Copy link
Contributor Author

@greptileai

@malcolmgreaves malcolmgreaves self-assigned this Aug 28, 2025
greptile-apps[bot]

This comment was marked as outdated.

@malcolmgreaves
Copy link
Contributor Author

@greptileai

greptile-apps[bot]

This comment was marked as outdated.

Copy link

codecov bot commented Aug 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.45%. Comparing base (ae638e5) to head (06164a3).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5076      +/-   ##
==========================================
+ Coverage   75.27%   76.45%   +1.17%     
==========================================
  Files         949      949              
  Lines      132520   130350    -2170     
==========================================
- Hits        99761    99665      -96     
+ Misses      32759    30685    -2074     

see 26 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@malcolmgreaves malcolmgreaves force-pushed the mg/fix_ci_nightly_property branch from 06164a3 to a20bf05 Compare August 29, 2025 21:04
@malcolmgreaves malcolmgreaves enabled auto-merge (squash) August 29, 2025 21:07
@malcolmgreaves
Copy link
Contributor Author

@greptileai

greptile-apps[bot]

This comment was marked as outdated.

Copy link
Member

@kevinzwang kevinzwang left a comment

Choose a reason for hiding this comment

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

Just make sure to run it manually and check if it works before merging

@malcolmgreaves
Copy link
Contributor Author

@greptileai

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Summary

This review covers only the changes made since the last review (commit ff23a54), not the entire PR.

The most recent changes address the previous feedback about removing problematic source activate commands from the GitHub Actions workflow. The developer has cleaned up two critical sections where virtual environment activation was being attempted in ways that don't work properly in GitHub Actions:

  1. Rust library build step: Removed the source activate command that was preceding the maturin develop --uv --release command. This activation command would not persist between shell invocations in GitHub Actions and could cause the build to fail.

  2. Test execution step: Removed the source activate command that was preceding the pytest execution. Since the PATH was already modified to include the virtual environment's bin directory, this activation was redundant and potentially problematic.

These changes follow GitHub Actions best practices where environment modifications (like the earlier PATH update with echo "$GITHUB_WORKSPACE/.venv/bin" >> $GITHUB_PATH) are the preferred way to make tools available across workflow steps, rather than using shell-specific activation commands that don't persist.

Confidence score: 4/5

  • This PR is now safe to merge with minimal risk after addressing the activation command issues
  • Score reflects the removal of problematic shell commands that could cause CI failures
  • No files require special attention as the changes are straightforward CI workflow improvements

1 file reviewed, no comments

Edit Code Review Bot Settings | Greptile

@malcolmgreaves malcolmgreaves merged commit 9b28151 into main Aug 29, 2025
52 of 54 checks passed
@malcolmgreaves malcolmgreaves deleted the mg/fix_ci_nightly_property branch August 29, 2025 22:15
Jay-ju pushed a commit to Jay-ju/Daft that referenced this pull request Sep 1, 2025
Fixes the nightly property tests by installing `uv`. 
Uses the astral-sh action for improved caching.
venkateshdb pushed a commit to venkateshdb/Daft that referenced this pull request Sep 6, 2025
Fixes the nightly property tests by installing `uv`. 
Uses the astral-sh action for improved caching.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants