Skip to content
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

Expose num_workers in VideoClips #1359

Merged
merged 1 commit into from
Sep 23, 2019
Merged

Expose num_workers in VideoClips #1359

merged 1 commit into from
Sep 23, 2019

Conversation

ekosman
Copy link
Contributor

@ekosman ekosman commented Sep 22, 2019

I exposed num_workers in videoClips object because using the original value caused errors in my system

@bjuncek
Copy link
Contributor

bjuncek commented Sep 23, 2019

Hi @ekosman, thank you very much, but I think (and @fmassa might confirm) that we're moving towards the custom C++ video reader in the followup release following #1303.

Having said that, this looks harmless enough to mere regardless

@ekosman
Copy link
Contributor Author

ekosman commented Sep 23, 2019

Ok thank you @bjuncek, although we discussed this in #1307 but a new video reader sounds super

@bjuncek
Copy link
Contributor

bjuncek commented Sep 23, 2019

@ekosman fair point - I didn't realize that C++ and PyAv readers are going to co-exist for a while. I call this ok to merge then :)

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot!

@fmassa fmassa merged commit 02a8c0a into pytorch:master Sep 23, 2019
@fmassa
Copy link
Member

fmassa commented Sep 23, 2019

BTW, can you add some documentation explaining this argument? We didn't add it to _precomputed_metadata because it's an internal argument, but I think num_workers should be documented.

@ekosman
Copy link
Contributor Author

ekosman commented Sep 23, 2019

@fmassa Yes, I'm adding documentation for this parameter. btw, I propose using 0 as default value, so the internal data will be loaded in the main process and it will be more consistent with DataLoader (this is the default value for DataLoader)

@fmassa
Copy link
Member

fmassa commented Sep 24, 2019

Sounds good using 0 as a default

@fmassa fmassa mentioned this pull request Oct 31, 2019
fmassa pushed a commit that referenced this pull request Oct 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants