Fix failing tests and mkdocs #1322
Merged
Conversation
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
Contributor
Reviewer's GuideThis PR resolves macOS test failures by pinning FFmpeg to version 7 and cleans up mkdocs compatibility by standardizing docstring formatting and fixing minor typos, alongside a small import refactor for consistent code style. File-Level Changes
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}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- On macOS,
brew install ffmpeg@7is keg-only by default, so you’ll need to either runbrew link --force --overwrite ffmpeg@7or add its bin path to $PATH so CI actually uses the v7 binaries. - For Windows and Ubuntu runners, explicitly pin the ffmpeg major version (e.g.
choco install ffmpeg --version=7.xor using an apt pin) instead of relying on the default install to ensure you don’t accidentally pick up v8 when it’s released. - Consider adding a quick
ffmpeg -versioncheck after installation to fail fast if the wrong version ends up in the CI environment.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- On macOS, `brew install ffmpeg@7` is keg-only by default, so you’ll need to either run `brew link --force --overwrite ffmpeg@7` or add its bin path to $PATH so CI actually uses the v7 binaries.
- For Windows and Ubuntu runners, explicitly pin the ffmpeg major version (e.g. `choco install ffmpeg --version=7.x` or using an apt pin) instead of relying on the default install to ensure you don’t accidentally pick up v8 when it’s released.
- Consider adding a quick `ffmpeg -version` check after installation to fail fast if the wrong version ends up in the CI environment.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1322 +/- ##
=======================================
Coverage 88.84% 88.84%
=======================================
Files 155 155
Lines 14238 14240 +2
Branches 2025 2025
=======================================
+ Hits 12650 12652 +2
Misses 1124 1124
Partials 464 464
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Deploying datachain-documentation with
|
| Latest commit: |
9555181
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://08860c21.datachain-documentation.pages.dev |
| Branch Preview URL: | https://fix-ci-tests-ffmpeg.datachain-documentation.pages.dev |
dreadatour
commented
Sep 8, 2025
Comment on lines
+118
to
+119
| brew install ffmpeg@7 | ||
| echo 'DYLD_FALLBACK_LIBRARY_PATH=/opt/homebrew/opt/ffmpeg@7/lib' >> "$GITHUB_ENV" |
Contributor
Author
There was a problem hiding this comment.
PyTorch does not supports ffmpeg 8
Contributor
There was a problem hiding this comment.
let's put a link to this please meta-pytorch/torchcodec#839 to remove it when it is fixed
Contributor
Author
There was a problem hiding this comment.
Added a comment. Thank you! 🙏
dreadatour
changed the title
4 tasks
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- This PR mixes functional CI changes (ffmpeg pinning) with sweeping docstring/style updates, which makes it harder to review—consider splitting it into separate commits or PRs for clarity and maintainability.
- In tests.yml the ffmpeg version is hardcoded in multiple places; centralizing it via a variable or matrix axis would simplify future updates and reduce duplication.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- This PR mixes functional CI changes (ffmpeg pinning) with sweeping docstring/style updates, which makes it harder to review—consider splitting it into separate commits or PRs for clarity and maintainability.
- In tests.yml the ffmpeg version is hardcoded in multiple places; centralizing it via a variable or matrix axis would simplify future updates and reduce duplication.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
dreadatour
commented
Sep 8, 2025
Comment on lines
+579
to
+582
| name: dataset name. This can be either a fully qualified name, including | ||
| the namespace and project, or just a regular dataset name. In the latter | ||
| case, the namespace and project will be taken from the settings | ||
| (if specified) or from the default values otherwise. |
Contributor
Author
There was a problem hiding this comment.
I have asked AI to help me with this 👀
dreadatour
commented
Sep 8, 2025
Comment on lines
+37
to
+38
| limit: The maximum number of items to read from the HF dataset. | ||
| Applies `take(limit)` to `datasets.load_dataset`. |
Contributor
Author
There was a problem hiding this comment.
Also here, AI was used to improve this docstring.
shcheklein
approved these changes
Sep 8, 2025
amritghimire
approved these changes
Sep 8, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Fix tests:
mkdocs build+ fix some mistypes in docstrings + fix import according to code styleNote: it is better to review this PR with "changes in whitespaces" hidden.
Summary by Sourcery
Fix CI failures by pinning ffmpeg to v7 on macOS and update docstrings and imports for mkdocs build and code style compliance
Bug Fixes:
Enhancements:
CI:
Documentation: