-
Notifications
You must be signed in to change notification settings - Fork 284
torchcodec for linux to use the latest datasets #2424
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
torchcodec for linux to use the latest datasets #2424
Conversation
diffusers | ||
datasets==3.6.0 | ||
datasets==4.0.0; sys_platform == "linux" | ||
datasets==3.6.0; sys_platform != "linux" |
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.
@sbalandi, should I actually fix the version? Since this is a tool, you may want to relax so it's easier to run for people.
There was upper bound initially that's why I touched this file. Maybe the version requirement can be removed completely?
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.
I would prefer to leave:
datasets<=4.0.0; sys_platform == "linux"
datasets<4.0.0; sys_platform != "linux"
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.
But are you sure, that load_dataset is not fail with RuntimeError: Dataset scripts are no longer supported, but found ...
for wwb ?
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.
The current state sets the latest datasets version for linux and it passed
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.
So should wwb's datasets version requirement be relaxed or merge as is?
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.
@Wovchena I'm very sorry for the long answer, please, relax requirements
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.
Relaxed
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.
Pull Request Overview
This PR updates the datasets dependency to use newer versions on Linux platforms and adds torchcodec as a Linux-specific dependency for benchmarking and testing.
- Updates datasets version constraint from exact pin to minimum version in benchmark requirements
- Introduces platform-specific datasets versioning with newer version (4.0.0) for Linux
- Adds torchcodec 0.4.0 as a Linux-only dependency for testing
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
tools/who_what_benchmark/requirements.txt | Changes datasets from exact version pin to minimum version constraint |
tests/python_tests/requirements.txt | Adds platform-specific datasets versioning and torchcodec dependency for Linux |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
be8ab96
No description provided.